From a0f5014192392885e10778ff0b8fe24172cde59c Mon Sep 17 00:00:00 2001 From: Jakob Leben Date: Mon, 12 Jan 2026 21:33:30 -0800 Subject: [PATCH 1/8] CLI improvements --- icechunk/src/cli/interface.rs | 607 +++++++++++++++++++++++++++++++--- 1 file changed, 554 insertions(+), 53 deletions(-) diff --git a/icechunk/src/cli/interface.rs b/icechunk/src/cli/interface.rs index 2cf7279ec..2aa58d7c4 100644 --- a/icechunk/src/cli/interface.rs +++ b/icechunk/src/cli/interface.rs @@ -1,15 +1,20 @@ //! CLI command definitions and handlers. +use crate::format::ChunkIndices; +use crate::format::snapshot::{DimensionName, NodeData}; use crate::repository::VersionInfo; +use chrono::Local; use clap::{Args, Parser, Subcommand}; use dialoguer::{Input, Select}; use futures::stream::StreamExt as _; +use itertools::Itertools as _; use serde_yaml_ng; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::fs::{File, create_dir_all}; use std::io::stdout; use std::path::PathBuf; use std::sync::Arc; +use std::iter::zip; use anyhow::{Context as _, Ok, Result}; @@ -18,6 +23,7 @@ use crate::storage::{ new_tigris_storage, }; use crate::{Repository, RepositoryConfig, Storage, new_s3_storage}; +use crate::format::{SnapshotId, snapshot::SnapshotInfo}; use crate::cli::config::{CliConfig, RepositoryAlias, RepositoryDefinition}; use crate::config::{AzureCredentials, GcsCredentials, S3Credentials, S3Options}; @@ -34,12 +40,20 @@ pub struct IcechunkCLI { #[derive(Debug, Subcommand)] enum Command { - #[command(subcommand, about = "Manage repositories")] - Repo(RepoCommand), - #[command(subcommand, about = "Manage snapshots")] - Snapshot(SnapshotCommand), #[command(subcommand, about = "Manage configuration")] Config(ConfigCommand), + #[command(subcommand, about = "Manage repositories")] + Repo(RepoCommand), + #[clap(name = "history", about = "Show history of a branch, tag or snapshot")] + History(HistoryArgs), + #[clap(name = "inspect", about = "Show snapshot details")] + Inspect(InspectArgs), + #[command(subcommand, about = "Manage branches")] + Branch(BranchCommand), + #[command(subcommand, about = "Manage tags")] + Tag(TagCommand), + #[clap(name = "diff", about = "Show diff between two refs.")] + Diff(DiffArgs) } #[derive(Debug, Subcommand)] @@ -48,10 +62,117 @@ enum RepoCommand { Create(CreateCommand), } +#[derive(Debug, Args)] +struct HistoryArgs { + #[arg(name = "alias", help = "Alias of the repository in the config")] + repo: RepositoryAlias, + #[arg( + name = "reference", + default_value = "main", + help = "Branch, tag or snapshot to show history for." + )] + reference: String, + #[arg(short = 'n', default_value_t = 10, help = "Number of snapshots to list")] + n: usize, +} + +#[derive(Debug, Args)] +struct InspectArgs { + #[arg(name = "alias", help = "Alias of the repository in the config")] + repo: RepositoryAlias, + #[arg( + name = "reference", + default_value = "main", + help = "Branch, tag or snapshot ID." + )] + reference: String, +} + +#[derive(Debug, Subcommand)] +enum BranchCommand { + #[clap(name = "list", about = "List branches")] + List(BranchListArgs), + #[clap(name = "create", about = "Create branch")] + Create(BranchCreateArgs), + #[clap(name = "delete", about = "Delete branch")] + Delete(BranchDeleteArgs), +} + +#[derive(Debug, Args)] +struct BranchListArgs { + #[arg(name = "alias", help = "Alias of the repository in the config")] + repo: RepositoryAlias, +} + +#[derive(Debug, Args)] +struct BranchCreateArgs { + #[arg(name = "alias", help = "Alias of the repository in the config")] + repo: RepositoryAlias, + #[arg(name = "branch", help = "Name of the branch to create")] + branch: String, + #[arg( + name = "from", + default_value = "main", + help = "Branch, tag or snapshot to create the branch from" + )] + from: String, +} + +#[derive(Debug, Args)] +struct BranchDeleteArgs { + #[arg(name = "alias", help = "Alias of the repository in the config")] + repo: RepositoryAlias, + #[arg(name = "branch", help = "Name of the branch to delete")] + branch: String, +} + + #[derive(Debug, Subcommand)] -enum SnapshotCommand { - #[clap(name = "list", about = "List snapshots in a repository")] - List(ListCommand), +enum TagCommand { + #[clap(name = "list", about = "List branches")] + List(TagListArgs), + #[clap(name = "create", about = "Create branch")] + Create(TagCreateArgs), + #[clap(name = "delete", about = "Delete branch")] + Delete(TagDeleteArgs), +} + +#[derive(Debug, Args)] +struct TagListArgs { + #[arg(name = "alias", help = "Alias of the repository in the config")] + repo: RepositoryAlias, +} + +#[derive(Debug, Args)] +struct TagCreateArgs { + #[arg(name = "alias", help = "Alias of the repository in the config")] + repo: RepositoryAlias, + #[arg(name = "tag", help = "Name of the tag to create")] + tag: String, + #[arg( + name = "target", + default_value = "main", + help = "Branch, tag or snapshot to create the tag for" + )] + target: String, +} + +#[derive(Debug, Args)] +struct TagDeleteArgs { + #[arg(name = "alias", help = "Alias of the repository in the config")] + repo: RepositoryAlias, + #[arg(name = "tag", help = "Name of the tag to delete")] + tag: String, +} + +#[derive(Debug, Args)] +struct DiffArgs { + #[arg(name = "alias", help = "Alias of the repository in the config")] + repo: RepositoryAlias, + #[arg(name = "from", help = "Source reference")] + from: String, + #[arg(name = "to", help = "Target reference")] + to: String, } #[derive(Debug, Subcommand)] @@ -93,20 +214,6 @@ struct AddCommand { repo: RepositoryAlias, } -#[derive(Debug, Args)] -struct ListCommand { - repo: RepositoryAlias, - #[arg(short = 'n', default_value_t = 10, help = "Number of snapshots to list")] - n: usize, - #[arg( - short = 'b', - long = "branch", - default_value = "main", - help = "Branch to list snapshots from" - )] - branch: String, -} - const CONFIG_DIR: &str = "icechunk"; const CONFIG_NAME: &str = "cli-config.yaml"; @@ -213,6 +320,54 @@ async fn get_storage( } } +async fn open_repository( + repo_alias: &RepositoryAlias, + config: &CliConfig, +) -> Result { + let repo = + config.repos.get(repo_alias).context(format!("Repository {:?} not found in config", repo_alias))?; + let storage = get_storage(repo).await?; + let config = Some(repo.get_config().clone()); + + let repository = Repository::open(config, Arc::clone(&storage), HashMap::new()) + .await + .context(format!("Failed to open repository {:?}", repo_alias))?; + + Ok(repository) +} + +async fn resolve_ref_by_name( + repository: &Repository, + reference: &String, +) -> Result { + let result = repository.resolve_version(&VersionInfo::BranchTipRef(reference.clone())).await; + match result { + Result::Ok(snapshot_id) => return Ok(snapshot_id), + Err(_) => { /* Try next */ } + } + let result = repository.resolve_version(&VersionInfo::TagRef(reference.clone())).await; + match result { + Result::Ok(snapshot_id) => return Ok(snapshot_id), + Err(_) => { /* Try next */ } + } + + let snapshot_id = SnapshotId::try_from(reference.as_str()); + match snapshot_id { + Result::Ok(id) => { + let result = repository.resolve_version(&VersionInfo::SnapshotId(id)).await; + match result { + Result::Ok(snapshot_id) => return Ok(snapshot_id), + Err(_) => { /* Try next */ } + } + } + Err(_) => { /* Try next */ } + } + Err(anyhow::anyhow!( + "Reference {:?} not found to be a branch, tag or snapshot", + reference + )) +} + async fn repo_create(init_cmd: &CreateCommand, config: &CliConfig) -> Result<()> { let repo = config.repos.get(&init_cmd.repo).context("Repository not found in config")?; @@ -229,32 +384,336 @@ async fn repo_create(init_cmd: &CreateCommand, config: &CliConfig) -> Result<()> Ok(()) } -async fn snapshot_list( - list_cmd: &ListCommand, +async fn list_branches( + args: &BranchListArgs, config: &CliConfig, - mut writer: impl std::io::Write, ) -> Result<()> { - let repo = - config.repos.get(&list_cmd.repo).context("Repository not found in config")?; - let storage = get_storage(repo).await?; - let config = Some(repo.get_config().clone()); + let repository = open_repository(&args.repo, config).await?; + let branches = repository.list_branches().await.context("Failed to list branches")?; + for branch in branches { + let snapshot = repository.lookup_branch(branch.as_str()).await?; + println!("{} {}", snapshot, branch); + } + Ok(()) +} - let repository = Repository::open(config, Arc::clone(&storage), HashMap::new()) +async fn create_branch( + args: &BranchCreateArgs, + config: &CliConfig, +) -> Result<()> { + let repository = open_repository(&args.repo, config).await?; + let from_snapshot = resolve_ref_by_name(&repository, &args.from).await?; + repository + .create_branch(&args.branch, &from_snapshot) .await - .context(format!("Failed to open repository {:?}", list_cmd.repo))?; + .context(format!( + "Failed to create branch {:?} from {:?}", + args.branch, args.from + ))?; + + println!( + "✅ Created branch {:?} from {:?} in repository {:?}", + args.branch, args.from, args.repo + ); - let branch_ref = VersionInfo::BranchTipRef(list_cmd.branch.clone()); - let ancestry = repository.ancestry(&branch_ref).await?; + Ok(()) +} + +async fn delete_branch( + args: &BranchDeleteArgs, + config: &CliConfig, +) -> Result<()> { + let repository = open_repository(&args.repo, config).await?; + + repository + .delete_branch(&args.branch) + .await + .context(format!("Failed to delete branch {:?}", args.branch))?; - let snapshots: Vec<_> = ancestry.take(list_cmd.n).collect().await; + println!( + "✅ Deleted branch {:?} in repository {:?}", + args.branch, args.repo + ); + Ok(()) +} + +async fn list_tags( + args: &TagListArgs, + config: &CliConfig, +) -> Result<()> { + let repository = open_repository(&args.repo, config).await?; + let tags = repository.list_tags().await.context("Failed to list tags")?; + for tag in tags { + let snapshot = repository.lookup_tag(tag.as_str()).await?; + println!("{} {}", snapshot, tag); + } + Ok(()) +} + +async fn create_tag( + args: &TagCreateArgs, + config: &CliConfig, +) -> Result<()> { + let repository = open_repository(&args.repo, config).await?; + let target_snapshot = resolve_ref_by_name(&repository, &args.target).await?; + + repository + .create_tag(&args.tag, &target_snapshot) + .await + .context(format!( + "Failed to create tag {:?} from {:?}", + args.tag, args.target + ))?; + + println!( + "✅ Created tag {:?} for {:?} in repository {:?}", + args.tag, target_snapshot, args.repo + ); + + Ok(()) +} + +async fn delete_tag( + args: &TagDeleteArgs, + config: &CliConfig, +) -> Result<()> { + let repository = open_repository(&args.repo, config).await?; + + repository + .delete_tag(&args.tag) + .await + .context(format!("Failed to delete tag {:?}", args.tag))?; + + println!( + "✅ Deleted tag {:?} in repository {:?}", + args.tag, args.repo + ); + + Ok(()) +} + +fn show_snapshot(mut writer: impl std::io::Write, snapshot: SnapshotInfo, with_meta: bool) -> Result<()> { + writeln!(writer, "Snapshot: {}", snapshot.id)?; + writeln!(writer, "Date: {}", snapshot.flushed_at.with_timezone(&Local).format("%B %d %Y %H:%M:%S"))?; + if with_meta && !snapshot.metadata.is_empty() { + writeln!(writer, "Metadata:")?; + for (key, value) in snapshot.metadata { + writeln!(writer, " {}: {}", key, value)?; + } + } + if !snapshot.message.is_empty() { + writeln!(writer, "\n {}", snapshot.message)?; + } + Ok(()) +} + +async fn history( + args: &HistoryArgs, + config: &CliConfig, + mut writer: impl std::io::Write, +) -> Result<()> { + let repository = open_repository(&args.repo, config).await?; + let snapshot = resolve_ref_by_name(&repository, &args.reference).await?; + let ancestry = repository.ancestry(&VersionInfo::SnapshotId(snapshot)).await?; + let snapshots: Vec<_> = ancestry.take(args.n).collect().await; for snapshot in snapshots { - writeln!(writer, "{:?}", snapshot.context("Failed to get snapshot")?)?; + show_snapshot(&mut writer, snapshot.context("Failed to get snapshot")?, false)?; + writeln!(writer, "")?; } + Ok(()) +} + + +fn array_shape_string(dimension_name: &Option>, values: Vec) -> String { + match dimension_name { + Some(names) => { + zip(names.iter(), values.iter()) + .map(|(name, v)| + match name { + DimensionName::Name(n) => format!("{}={}", n, v), + DimensionName::NotSpecified => format!("{}", v), + } + ) + .join(", ") + }, + None => { + values.join(", ") + } + } +} + +async fn inspect( + args: &InspectArgs, + config: &CliConfig, + mut writer: impl std::io::Write, +) -> Result<()> { + let repository = open_repository(&args.repo, config).await?; + let snapshot_id = resolve_ref_by_name(&repository, &args.reference).await?; + let snapshot = repository.asset_manager().fetch_snapshot(&snapshot_id).await?; + + writeln!(writer, "Snapshot: {}", snapshot_id)?; + writeln!(writer, "Date: {}", snapshot.flushed_at()?.with_timezone(&Local).format("%B %d %Y %H:%M:%S"))?; + let metadata = snapshot.metadata()?; + if !metadata.is_empty() { + writeln!(writer, "Metadata:")?; + for (key, value) in metadata { + writeln!(writer, " {}: {}", key, value)?; + } + } + let message = snapshot.message(); + if !message.is_empty() { + writeln!(writer, "Message:\n {}", message)?; + } + + writeln!(writer, "\n-- Nodes --")?; + for node_info_res in snapshot.iter() { + let node_info = node_info_res.context("Failed to get snapshot info")?; + let node_data = node_info.node_data; + match node_data { + NodeData::Array {shape, dimension_names, manifests} => { + let array_size: Vec<_> = shape.iter().map(|s| s.array_length().to_string()).collect(); + let chunk_size: Vec<_> = shape.iter().map(|s| format!( + "{}({})", + s.chunk_length(), + (s.array_length() + s.chunk_length() - 1)/s.chunk_length() + )).collect(); + writeln!(writer, "{}", node_info.path)?; + writeln!(writer, " size: {}", array_shape_string(&dimension_names, array_size))?; + writeln!(writer, " chunk: {}", array_shape_string(&dimension_names, chunk_size))?; + for manifest in manifests { + let extents = manifest.extents.iter().map(|e| + if e.end - 1 == e.start { + e.start.to_string() + } else { + format!("{}-{}", e.start, e.end - 1) + } + ).collect(); + writeln!(writer, " manifest: {} {}", + manifest.object_id, + array_shape_string(&dimension_names, extents) + )?; + } + }, + NodeData::Group => { + writeln!(writer, "{}", node_info.path)?; + }, + } + } + + writeln!(writer, "\n-- Manifests --")?; + for manifest_info in snapshot.manifest_files() { + writeln!(writer, "{}", manifest_info.id)?; + writeln!(writer, " num chunk refs: {}", manifest_info.num_chunk_refs)?; + // TODO: Human-friendly size + writeln!(writer, " size (bytes): {}", manifest_info.size_bytes)?; + } + Ok(()) } +async fn diff( + args: &DiffArgs, + config: &CliConfig, + mut writer: impl std::io::Write, +) -> Result<()> { + let repository = open_repository(&args.repo, config).await?; + + let from_ref = VersionInfo::SnapshotId(resolve_ref_by_name(&repository, &args.from).await?); + let to_ref = VersionInfo::SnapshotId(resolve_ref_by_name(&repository, &args.to).await?); + + let diff = repository + .diff(&from_ref, &to_ref) + .await + .context(format!( + "Failed to compute diff between {:?} and {:?}", + args.from, args.to + ))?; + + let new_arrays_hash: HashSet<_> = diff.new_arrays.iter().cloned().collect(); + let new_groups_hash: HashSet<_> = diff.new_groups.iter().cloned().collect(); + let deleted_arrays_hash: HashSet<_> = diff.deleted_arrays.iter().cloned().collect(); + let deleted_groups_hash: HashSet<_> = diff.deleted_groups.iter().cloned().collect(); + let updated_arrays_hash: HashSet<_> = diff.updated_arrays.iter().cloned().collect(); + let updated_groups_hash: HashSet<_> = diff.updated_groups.iter().cloned().collect(); + let updated_chunks_hash: HashSet<_> = diff.updated_chunks.keys().cloned().collect(); + + let modified_paths = { + let mut modified_paths: Vec<_> = new_arrays_hash + .union(&new_groups_hash) + .cloned() + .collect::>() + .union(&deleted_arrays_hash) + .cloned() + .collect::>() + .union(&deleted_groups_hash) + .cloned() + .collect::>() + .union(&updated_arrays_hash) + .cloned() + .collect::>() + .union(&updated_groups_hash) + .cloned() + .collect::>() + .union(&updated_chunks_hash) + .cloned() + .collect(); + + modified_paths.sort(); + modified_paths + }; + + + for path in modified_paths { + let is_new = new_arrays_hash.contains(&path) || new_groups_hash.contains(&path); + let is_deleted = deleted_arrays_hash.contains(&path) || deleted_groups_hash.contains(&path); + let is_updated = updated_arrays_hash.contains(&path) || updated_groups_hash.contains(&path); + let has_updated_chunks = updated_chunks_hash.contains(&path); + + // Sometimes a path will be new or deleted and also have updated chunks. + // In that case we do not print updated chunks as they are all new or all deleted. + if is_new && is_deleted { + writeln!(writer, "-+ {}", path.to_string())?; + continue; + } + else if is_new { + writeln!(writer, "+ {}", path.to_string())?; + continue; + } else if is_deleted { + writeln!(writer, "- {}", path.to_string())?; + continue; + } else if is_updated { + writeln!(writer, "~ {}", path.to_string())?; + } else if has_updated_chunks { + writeln!(writer, " {}", path.to_string())?; + } + + fn chunk_to_string(c: &ChunkIndices) -> String { + let idxs = c.0.iter().map(|i| i.to_string()).join(", "); + format!("({})", idxs) + } + + let updated_chunks = diff.updated_chunks.get(&path); + match updated_chunks { + Some(chunks) => { + write!(writer, " Modified {} chunk(s): ", chunks.len())?; + let t = chunks.iter().take(10).map(chunk_to_string).join(", "); + write!(writer, " {}", t)?; + if chunks.len() > 10 { + writeln!(writer, ", ...")?; + } else { + writeln!(writer, "")?; + } + } + None => {} + } + } + + Ok(()) +} + + async fn config_add(add_cmd: &AddCommand, config: &CliConfig) -> Result { if config.repos.contains_key(&add_cmd.repo) { return Err(anyhow::anyhow!("Repository {:?} already exists", add_cmd.repo)); @@ -416,8 +875,41 @@ pub async fn run_cli(args: IcechunkCLI) -> Result<()> { Command::Repo(RepoCommand::Create(init_cmd)) => { repo_create(&init_cmd, &config).await } - Command::Snapshot(SnapshotCommand::List(list_cmd)) => { - snapshot_list(&list_cmd, &config, stdout()).await + Command::History(history_args) => { + history(&history_args, &config, stdout()).await?; + Ok(()) + } + Command::Inspect(inspect_args) => { + inspect(&inspect_args, &config, stdout()).await?; + Ok(()) + } + Command::Branch(BranchCommand::List(list_args)) => { + list_branches(&list_args, &config).await?; + Ok(()) + } + Command::Branch(BranchCommand::Create(create_args)) => { + create_branch(&create_args, &config).await?; + Ok(()) + } + Command::Branch(BranchCommand::Delete(delete_args)) => { + delete_branch(&delete_args, &config).await?; + Ok(()) + } + Command::Tag(TagCommand::List(list_args)) => { + list_tags(&list_args, &config).await?; + Ok(()) + } + Command::Tag(TagCommand::Create(create_args)) => { + create_tag(&create_args, &config).await?; + Ok(()) + } + Command::Tag(TagCommand::Delete(delete_args)) => { + delete_tag(&delete_args, &config).await?; + Ok(()) + } + Command::Diff(diff_args) => { + diff(&diff_args, &config, stdout()).await?; + Ok(()) } Command::Config(ConfigCommand::Init(init_cmd)) => { let new_config = config_init(&init_cmd, &config).await?; @@ -442,6 +934,8 @@ mod tests { use super::*; + use regex::Regex; + #[tokio_test] async fn test_repo_create() { let temp = assert_fs::TempDir::new().unwrap(); @@ -473,7 +967,7 @@ mod tests { } #[tokio_test] - async fn test_snapshot_list() { + async fn test_config_list() { let temp = assert_fs::TempDir::new().unwrap(); let path = temp.path().to_path_buf(); @@ -488,24 +982,17 @@ mod tests { let config = CliConfig { repos }; - let init_cmd = CreateCommand { repo: repo_alias.clone() }; - - repo_create(&init_cmd, &config).await.unwrap(); - - let list_cmd = - ListCommand { repo: repo_alias.clone(), n: 10, branch: "main".to_string() }; - let mut writer = Vec::new(); - snapshot_list(&list_cmd, &config.clone(), &mut writer).await.unwrap(); + + config_list(&config, &mut writer).await.unwrap(); let output = String::from_utf8(writer).unwrap(); - assert_eq!(output.lines().count(), 1); - assert!(output.contains("SnapshotInfo")); + assert!(output.contains("LocalFileSystem")); } #[tokio_test] - async fn test_config_list() { + async fn test_history() { let temp = assert_fs::TempDir::new().unwrap(); let path = temp.path().to_path_buf(); @@ -520,12 +1007,26 @@ mod tests { let config = CliConfig { repos }; - let mut writer = Vec::new(); - - config_list(&config, &mut writer).await.unwrap(); + let init_cmd = CreateCommand { repo: repo_alias.clone() }; + repo_create(&init_cmd, &config).await.unwrap(); + let args = HistoryArgs { + repo: repo_alias.clone(), + reference: "main".to_string(), + n: 10, + }; + let mut writer = Vec::new(); + history(&args, &config, &mut writer).await.unwrap(); let output = String::from_utf8(writer).unwrap(); + println!("{}", output); - assert!(output.contains("LocalFileSystem")); + let re = Regex::new( +r"Snapshot: [0-9A-Z]{20} +Date: \w+ \d{2} \d+ \d{2}:\d{2}:\d{2} + + Repository initialized +" + ).unwrap(); + assert!(re.is_match(output.as_str())); } } From b30542fc80690aa7ff5d2cd2db6027f7972bd0db Mon Sep 17 00:00:00 2001 From: Jakob Leben Date: Mon, 12 Jan 2026 21:42:35 -0800 Subject: [PATCH 2/8] Fix tag command help strings --- icechunk/src/cli/interface.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/icechunk/src/cli/interface.rs b/icechunk/src/cli/interface.rs index 2aa58d7c4..06662f74e 100644 --- a/icechunk/src/cli/interface.rs +++ b/icechunk/src/cli/interface.rs @@ -129,11 +129,11 @@ struct BranchDeleteArgs { #[derive(Debug, Subcommand)] enum TagCommand { - #[clap(name = "list", about = "List branches")] + #[clap(name = "list", about = "List tags")] List(TagListArgs), - #[clap(name = "create", about = "Create branch")] + #[clap(name = "create", about = "Create tag")] Create(TagCreateArgs), - #[clap(name = "delete", about = "Delete branch")] + #[clap(name = "delete", about = "Delete tag")] Delete(TagDeleteArgs), } From 7fc5b872273c16dd59ac5cbcdedb8395d752e9af Mon Sep 17 00:00:00 2001 From: Jakob Leben Date: Sun, 25 Jan 2026 21:44:42 -0800 Subject: [PATCH 3/8] Remove resolve_ref_by_name --- icechunk/src/cli/interface.rs | 64 ++++++++++------------------------- 1 file changed, 17 insertions(+), 47 deletions(-) diff --git a/icechunk/src/cli/interface.rs b/icechunk/src/cli/interface.rs index 06662f74e..8b9081afb 100644 --- a/icechunk/src/cli/interface.rs +++ b/icechunk/src/cli/interface.rs @@ -2,7 +2,7 @@ use crate::format::ChunkIndices; use crate::format::snapshot::{DimensionName, NodeData}; -use crate::repository::VersionInfo; +use crate::repository::{RepositoryErrorKind, VersionInfo}; use chrono::Local; use clap::{Args, Parser, Subcommand}; use dialoguer::{Input, Select}; @@ -68,8 +68,7 @@ struct HistoryArgs { repo: RepositoryAlias, #[arg( name = "reference", - default_value = "main", - help = "Branch, tag or snapshot to show history for." + help = "ID of snapshot to show history for" )] reference: String, #[arg(short = 'n', default_value_t = 10, help = "Number of snapshots to list")] @@ -82,8 +81,7 @@ struct InspectArgs { repo: RepositoryAlias, #[arg( name = "reference", - default_value = "main", - help = "Branch, tag or snapshot ID." + help = "ID of snapshot to inspect" )] reference: String, } @@ -112,8 +110,7 @@ struct BranchCreateArgs { branch: String, #[arg( name = "from", - default_value = "main", - help = "Branch, tag or snapshot to create the branch from" + help = "ID of snapshot to create the branch from" )] from: String, } @@ -151,8 +148,7 @@ struct TagCreateArgs { tag: String, #[arg( name = "target", - default_value = "main", - help = "Branch, tag or snapshot to create the tag for" + help = "ID of snapshot to create the tag for" )] target: String, } @@ -169,9 +165,9 @@ struct TagDeleteArgs { struct DiffArgs { #[arg(name = "alias", help = "Alias of the repository in the config")] repo: RepositoryAlias, - #[arg(name = "from", help = "Source reference")] + #[arg(name = "from", help = "Source snapshot ID")] from: String, - #[arg(name = "to", help = "Target reference")] + #[arg(name = "to", help = "Target snapshot ID")] to: String, } @@ -336,36 +332,10 @@ async fn open_repository( Ok(repository) } -async fn resolve_ref_by_name( - repository: &Repository, - reference: &String, -) -> Result { - let result = repository.resolve_version(&VersionInfo::BranchTipRef(reference.clone())).await; - match result { - Result::Ok(snapshot_id) => return Ok(snapshot_id), - Err(_) => { /* Try next */ } - } - let result = repository.resolve_version(&VersionInfo::TagRef(reference.clone())).await; - match result { - Result::Ok(snapshot_id) => return Ok(snapshot_id), - Err(_) => { /* Try next */ } - } - - let snapshot_id = SnapshotId::try_from(reference.as_str()); - match snapshot_id { - Result::Ok(id) => { - let result = repository.resolve_version(&VersionInfo::SnapshotId(id)).await; - match result { - Result::Ok(snapshot_id) => return Ok(snapshot_id), - Err(_) => { /* Try next */ } - } - } - Err(_) => { /* Try next */ } - } - Err(anyhow::anyhow!( - "Reference {:?} not found to be a branch, tag or snapshot", - reference - )) +fn parse_snapshot(reference: &String) -> Result { + let snapshot_id = SnapshotId::try_from(reference.as_str()) + .map_err(|_| RepositoryErrorKind::InvalidSnapshotId(reference.clone()))?; + Ok(snapshot_id) } async fn repo_create(init_cmd: &CreateCommand, config: &CliConfig) -> Result<()> { @@ -402,7 +372,7 @@ async fn create_branch( config: &CliConfig, ) -> Result<()> { let repository = open_repository(&args.repo, config).await?; - let from_snapshot = resolve_ref_by_name(&repository, &args.from).await?; + let from_snapshot = parse_snapshot(&args.from)?; repository .create_branch(&args.branch, &from_snapshot) .await @@ -456,7 +426,7 @@ async fn create_tag( config: &CliConfig, ) -> Result<()> { let repository = open_repository(&args.repo, config).await?; - let target_snapshot = resolve_ref_by_name(&repository, &args.target).await?; + let target_snapshot = parse_snapshot(&args.target)?; repository .create_tag(&args.tag, &target_snapshot) @@ -514,7 +484,7 @@ async fn history( mut writer: impl std::io::Write, ) -> Result<()> { let repository = open_repository(&args.repo, config).await?; - let snapshot = resolve_ref_by_name(&repository, &args.reference).await?; + let snapshot = parse_snapshot(&args.reference)?; let ancestry = repository.ancestry(&VersionInfo::SnapshotId(snapshot)).await?; let snapshots: Vec<_> = ancestry.take(args.n).collect().await; for snapshot in snapshots { @@ -549,7 +519,7 @@ async fn inspect( mut writer: impl std::io::Write, ) -> Result<()> { let repository = open_repository(&args.repo, config).await?; - let snapshot_id = resolve_ref_by_name(&repository, &args.reference).await?; + let snapshot_id = parse_snapshot(&args.reference)?; let snapshot = repository.asset_manager().fetch_snapshot(&snapshot_id).await?; writeln!(writer, "Snapshot: {}", snapshot_id)?; @@ -620,8 +590,8 @@ async fn diff( ) -> Result<()> { let repository = open_repository(&args.repo, config).await?; - let from_ref = VersionInfo::SnapshotId(resolve_ref_by_name(&repository, &args.from).await?); - let to_ref = VersionInfo::SnapshotId(resolve_ref_by_name(&repository, &args.to).await?); + let from_ref = VersionInfo::SnapshotId(parse_snapshot(&args.from)?); + let to_ref = VersionInfo::SnapshotId(parse_snapshot(&args.to)?); let diff = repository .diff(&from_ref, &to_ref) From f2fa8c05921067c82246bd8af42fac9f5af5a4d7 Mon Sep 17 00:00:00 2001 From: Jakob Leben Date: Sun, 25 Jan 2026 21:51:59 -0800 Subject: [PATCH 4/8] Rename history to ancestry --- icechunk/src/cli/interface.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/icechunk/src/cli/interface.rs b/icechunk/src/cli/interface.rs index 8b9081afb..5222ef552 100644 --- a/icechunk/src/cli/interface.rs +++ b/icechunk/src/cli/interface.rs @@ -44,8 +44,8 @@ enum Command { Config(ConfigCommand), #[command(subcommand, about = "Manage repositories")] Repo(RepoCommand), - #[clap(name = "history", about = "Show history of a branch, tag or snapshot")] - History(HistoryArgs), + #[clap(name = "ancestry", about = "Show ancestry of a branch, tag or snapshot")] + Ancestry(AncestryArgs), #[clap(name = "inspect", about = "Show snapshot details")] Inspect(InspectArgs), #[command(subcommand, about = "Manage branches")] @@ -63,12 +63,12 @@ enum RepoCommand { } #[derive(Debug, Args)] -struct HistoryArgs { +struct AncestryArgs { #[arg(name = "alias", help = "Alias of the repository in the config")] repo: RepositoryAlias, #[arg( name = "reference", - help = "ID of snapshot to show history for" + help = "ID of snapshot to show ancestry for" )] reference: String, #[arg(short = 'n', default_value_t = 10, help = "Number of snapshots to list")] @@ -478,8 +478,8 @@ fn show_snapshot(mut writer: impl std::io::Write, snapshot: SnapshotInfo, with_m Ok(()) } -async fn history( - args: &HistoryArgs, +async fn ancestry( + args: &AncestryArgs, config: &CliConfig, mut writer: impl std::io::Write, ) -> Result<()> { @@ -845,8 +845,8 @@ pub async fn run_cli(args: IcechunkCLI) -> Result<()> { Command::Repo(RepoCommand::Create(init_cmd)) => { repo_create(&init_cmd, &config).await } - Command::History(history_args) => { - history(&history_args, &config, stdout()).await?; + Command::Ancestry(ancestry_args) => { + ancestry(&ancestry_args, &config, stdout()).await?; Ok(()) } Command::Inspect(inspect_args) => { @@ -962,7 +962,7 @@ mod tests { } #[tokio_test] - async fn test_history() { + async fn test_ancestry() { let temp = assert_fs::TempDir::new().unwrap(); let path = temp.path().to_path_buf(); @@ -980,13 +980,13 @@ mod tests { let init_cmd = CreateCommand { repo: repo_alias.clone() }; repo_create(&init_cmd, &config).await.unwrap(); - let args = HistoryArgs { + let args = AncestryArgs { repo: repo_alias.clone(), reference: "main".to_string(), n: 10, }; let mut writer = Vec::new(); - history(&args, &config, &mut writer).await.unwrap(); + ancestry(&args, &config, &mut writer).await.unwrap(); let output = String::from_utf8(writer).unwrap(); println!("{}", output); From 96c72734d033970d00e1f259470466386e6b201e Mon Sep 17 00:00:00 2001 From: DahnJ Date: Sat, 25 Jul 2026 00:21:02 +0200 Subject: [PATCH 5/8] fix: rebase CLI improvements onto current main Fixes compile breaks from the format-crate split and DimensionShape API changes, rewrites inspect to reuse inspect.rs's SnapshotInfoInspect/ ManifestFileInfoInspect instead of re-walking snapshots by hand, adds test coverage for branch/tag lifecycle and inspect/diff, and gives list_branches/list_tags a writer param for testability. Also: stream ancestry results instead of collecting into a Vec, drop an unnecessary Arc::clone of storage, stop leaking test temp dirs by returning the TempDir guard, merge duplicate imports, and standardize clap derive attributes on #[command(...)] instead of the legacy #[clap(...)] alias. Co-Authored-By: Claude Sonnet 5 --- icechunk/src/cli/interface.rs | 526 ++++++++++++++++++++++------------ icechunk/src/inspect.rs | 78 +++-- 2 files changed, 396 insertions(+), 208 deletions(-) diff --git a/icechunk/src/cli/interface.rs b/icechunk/src/cli/interface.rs index 5222ef552..0c4b19e06 100644 --- a/icechunk/src/cli/interface.rs +++ b/icechunk/src/cli/interface.rs @@ -1,7 +1,7 @@ //! CLI command definitions and handlers. -use crate::format::ChunkIndices; -use crate::format::snapshot::{DimensionName, NodeData}; +use crate::format::{ChunkIndices, SnapshotId, snapshot::SnapshotInfo}; +use crate::inspect; use crate::repository::{RepositoryErrorKind, VersionInfo}; use chrono::Local; use clap::{Args, Parser, Subcommand}; @@ -14,7 +14,6 @@ use std::fs::{File, create_dir_all}; use std::io::stdout; use std::path::PathBuf; use std::sync::Arc; -use std::iter::zip; use anyhow::{Context as _, Ok, Result}; @@ -23,7 +22,6 @@ use crate::storage::{ new_tigris_storage, }; use crate::{Repository, RepositoryConfig, Storage, new_s3_storage}; -use crate::format::{SnapshotId, snapshot::SnapshotInfo}; use crate::cli::config::{CliConfig, RepositoryAlias, RepositoryDefinition}; use crate::config::{AzureCredentials, GcsCredentials, S3Credentials, S3Options}; @@ -32,7 +30,7 @@ use dirs::config_dir; use super::config::{AzureRepoLocation, RepoLocation}; #[derive(Debug, Parser)] -#[clap()] +#[command()] pub struct IcechunkCLI { #[command(subcommand)] cmd: Command, @@ -44,21 +42,21 @@ enum Command { Config(ConfigCommand), #[command(subcommand, about = "Manage repositories")] Repo(RepoCommand), - #[clap(name = "ancestry", about = "Show ancestry of a branch, tag or snapshot")] + #[command(name = "ancestry", about = "Show ancestry of a branch, tag or snapshot")] Ancestry(AncestryArgs), - #[clap(name = "inspect", about = "Show snapshot details")] + #[command(name = "inspect", about = "Show snapshot details")] Inspect(InspectArgs), #[command(subcommand, about = "Manage branches")] Branch(BranchCommand), #[command(subcommand, about = "Manage tags")] Tag(TagCommand), - #[clap(name = "diff", about = "Show diff between two refs.")] - Diff(DiffArgs) + #[command(name = "diff", about = "Show diff between two refs.")] + Diff(DiffArgs), } #[derive(Debug, Subcommand)] enum RepoCommand { - #[clap(name = "create", about = "Create a repository")] + #[command(name = "create", about = "Create a repository")] Create(CreateCommand), } @@ -66,10 +64,7 @@ enum RepoCommand { struct AncestryArgs { #[arg(name = "alias", help = "Alias of the repository in the config")] repo: RepositoryAlias, - #[arg( - name = "reference", - help = "ID of snapshot to show ancestry for" - )] + #[arg(name = "reference", help = "ID of snapshot to show ancestry for")] reference: String, #[arg(short = 'n', default_value_t = 10, help = "Number of snapshots to list")] n: usize, @@ -79,20 +74,17 @@ struct AncestryArgs { struct InspectArgs { #[arg(name = "alias", help = "Alias of the repository in the config")] repo: RepositoryAlias, - #[arg( - name = "reference", - help = "ID of snapshot to inspect" - )] + #[arg(name = "reference", help = "ID of snapshot to inspect")] reference: String, } #[derive(Debug, Subcommand)] enum BranchCommand { - #[clap(name = "list", about = "List branches")] + #[command(name = "list", about = "List branches")] List(BranchListArgs), - #[clap(name = "create", about = "Create branch")] + #[command(name = "create", about = "Create branch")] Create(BranchCreateArgs), - #[clap(name = "delete", about = "Delete branch")] + #[command(name = "delete", about = "Delete branch")] Delete(BranchDeleteArgs), } @@ -108,10 +100,7 @@ struct BranchCreateArgs { repo: RepositoryAlias, #[arg(name = "branch", help = "Name of the branch to create")] branch: String, - #[arg( - name = "from", - help = "ID of snapshot to create the branch from" - )] + #[arg(name = "from", help = "ID of snapshot to create the branch from")] from: String, } @@ -123,14 +112,13 @@ struct BranchDeleteArgs { branch: String, } - #[derive(Debug, Subcommand)] enum TagCommand { - #[clap(name = "list", about = "List tags")] + #[command(name = "list", about = "List tags")] List(TagListArgs), - #[clap(name = "create", about = "Create tag")] + #[command(name = "create", about = "Create tag")] Create(TagCreateArgs), - #[clap(name = "delete", about = "Delete tag")] + #[command(name = "delete", about = "Delete tag")] Delete(TagDeleteArgs), } @@ -146,10 +134,7 @@ struct TagCreateArgs { repo: RepositoryAlias, #[arg(name = "tag", help = "Name of the tag to create")] tag: String, - #[arg( - name = "target", - help = "ID of snapshot to create the tag for" - )] + #[arg(name = "target", help = "ID of snapshot to create the tag for")] target: String, } @@ -173,9 +158,9 @@ struct DiffArgs { #[derive(Debug, Subcommand)] enum ConfigCommand { - #[clap(name = "init", about = "Interactively create a new config file.")] + #[command(name = "init", about = "Interactively create a new config file.")] Init(InitCommand), - #[clap( + #[command( name = "add", about = concat!( "Interactively add a repository to the config. ", @@ -183,7 +168,7 @@ enum ConfigCommand { ) )] Add(AddCommand), - #[clap(name = "list", about = "Print the current config.")] + #[command(name = "list", about = "Print the current config.")] List, } @@ -320,21 +305,23 @@ async fn open_repository( repo_alias: &RepositoryAlias, config: &CliConfig, ) -> Result { - let repo = - config.repos.get(repo_alias).context(format!("Repository {:?} not found in config", repo_alias))?; + let repo = config + .repos + .get(repo_alias) + .context(format!("Repository {repo_alias:?} not found in config"))?; let storage = get_storage(repo).await?; let config = Some(repo.get_config().clone()); - let repository = Repository::open(config, Arc::clone(&storage), HashMap::new()) + let repository = Repository::open(config, storage, HashMap::new()) .await - .context(format!("Failed to open repository {:?}", repo_alias))?; + .context(format!("Failed to open repository {repo_alias:?}"))?; Ok(repository) } -fn parse_snapshot(reference: &String) -> Result { - let snapshot_id = SnapshotId::try_from(reference.as_str()) - .map_err(|_| RepositoryErrorKind::InvalidSnapshotId(reference.clone()))?; +fn parse_snapshot(reference: &str) -> Result { + let snapshot_id = SnapshotId::try_from(reference) + .map_err(|_| RepositoryErrorKind::InvalidSnapshotId(reference.to_string()))?; Ok(snapshot_id) } @@ -345,7 +332,7 @@ async fn repo_create(init_cmd: &CreateCommand, config: &CliConfig) -> Result<()> let config = Some(repo.get_config().clone()); - Repository::create(config, Arc::clone(&storage), HashMap::new(), None, true) + Repository::create(config, storage, HashMap::new(), None, true) .await .context(format!("Failed to create repository {:?}", init_cmd.repo))?; @@ -357,29 +344,24 @@ async fn repo_create(init_cmd: &CreateCommand, config: &CliConfig) -> Result<()> async fn list_branches( args: &BranchListArgs, config: &CliConfig, + mut writer: impl std::io::Write, ) -> Result<()> { let repository = open_repository(&args.repo, config).await?; let branches = repository.list_branches().await.context("Failed to list branches")?; for branch in branches { let snapshot = repository.lookup_branch(branch.as_str()).await?; - println!("{} {}", snapshot, branch); + writeln!(writer, "{snapshot} {branch}")?; } Ok(()) } -async fn create_branch( - args: &BranchCreateArgs, - config: &CliConfig, -) -> Result<()> { +async fn create_branch(args: &BranchCreateArgs, config: &CliConfig) -> Result<()> { let repository = open_repository(&args.repo, config).await?; let from_snapshot = parse_snapshot(&args.from)?; - repository - .create_branch(&args.branch, &from_snapshot) - .await - .context(format!( - "Failed to create branch {:?} from {:?}", - args.branch, args.from - ))?; + repository.create_branch(&args.branch, &from_snapshot).await.context(format!( + "Failed to create branch {:?} from {:?}", + args.branch, args.from + ))?; println!( "✅ Created branch {:?} from {:?} in repository {:?}", @@ -389,10 +371,7 @@ async fn create_branch( Ok(()) } -async fn delete_branch( - args: &BranchDeleteArgs, - config: &CliConfig, -) -> Result<()> { +async fn delete_branch(args: &BranchDeleteArgs, config: &CliConfig) -> Result<()> { let repository = open_repository(&args.repo, config).await?; repository @@ -400,10 +379,7 @@ async fn delete_branch( .await .context(format!("Failed to delete branch {:?}", args.branch))?; - println!( - "✅ Deleted branch {:?} in repository {:?}", - args.branch, args.repo - ); + println!("✅ Deleted branch {:?} in repository {:?}", args.branch, args.repo); Ok(()) } @@ -411,30 +387,25 @@ async fn delete_branch( async fn list_tags( args: &TagListArgs, config: &CliConfig, + mut writer: impl std::io::Write, ) -> Result<()> { let repository = open_repository(&args.repo, config).await?; let tags = repository.list_tags().await.context("Failed to list tags")?; for tag in tags { let snapshot = repository.lookup_tag(tag.as_str()).await?; - println!("{} {}", snapshot, tag); + writeln!(writer, "{snapshot} {tag}")?; } Ok(()) } -async fn create_tag( - args: &TagCreateArgs, - config: &CliConfig, -) -> Result<()> { +async fn create_tag(args: &TagCreateArgs, config: &CliConfig) -> Result<()> { let repository = open_repository(&args.repo, config).await?; let target_snapshot = parse_snapshot(&args.target)?; - + repository .create_tag(&args.tag, &target_snapshot) .await - .context(format!( - "Failed to create tag {:?} from {:?}", - args.tag, args.target - ))?; + .context(format!("Failed to create tag {:?} from {:?}", args.tag, args.target))?; println!( "✅ Created tag {:?} for {:?} in repository {:?}", @@ -444,10 +415,7 @@ async fn create_tag( Ok(()) } -async fn delete_tag( - args: &TagDeleteArgs, - config: &CliConfig, -) -> Result<()> { +async fn delete_tag(args: &TagDeleteArgs, config: &CliConfig) -> Result<()> { let repository = open_repository(&args.repo, config).await?; repository @@ -455,21 +423,26 @@ async fn delete_tag( .await .context(format!("Failed to delete tag {:?}", args.tag))?; - println!( - "✅ Deleted tag {:?} in repository {:?}", - args.tag, args.repo - ); + println!("✅ Deleted tag {:?} in repository {:?}", args.tag, args.repo); Ok(()) } -fn show_snapshot(mut writer: impl std::io::Write, snapshot: SnapshotInfo, with_meta: bool) -> Result<()> { +fn show_snapshot( + mut writer: impl std::io::Write, + snapshot: SnapshotInfo, + with_meta: bool, +) -> Result<()> { writeln!(writer, "Snapshot: {}", snapshot.id)?; - writeln!(writer, "Date: {}", snapshot.flushed_at.with_timezone(&Local).format("%B %d %Y %H:%M:%S"))?; + writeln!( + writer, + "Date: {}", + snapshot.flushed_at.with_timezone(&Local).format("%B %d %Y %H:%M:%S") + )?; if with_meta && !snapshot.metadata.is_empty() { writeln!(writer, "Metadata:")?; for (key, value) in snapshot.metadata { - writeln!(writer, " {}: {}", key, value)?; + writeln!(writer, " {key}: {value}")?; } } if !snapshot.message.is_empty() { @@ -485,32 +458,27 @@ async fn ancestry( ) -> Result<()> { let repository = open_repository(&args.repo, config).await?; let snapshot = parse_snapshot(&args.reference)?; - let ancestry = repository.ancestry(&VersionInfo::SnapshotId(snapshot)).await?; - let snapshots: Vec<_> = ancestry.take(args.n).collect().await; - for snapshot in snapshots { + let mut ancestry = Box::pin( + repository.ancestry(&VersionInfo::SnapshotId(snapshot)).await?.take(args.n), + ); + while let Some(snapshot) = ancestry.next().await { show_snapshot(&mut writer, snapshot.context("Failed to get snapshot")?, false)?; - writeln!(writer, "")?; + writeln!(writer)?; } Ok(()) } - -fn array_shape_string(dimension_name: &Option>, values: Vec) -> String { - match dimension_name { - Some(names) => { - zip(names.iter(), values.iter()) - .map(|(name, v)| - match name { - DimensionName::Name(n) => format!("{}={}", n, v), - DimensionName::NotSpecified => format!("{}", v), - } - ) - .join(", ") - }, - None => { - values.join(", ") - } - } +/// Formats one value per array dimension, prefixed by its name when known, +/// e.g. `x=100, y=200` or, for unnamed dimensions, plain `100, 200`. +fn format_per_dim(shape: &[inspect::DimensionShapeInspect], values: &[String]) -> String { + shape + .iter() + .zip(values.iter()) + .map(|(dim, v)| match &dim.name { + Some(n) => format!("{n}={v}"), + None => v.clone(), + }) + .join(", ") } async fn inspect( @@ -520,66 +488,76 @@ async fn inspect( ) -> Result<()> { let repository = open_repository(&args.repo, config).await?; let snapshot_id = parse_snapshot(&args.reference)?; - let snapshot = repository.asset_manager().fetch_snapshot(&snapshot_id).await?; - - writeln!(writer, "Snapshot: {}", snapshot_id)?; - writeln!(writer, "Date: {}", snapshot.flushed_at()?.with_timezone(&Local).format("%B %d %Y %H:%M:%S"))?; - let metadata = snapshot.metadata()?; - if !metadata.is_empty() { + let info = inspect::inspect_snapshot(repository.asset_manager(), &snapshot_id) + .await + .context("Failed to inspect snapshot")?; + + writeln!(writer, "Snapshot: {}", info.id)?; + writeln!( + writer, + "Date: {}", + info.flushed_at.with_timezone(&Local).format("%B %d %Y %H:%M:%S") + )?; + if !info.metadata.is_empty() { writeln!(writer, "Metadata:")?; - for (key, value) in metadata { - writeln!(writer, " {}: {}", key, value)?; + for (key, value) in info.metadata { + writeln!(writer, " {key}: {value}")?; } } - let message = snapshot.message(); - if !message.is_empty() { - writeln!(writer, "Message:\n {}", message)?; + if !info.commit_message.is_empty() { + writeln!(writer, "Message:\n {}", info.commit_message)?; } writeln!(writer, "\n-- Nodes --")?; - for node_info_res in snapshot.iter() { - let node_info = node_info_res.context("Failed to get snapshot info")?; - let node_data = node_info.node_data; - match node_data { - NodeData::Array {shape, dimension_names, manifests} => { - let array_size: Vec<_> = shape.iter().map(|s| s.array_length().to_string()).collect(); - let chunk_size: Vec<_> = shape.iter().map(|s| format!( - "{}({})", - s.chunk_length(), - (s.array_length() + s.chunk_length() - 1)/s.chunk_length() - )).collect(); - writeln!(writer, "{}", node_info.path)?; - writeln!(writer, " size: {}", array_shape_string(&dimension_names, array_size))?; - writeln!(writer, " chunk: {}", array_shape_string(&dimension_names, chunk_size))?; - for manifest in manifests { - let extents = manifest.extents.iter().map(|e| - if e.end - 1 == e.start { - e.start.to_string() + for node in info.nodes { + writeln!(writer, "{}", node.path)?; + if let Some(shape) = &node.shape { + let sizes: Vec = + shape.iter().map(|d| d.array_length.to_string()).collect(); + let chunks: Vec = shape + .iter() + .map(|d| { + let chunk_length = if d.num_chunks > 0 { + d.array_length.div_ceil(d.num_chunks as u64) + } else { + 0 + }; + format!("{chunk_length}({})", d.num_chunks) + }) + .collect(); + writeln!(writer, " size: {}", format_per_dim(shape, &sizes))?; + writeln!(writer, " chunk: {}", format_per_dim(shape, &chunks))?; + + for manifest_ref in node.manifest_refs.iter().flatten() { + let extents: Vec = manifest_ref + .extents + .iter() + .map(|(start, end)| { + if *end == start + 1 { + start.to_string() } else { - format!("{}-{}", e.start, e.end - 1) + format!("{start}-{}", end - 1) } - ).collect(); - writeln!(writer, " manifest: {} {}", - manifest.object_id, - array_shape_string(&dimension_names, extents) - )?; - } - }, - NodeData::Group => { - writeln!(writer, "{}", node_info.path)?; - }, + }) + .collect(); + writeln!( + writer, + " manifest: {} {}", + manifest_ref.id, + format_per_dim(shape, &extents) + )?; + } } } writeln!(writer, "\n-- Manifests --")?; - for manifest_info in snapshot.manifest_files() { - writeln!(writer, "{}", manifest_info.id)?; - writeln!(writer, " num chunk refs: {}", manifest_info.num_chunk_refs)?; + for manifest in info.manifests { + writeln!(writer, "{}", manifest.id)?; + writeln!(writer, " num chunk refs: {}", manifest.num_chunk_refs)?; // TODO: Human-friendly size - writeln!(writer, " size (bytes): {}", manifest_info.size_bytes)?; + writeln!(writer, " size (bytes): {}", manifest.size_bytes)?; } - Ok(()) } @@ -593,13 +571,10 @@ async fn diff( let from_ref = VersionInfo::SnapshotId(parse_snapshot(&args.from)?); let to_ref = VersionInfo::SnapshotId(parse_snapshot(&args.to)?); - let diff = repository - .diff(&from_ref, &to_ref) - .await - .context(format!( - "Failed to compute diff between {:?} and {:?}", - args.from, args.to - ))?; + let diff = repository.diff(&from_ref, &to_ref).await.context(format!( + "Failed to compute diff between {:?} and {:?}", + args.from, args.to + ))?; let new_arrays_hash: HashSet<_> = diff.new_arrays.iter().cloned().collect(); let new_groups_hash: HashSet<_> = diff.new_groups.iter().cloned().collect(); @@ -634,56 +609,52 @@ async fn diff( modified_paths }; - for path in modified_paths { let is_new = new_arrays_hash.contains(&path) || new_groups_hash.contains(&path); - let is_deleted = deleted_arrays_hash.contains(&path) || deleted_groups_hash.contains(&path); - let is_updated = updated_arrays_hash.contains(&path) || updated_groups_hash.contains(&path); + let is_deleted = + deleted_arrays_hash.contains(&path) || deleted_groups_hash.contains(&path); + let is_updated = + updated_arrays_hash.contains(&path) || updated_groups_hash.contains(&path); let has_updated_chunks = updated_chunks_hash.contains(&path); // Sometimes a path will be new or deleted and also have updated chunks. // In that case we do not print updated chunks as they are all new or all deleted. if is_new && is_deleted { - writeln!(writer, "-+ {}", path.to_string())?; + writeln!(writer, "-+ {path}")?; continue; - } - else if is_new { - writeln!(writer, "+ {}", path.to_string())?; + } else if is_new { + writeln!(writer, "+ {path}")?; continue; } else if is_deleted { - writeln!(writer, "- {}", path.to_string())?; + writeln!(writer, "- {path}")?; continue; } else if is_updated { - writeln!(writer, "~ {}", path.to_string())?; + writeln!(writer, "~ {path}")?; } else if has_updated_chunks { - writeln!(writer, " {}", path.to_string())?; + writeln!(writer, " {path}")?; } fn chunk_to_string(c: &ChunkIndices) -> String { let idxs = c.0.iter().map(|i| i.to_string()).join(", "); - format!("({})", idxs) + format!("({idxs})") } let updated_chunks = diff.updated_chunks.get(&path); - match updated_chunks { - Some(chunks) => { - write!(writer, " Modified {} chunk(s): ", chunks.len())?; - let t = chunks.iter().take(10).map(chunk_to_string).join(", "); - write!(writer, " {}", t)?; - if chunks.len() > 10 { - writeln!(writer, ", ...")?; - } else { - writeln!(writer, "")?; - } + if let Some(chunks) = updated_chunks { + write!(writer, " Modified {} chunk(s): ", chunks.len())?; + let t = chunks.iter().take(10).map(chunk_to_string).join(", "); + write!(writer, " {t}")?; + if chunks.len() > 10 { + writeln!(writer, ", ...")?; + } else { + writeln!(writer)?; } - None => {} } } Ok(()) } - async fn config_add(add_cmd: &AddCommand, config: &CliConfig) -> Result { if config.repos.contains_key(&add_cmd.repo) { return Err(anyhow::anyhow!("Repository {:?} already exists", add_cmd.repo)); @@ -854,7 +825,7 @@ pub async fn run_cli(args: IcechunkCLI) -> Result<()> { Ok(()) } Command::Branch(BranchCommand::List(list_args)) => { - list_branches(&list_args, &config).await?; + list_branches(&list_args, &config, stdout()).await?; Ok(()) } Command::Branch(BranchCommand::Create(create_args)) => { @@ -866,7 +837,7 @@ pub async fn run_cli(args: IcechunkCLI) -> Result<()> { Ok(()) } Command::Tag(TagCommand::List(list_args)) => { - list_tags(&list_args, &config).await?; + list_tags(&list_args, &config, stdout()).await?; Ok(()) } Command::Tag(TagCommand::Create(create_args)) => { @@ -900,9 +871,11 @@ pub async fn run_cli(args: IcechunkCLI) -> Result<()> { mod tests { use std::fs::read_dir; + use bytes::Bytes; use icechunk_macros::tokio_test; use super::*; + use crate::format::Path; use regex::Regex; @@ -980,9 +953,12 @@ mod tests { let init_cmd = CreateCommand { repo: repo_alias.clone() }; repo_create(&init_cmd, &config).await.unwrap(); + let repository = open_repository(&repo_alias, &config).await.unwrap(); + let main_tip = repository.lookup_branch("main").await.unwrap(); + let args = AncestryArgs { repo: repo_alias.clone(), - reference: "main".to_string(), + reference: main_tip.to_string(), n: 10, }; let mut writer = Vec::new(); @@ -991,12 +967,188 @@ mod tests { println!("{}", output); let re = Regex::new( -r"Snapshot: [0-9A-Z]{20} + r"Snapshot: [0-9A-Z]{20} Date: \w+ \d{2} \d+ \d{2}:\d{2}:\d{2} Repository initialized -" - ).unwrap(); +", + ) + .unwrap(); assert!(re.is_match(output.as_str())); } + + /// Creates a fresh local-filesystem repo for a test, returning its alias, + /// config, root snapshot id, and the backing temp dir. The temp dir must + /// be kept alive (bind it, don't drop it) for as long as the repo is used + /// -- it deletes its directory on drop. + async fn setup_test_repo() + -> (RepositoryAlias, CliConfig, SnapshotId, assert_fs::TempDir) { + let temp = assert_fs::TempDir::new().unwrap(); + let path = temp.path().to_path_buf(); + + let repo_alias = RepositoryAlias("test-repo".to_string()); + let repo = RepositoryDefinition::LocalFileSystem { + path, + config: RepositoryConfig::default(), + }; + + let mut repos = HashMap::new(); + repos.insert(repo_alias.clone(), repo); + let config = CliConfig { repos }; + + let init_cmd = CreateCommand { repo: repo_alias.clone() }; + repo_create(&init_cmd, &config).await.unwrap(); + + let repository = open_repository(&repo_alias, &config).await.unwrap(); + let root = repository.lookup_branch("main").await.unwrap(); + + (repo_alias, config, root, temp) + } + + #[tokio_test] + async fn test_branch_lifecycle() { + let (repo_alias, config, root, _temp) = setup_test_repo().await; + + create_branch( + &BranchCreateArgs { + repo: repo_alias.clone(), + branch: "feature".to_string(), + from: root.to_string(), + }, + &config, + ) + .await + .unwrap(); + + let mut writer = Vec::new(); + list_branches(&BranchListArgs { repo: repo_alias.clone() }, &config, &mut writer) + .await + .unwrap(); + let output = String::from_utf8(writer).unwrap(); + assert!(output.contains("feature")); + assert!(output.contains("main")); + + delete_branch( + &BranchDeleteArgs { repo: repo_alias.clone(), branch: "feature".to_string() }, + &config, + ) + .await + .unwrap(); + + let mut writer = Vec::new(); + list_branches(&BranchListArgs { repo: repo_alias.clone() }, &config, &mut writer) + .await + .unwrap(); + let output = String::from_utf8(writer).unwrap(); + assert!(!output.contains("feature")); + assert!(output.contains("main")); + } + + #[tokio_test] + async fn test_tag_lifecycle() { + let (repo_alias, config, root, _temp) = setup_test_repo().await; + + create_tag( + &TagCreateArgs { + repo: repo_alias.clone(), + tag: "v1".to_string(), + target: root.to_string(), + }, + &config, + ) + .await + .unwrap(); + + let mut writer = Vec::new(); + list_tags(&TagListArgs { repo: repo_alias.clone() }, &config, &mut writer) + .await + .unwrap(); + let output = String::from_utf8(writer).unwrap(); + assert!(output.contains("v1")); + + delete_tag( + &TagDeleteArgs { repo: repo_alias.clone(), tag: "v1".to_string() }, + &config, + ) + .await + .unwrap(); + + let mut writer = Vec::new(); + list_tags(&TagListArgs { repo: repo_alias.clone() }, &config, &mut writer) + .await + .unwrap(); + let output = String::from_utf8(writer).unwrap(); + assert!(!output.contains("v1")); + } + + #[tokio_test] + async fn test_inspect() { + let (repo_alias, config, root, _temp) = setup_test_repo().await; + + let repository = open_repository(&repo_alias, &config).await.unwrap(); + let mut session = repository.writable_session("main").await.unwrap(); + session + .add_group(Path::try_from("/data").unwrap(), Bytes::copy_from_slice(b"")) + .await + .unwrap(); + let new_snap = + session.commit("add group").max_concurrent_nodes(8).execute().await.unwrap(); + + let mut writer = Vec::new(); + inspect( + &InspectArgs { repo: repo_alias.clone(), reference: new_snap.to_string() }, + &config, + &mut writer, + ) + .await + .unwrap(); + let output = String::from_utf8(writer).unwrap(); + + assert!(output.contains(&new_snap.to_string())); + assert!(output.contains("-- Nodes --")); + assert!(output.contains("/data")); + assert!(output.contains("-- Manifests --")); + + // root snapshot has no nodes yet + let mut writer = Vec::new(); + inspect( + &InspectArgs { repo: repo_alias.clone(), reference: root.to_string() }, + &config, + &mut writer, + ) + .await + .unwrap(); + let output = String::from_utf8(writer).unwrap(); + assert!(!output.contains("/data")); + } + + #[tokio_test] + async fn test_diff() { + let (repo_alias, config, root, _temp) = setup_test_repo().await; + + let repository = open_repository(&repo_alias, &config).await.unwrap(); + let mut session = repository.writable_session("main").await.unwrap(); + session + .add_group(Path::try_from("/data").unwrap(), Bytes::copy_from_slice(b"")) + .await + .unwrap(); + let new_snap = + session.commit("add group").max_concurrent_nodes(8).execute().await.unwrap(); + + let mut writer = Vec::new(); + diff( + &DiffArgs { + repo: repo_alias.clone(), + from: root.to_string(), + to: new_snap.to_string(), + }, + &config, + &mut writer, + ) + .await + .unwrap(); + let output = String::from_utf8(writer).unwrap(); + + assert!(output.contains("+ /data")); + } } diff --git a/icechunk/src/inspect.rs b/icechunk/src/inspect.rs index 426bb6a6a..a034c3c3f 100644 --- a/icechunk/src/inspect.rs +++ b/icechunk/src/inspect.rs @@ -18,7 +18,8 @@ use crate::{ manifest::{ChunkPayload, ManifestRef}, repo_info::UpdateType, snapshot::{ - ManifestFileInfo, NodeData, NodeSnapshot, NodeType, SnapshotProperties, + DimensionName, ManifestFileInfo, NodeData, NodeSnapshot, NodeType, + SnapshotProperties, }, transaction_log::TransactionLog, }, @@ -28,10 +29,10 @@ use crate::{ use icechunk_types::{ICResultExt as _, error::ICResultCtxExt as _}; #[derive(Debug, Serialize, Deserialize)] -struct ManifestFileInfoInspect { - id: String, - size_bytes: u64, - num_chunk_refs: u32, +pub(crate) struct ManifestFileInfoInspect { + pub(crate) id: String, + pub(crate) size_bytes: u64, + pub(crate) num_chunk_refs: u32, } impl From for ManifestFileInfoInspect { @@ -45,9 +46,9 @@ impl From for ManifestFileInfoInspect { } #[derive(Debug, Serialize, Deserialize)] -struct ManifestRefInspect { - id: String, - extents: Vec<(u32, u32)>, +pub(crate) struct ManifestRefInspect { + pub(crate) id: String, + pub(crate) extents: Vec<(u32, u32)>, } impl From for ManifestRefInspect { @@ -60,12 +61,22 @@ impl From for ManifestRefInspect { } #[derive(Debug, Serialize, Deserialize)] -struct NodeSnapshotInspect { - id: String, - path: String, - node_type: String, +pub(crate) struct DimensionShapeInspect { + #[serde(skip_serializing_if = "Option::is_none")] + pub(crate) name: Option, + pub(crate) array_length: u64, + pub(crate) num_chunks: u32, +} + +#[derive(Debug, Serialize, Deserialize)] +pub(crate) struct NodeSnapshotInspect { + pub(crate) id: String, + pub(crate) path: String, + pub(crate) node_type: String, #[serde(skip_serializing_if = "Option::is_none")] - manifest_refs: Option>, + pub(crate) shape: Option>, + #[serde(skip_serializing_if = "Option::is_none")] + pub(crate) manifest_refs: Option>, } impl From for NodeSnapshotInspect { @@ -77,6 +88,31 @@ impl From for NodeSnapshotInspect { NodeType::Group => "group".to_string(), NodeType::Array => "array".to_string(), }, + shape: match &value.node_data { + NodeData::Array { shape, dimension_names, .. } => { + let names: Vec> = match dimension_names { + Some(names) => names + .iter() + .map(|n| match n { + DimensionName::Name(n) => Some(n.clone()), + DimensionName::NotSpecified => None, + }) + .collect(), + None => vec![None; shape.len()], + }; + let dims = shape + .iter() + .zip(names) + .map(|(dim, name)| DimensionShapeInspect { + name, + array_length: dim.array_length(), + num_chunks: dim.num_chunks(), + }) + .collect(); + Some(dims) + } + NodeData::Group => None, + }, manifest_refs: match value.node_data { NodeData::Array { manifests, .. } => { let ms = manifests.into_iter().map(|m| m.into()).collect(); @@ -89,20 +125,20 @@ impl From for NodeSnapshotInspect { } #[derive(Debug, Serialize, Deserialize)] -struct SnapshotInfoInspect { +pub(crate) struct SnapshotInfoInspect { // TODO: add fields //path: String, //size_bytes: u64, - id: String, - flushed_at: DateTime, - commit_message: String, - metadata: SnapshotProperties, + pub(crate) id: String, + pub(crate) flushed_at: DateTime, + pub(crate) commit_message: String, + pub(crate) metadata: SnapshotProperties, - manifests: Vec, - nodes: Vec, + pub(crate) manifests: Vec, + pub(crate) nodes: Vec, } -async fn inspect_snapshot( +pub(crate) async fn inspect_snapshot( asset_manager: &AssetManager, id: &SnapshotId, ) -> RepositoryResult { From 8d1b8fe8b63b28d800378c34ba7ce222aae5b641 Mon Sep 17 00:00:00 2001 From: DahnJ Date: Sat, 25 Jul 2026 00:21:09 +0200 Subject: [PATCH 6/8] feat: resolve branch/tag names as CLI references, default ancestry to main References passed to ancestry/branch create/tag create can now be a branch name or tag name, not just a raw snapshot id, matching what these commands already claimed to support. ancestry also defaults its reference argument to "main" when omitted. Co-Authored-By: Claude Sonnet 5 --- icechunk/src/cli/interface.rs | 42 ++++++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/icechunk/src/cli/interface.rs b/icechunk/src/cli/interface.rs index 0c4b19e06..a145a88a8 100644 --- a/icechunk/src/cli/interface.rs +++ b/icechunk/src/cli/interface.rs @@ -2,7 +2,7 @@ use crate::format::{ChunkIndices, SnapshotId, snapshot::SnapshotInfo}; use crate::inspect; -use crate::repository::{RepositoryErrorKind, VersionInfo}; +use crate::repository::VersionInfo; use chrono::Local; use clap::{Args, Parser, Subcommand}; use dialoguer::{Input, Select}; @@ -64,7 +64,11 @@ enum RepoCommand { struct AncestryArgs { #[arg(name = "alias", help = "Alias of the repository in the config")] repo: RepositoryAlias, - #[arg(name = "reference", help = "ID of snapshot to show ancestry for")] + #[arg( + name = "reference", + default_value = "main", + help = "Branch, tag, or snapshot id to show ancestry for" + )] reference: String, #[arg(short = 'n', default_value_t = 10, help = "Number of snapshots to list")] n: usize, @@ -74,7 +78,11 @@ struct AncestryArgs { struct InspectArgs { #[arg(name = "alias", help = "Alias of the repository in the config")] repo: RepositoryAlias, - #[arg(name = "reference", help = "ID of snapshot to inspect")] + #[arg( + name = "reference", + default_value = "main", + help = "Branch, tag, or snapshot id to inspect" + )] reference: String, } @@ -319,10 +327,17 @@ async fn open_repository( Ok(repository) } -fn parse_snapshot(reference: &str) -> Result { - let snapshot_id = SnapshotId::try_from(reference) - .map_err(|_| RepositoryErrorKind::InvalidSnapshotId(reference.to_string()))?; - Ok(snapshot_id) +async fn parse_reference(repository: &Repository, reference: &str) -> Result { + if let Some(snapshot_id) = SnapshotId::try_from(reference).ok() { + return Ok(snapshot_id); + } + if let Some(snapshot_id) = repository.lookup_branch(reference).await.ok() { + return Ok(snapshot_id); + } + if let Some(snapshot_id) = repository.lookup_tag(reference).await.ok() { + return Ok(snapshot_id); + } + Err(anyhow::anyhow!("`{reference}` is not a valid snapshot id, branch, or tag")) } async fn repo_create(init_cmd: &CreateCommand, config: &CliConfig) -> Result<()> { @@ -357,7 +372,7 @@ async fn list_branches( async fn create_branch(args: &BranchCreateArgs, config: &CliConfig) -> Result<()> { let repository = open_repository(&args.repo, config).await?; - let from_snapshot = parse_snapshot(&args.from)?; + let from_snapshot = parse_reference(&repository, &args.from).await?; repository.create_branch(&args.branch, &from_snapshot).await.context(format!( "Failed to create branch {:?} from {:?}", args.branch, args.from @@ -400,7 +415,7 @@ async fn list_tags( async fn create_tag(args: &TagCreateArgs, config: &CliConfig) -> Result<()> { let repository = open_repository(&args.repo, config).await?; - let target_snapshot = parse_snapshot(&args.target)?; + let target_snapshot = parse_reference(&repository, &args.target).await?; repository .create_tag(&args.tag, &target_snapshot) @@ -457,7 +472,7 @@ async fn ancestry( mut writer: impl std::io::Write, ) -> Result<()> { let repository = open_repository(&args.repo, config).await?; - let snapshot = parse_snapshot(&args.reference)?; + let snapshot = parse_reference(&repository, &args.reference).await?; let mut ancestry = Box::pin( repository.ancestry(&VersionInfo::SnapshotId(snapshot)).await?.take(args.n), ); @@ -487,7 +502,7 @@ async fn inspect( mut writer: impl std::io::Write, ) -> Result<()> { let repository = open_repository(&args.repo, config).await?; - let snapshot_id = parse_snapshot(&args.reference)?; + let snapshot_id = parse_reference(&repository, &args.reference).await?; let info = inspect::inspect_snapshot(repository.asset_manager(), &snapshot_id) .await .context("Failed to inspect snapshot")?; @@ -568,8 +583,9 @@ async fn diff( ) -> Result<()> { let repository = open_repository(&args.repo, config).await?; - let from_ref = VersionInfo::SnapshotId(parse_snapshot(&args.from)?); - let to_ref = VersionInfo::SnapshotId(parse_snapshot(&args.to)?); + let from_ref = + VersionInfo::SnapshotId(parse_reference(&repository, &args.from).await?); + let to_ref = VersionInfo::SnapshotId(parse_reference(&repository, &args.to).await?); let diff = repository.diff(&from_ref, &to_ref).await.context(format!( "Failed to compute diff between {:?} and {:?}", From 3821116da4a6ce3265e68bc16fe8c6a81479ff0c Mon Sep 17 00:00:00 2001 From: DahnJ Date: Sat, 25 Jul 2026 00:21:21 +0200 Subject: [PATCH 7/8] Remove inspect and diff subcommands, improve test coverage Splits inspect/diff out to a separate branch (cli-diff-inspect) to keep this branch focused on the simpler, already-verified ancestry/ branch/tag subcommands. Also inverts control of setup_test_repo's TempDir (caller creates and owns it), adds test coverage for parse_reference's branch/tag/snapshot resolution paths and its error case, and fixes stale help text on branch/tag create's ref argument. Co-Authored-By: Claude Sonnet 5 --- icechunk/src/cli/interface.rs | 336 ++++------------------------------ 1 file changed, 34 insertions(+), 302 deletions(-) diff --git a/icechunk/src/cli/interface.rs b/icechunk/src/cli/interface.rs index a145a88a8..84f7241e7 100644 --- a/icechunk/src/cli/interface.rs +++ b/icechunk/src/cli/interface.rs @@ -1,21 +1,19 @@ //! CLI command definitions and handlers. -use crate::format::{ChunkIndices, SnapshotId, snapshot::SnapshotInfo}; -use crate::inspect; +use crate::format::{SnapshotId, snapshot::SnapshotInfo}; use crate::repository::VersionInfo; use chrono::Local; use clap::{Args, Parser, Subcommand}; use dialoguer::{Input, Select}; use futures::stream::StreamExt as _; -use itertools::Itertools as _; use serde_yaml_ng; -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use std::fs::{File, create_dir_all}; use std::io::stdout; use std::path::PathBuf; use std::sync::Arc; -use anyhow::{Context as _, Ok, Result}; +use anyhow::{Context as _, Result}; use crate::storage::{ new_azure_blob_storage, new_gcs_storage, new_local_filesystem_storage, @@ -44,14 +42,10 @@ enum Command { Repo(RepoCommand), #[command(name = "ancestry", about = "Show ancestry of a branch, tag or snapshot")] Ancestry(AncestryArgs), - #[command(name = "inspect", about = "Show snapshot details")] - Inspect(InspectArgs), #[command(subcommand, about = "Manage branches")] Branch(BranchCommand), #[command(subcommand, about = "Manage tags")] Tag(TagCommand), - #[command(name = "diff", about = "Show diff between two refs.")] - Diff(DiffArgs), } #[derive(Debug, Subcommand)] @@ -74,18 +68,6 @@ struct AncestryArgs { n: usize, } -#[derive(Debug, Args)] -struct InspectArgs { - #[arg(name = "alias", help = "Alias of the repository in the config")] - repo: RepositoryAlias, - #[arg( - name = "reference", - default_value = "main", - help = "Branch, tag, or snapshot id to inspect" - )] - reference: String, -} - #[derive(Debug, Subcommand)] enum BranchCommand { #[command(name = "list", about = "List branches")] @@ -108,7 +90,7 @@ struct BranchCreateArgs { repo: RepositoryAlias, #[arg(name = "branch", help = "Name of the branch to create")] branch: String, - #[arg(name = "from", help = "ID of snapshot to create the branch from")] + #[arg(name = "from", help = "Branch, tag, or snapshot id to create the branch from")] from: String, } @@ -142,7 +124,7 @@ struct TagCreateArgs { repo: RepositoryAlias, #[arg(name = "tag", help = "Name of the tag to create")] tag: String, - #[arg(name = "target", help = "ID of snapshot to create the tag for")] + #[arg(name = "target", help = "Branch, tag, or snapshot id to create the tag for")] target: String, } @@ -154,16 +136,6 @@ struct TagDeleteArgs { tag: String, } -#[derive(Debug, Args)] -struct DiffArgs { - #[arg(name = "alias", help = "Alias of the repository in the config")] - repo: RepositoryAlias, - #[arg(name = "from", help = "Source snapshot ID")] - from: String, - #[arg(name = "to", help = "Target snapshot ID")] - to: String, -} - #[derive(Debug, Subcommand)] enum ConfigCommand { #[command(name = "init", about = "Interactively create a new config file.")] @@ -328,13 +300,13 @@ async fn open_repository( } async fn parse_reference(repository: &Repository, reference: &str) -> Result { - if let Some(snapshot_id) = SnapshotId::try_from(reference).ok() { + if let Ok(snapshot_id) = SnapshotId::try_from(reference) { return Ok(snapshot_id); } - if let Some(snapshot_id) = repository.lookup_branch(reference).await.ok() { + if let Ok(snapshot_id) = repository.lookup_branch(reference).await { return Ok(snapshot_id); } - if let Some(snapshot_id) = repository.lookup_tag(reference).await.ok() { + if let Ok(snapshot_id) = repository.lookup_tag(reference).await { return Ok(snapshot_id); } Err(anyhow::anyhow!("`{reference}` is not a valid snapshot id, branch, or tag")) @@ -483,194 +455,6 @@ async fn ancestry( Ok(()) } -/// Formats one value per array dimension, prefixed by its name when known, -/// e.g. `x=100, y=200` or, for unnamed dimensions, plain `100, 200`. -fn format_per_dim(shape: &[inspect::DimensionShapeInspect], values: &[String]) -> String { - shape - .iter() - .zip(values.iter()) - .map(|(dim, v)| match &dim.name { - Some(n) => format!("{n}={v}"), - None => v.clone(), - }) - .join(", ") -} - -async fn inspect( - args: &InspectArgs, - config: &CliConfig, - mut writer: impl std::io::Write, -) -> Result<()> { - let repository = open_repository(&args.repo, config).await?; - let snapshot_id = parse_reference(&repository, &args.reference).await?; - let info = inspect::inspect_snapshot(repository.asset_manager(), &snapshot_id) - .await - .context("Failed to inspect snapshot")?; - - writeln!(writer, "Snapshot: {}", info.id)?; - writeln!( - writer, - "Date: {}", - info.flushed_at.with_timezone(&Local).format("%B %d %Y %H:%M:%S") - )?; - if !info.metadata.is_empty() { - writeln!(writer, "Metadata:")?; - for (key, value) in info.metadata { - writeln!(writer, " {key}: {value}")?; - } - } - if !info.commit_message.is_empty() { - writeln!(writer, "Message:\n {}", info.commit_message)?; - } - - writeln!(writer, "\n-- Nodes --")?; - for node in info.nodes { - writeln!(writer, "{}", node.path)?; - if let Some(shape) = &node.shape { - let sizes: Vec = - shape.iter().map(|d| d.array_length.to_string()).collect(); - let chunks: Vec = shape - .iter() - .map(|d| { - let chunk_length = if d.num_chunks > 0 { - d.array_length.div_ceil(d.num_chunks as u64) - } else { - 0 - }; - format!("{chunk_length}({})", d.num_chunks) - }) - .collect(); - writeln!(writer, " size: {}", format_per_dim(shape, &sizes))?; - writeln!(writer, " chunk: {}", format_per_dim(shape, &chunks))?; - - for manifest_ref in node.manifest_refs.iter().flatten() { - let extents: Vec = manifest_ref - .extents - .iter() - .map(|(start, end)| { - if *end == start + 1 { - start.to_string() - } else { - format!("{start}-{}", end - 1) - } - }) - .collect(); - writeln!( - writer, - " manifest: {} {}", - manifest_ref.id, - format_per_dim(shape, &extents) - )?; - } - } - } - - writeln!(writer, "\n-- Manifests --")?; - for manifest in info.manifests { - writeln!(writer, "{}", manifest.id)?; - writeln!(writer, " num chunk refs: {}", manifest.num_chunk_refs)?; - // TODO: Human-friendly size - writeln!(writer, " size (bytes): {}", manifest.size_bytes)?; - } - - Ok(()) -} - -async fn diff( - args: &DiffArgs, - config: &CliConfig, - mut writer: impl std::io::Write, -) -> Result<()> { - let repository = open_repository(&args.repo, config).await?; - - let from_ref = - VersionInfo::SnapshotId(parse_reference(&repository, &args.from).await?); - let to_ref = VersionInfo::SnapshotId(parse_reference(&repository, &args.to).await?); - - let diff = repository.diff(&from_ref, &to_ref).await.context(format!( - "Failed to compute diff between {:?} and {:?}", - args.from, args.to - ))?; - - let new_arrays_hash: HashSet<_> = diff.new_arrays.iter().cloned().collect(); - let new_groups_hash: HashSet<_> = diff.new_groups.iter().cloned().collect(); - let deleted_arrays_hash: HashSet<_> = diff.deleted_arrays.iter().cloned().collect(); - let deleted_groups_hash: HashSet<_> = diff.deleted_groups.iter().cloned().collect(); - let updated_arrays_hash: HashSet<_> = diff.updated_arrays.iter().cloned().collect(); - let updated_groups_hash: HashSet<_> = diff.updated_groups.iter().cloned().collect(); - let updated_chunks_hash: HashSet<_> = diff.updated_chunks.keys().cloned().collect(); - - let modified_paths = { - let mut modified_paths: Vec<_> = new_arrays_hash - .union(&new_groups_hash) - .cloned() - .collect::>() - .union(&deleted_arrays_hash) - .cloned() - .collect::>() - .union(&deleted_groups_hash) - .cloned() - .collect::>() - .union(&updated_arrays_hash) - .cloned() - .collect::>() - .union(&updated_groups_hash) - .cloned() - .collect::>() - .union(&updated_chunks_hash) - .cloned() - .collect(); - - modified_paths.sort(); - modified_paths - }; - - for path in modified_paths { - let is_new = new_arrays_hash.contains(&path) || new_groups_hash.contains(&path); - let is_deleted = - deleted_arrays_hash.contains(&path) || deleted_groups_hash.contains(&path); - let is_updated = - updated_arrays_hash.contains(&path) || updated_groups_hash.contains(&path); - let has_updated_chunks = updated_chunks_hash.contains(&path); - - // Sometimes a path will be new or deleted and also have updated chunks. - // In that case we do not print updated chunks as they are all new or all deleted. - if is_new && is_deleted { - writeln!(writer, "-+ {path}")?; - continue; - } else if is_new { - writeln!(writer, "+ {path}")?; - continue; - } else if is_deleted { - writeln!(writer, "- {path}")?; - continue; - } else if is_updated { - writeln!(writer, "~ {path}")?; - } else if has_updated_chunks { - writeln!(writer, " {path}")?; - } - - fn chunk_to_string(c: &ChunkIndices) -> String { - let idxs = c.0.iter().map(|i| i.to_string()).join(", "); - format!("({idxs})") - } - - let updated_chunks = diff.updated_chunks.get(&path); - if let Some(chunks) = updated_chunks { - write!(writer, " Modified {} chunk(s): ", chunks.len())?; - let t = chunks.iter().take(10).map(chunk_to_string).join(", "); - write!(writer, " {t}")?; - if chunks.len() > 10 { - writeln!(writer, ", ...")?; - } else { - writeln!(writer)?; - } - } - } - - Ok(()) -} - async fn config_add(add_cmd: &AddCommand, config: &CliConfig) -> Result { if config.repos.contains_key(&add_cmd.repo) { return Err(anyhow::anyhow!("Repository {:?} already exists", add_cmd.repo)); @@ -836,10 +620,6 @@ pub async fn run_cli(args: IcechunkCLI) -> Result<()> { ancestry(&ancestry_args, &config, stdout()).await?; Ok(()) } - Command::Inspect(inspect_args) => { - inspect(&inspect_args, &config, stdout()).await?; - Ok(()) - } Command::Branch(BranchCommand::List(list_args)) => { list_branches(&list_args, &config, stdout()).await?; Ok(()) @@ -864,10 +644,6 @@ pub async fn run_cli(args: IcechunkCLI) -> Result<()> { delete_tag(&delete_args, &config).await?; Ok(()) } - Command::Diff(diff_args) => { - diff(&diff_args, &config, stdout()).await?; - Ok(()) - } Command::Config(ConfigCommand::Init(init_cmd)) => { let new_config = config_init(&init_cmd, &config).await?; write_config(&new_config)?; @@ -887,11 +663,9 @@ pub async fn run_cli(args: IcechunkCLI) -> Result<()> { mod tests { use std::fs::read_dir; - use bytes::Bytes; use icechunk_macros::tokio_test; use super::*; - use crate::format::Path; use regex::Regex; @@ -993,13 +767,9 @@ Date: \w+ \d{2} \d+ \d{2}:\d{2}:\d{2} assert!(re.is_match(output.as_str())); } - /// Creates a fresh local-filesystem repo for a test, returning its alias, - /// config, root snapshot id, and the backing temp dir. The temp dir must - /// be kept alive (bind it, don't drop it) for as long as the repo is used - /// -- it deletes its directory on drop. - async fn setup_test_repo() - -> (RepositoryAlias, CliConfig, SnapshotId, assert_fs::TempDir) { - let temp = assert_fs::TempDir::new().unwrap(); + async fn setup_test_repo( + temp: &assert_fs::TempDir, + ) -> (RepositoryAlias, CliConfig, SnapshotId) { let path = temp.path().to_path_buf(); let repo_alias = RepositoryAlias("test-repo".to_string()); @@ -1018,12 +788,13 @@ Date: \w+ \d{2} \d+ \d{2}:\d{2}:\d{2} let repository = open_repository(&repo_alias, &config).await.unwrap(); let root = repository.lookup_branch("main").await.unwrap(); - (repo_alias, config, root, temp) + (repo_alias, config, root) } #[tokio_test] async fn test_branch_lifecycle() { - let (repo_alias, config, root, _temp) = setup_test_repo().await; + let temp = assert_fs::TempDir::new().unwrap(); + let (repo_alias, config, root) = setup_test_repo(&temp).await; create_branch( &BranchCreateArgs { @@ -1062,7 +833,8 @@ Date: \w+ \d{2} \d+ \d{2}:\d{2}:\d{2} #[tokio_test] async fn test_tag_lifecycle() { - let (repo_alias, config, root, _temp) = setup_test_repo().await; + let temp = assert_fs::TempDir::new().unwrap(); + let (repo_alias, config, root) = setup_test_repo(&temp).await; create_tag( &TagCreateArgs { @@ -1098,73 +870,33 @@ Date: \w+ \d{2} \d+ \d{2}:\d{2}:\d{2} } #[tokio_test] - async fn test_inspect() { - let (repo_alias, config, root, _temp) = setup_test_repo().await; - + async fn test_parse_reference() { + let temp = assert_fs::TempDir::new().unwrap(); + let (repo_alias, config, root) = setup_test_repo(&temp).await; let repository = open_repository(&repo_alias, &config).await.unwrap(); - let mut session = repository.writable_session("main").await.unwrap(); - session - .add_group(Path::try_from("/data").unwrap(), Bytes::copy_from_slice(b"")) - .await - .unwrap(); - let new_snap = - session.commit("add group").max_concurrent_nodes(8).execute().await.unwrap(); - - let mut writer = Vec::new(); - inspect( - &InspectArgs { repo: repo_alias.clone(), reference: new_snap.to_string() }, - &config, - &mut writer, - ) - .await - .unwrap(); - let output = String::from_utf8(writer).unwrap(); - - assert!(output.contains(&new_snap.to_string())); - assert!(output.contains("-- Nodes --")); - assert!(output.contains("/data")); - assert!(output.contains("-- Manifests --")); - // root snapshot has no nodes yet - let mut writer = Vec::new(); - inspect( - &InspectArgs { repo: repo_alias.clone(), reference: root.to_string() }, + create_tag( + &TagCreateArgs { + repo: repo_alias.clone(), + tag: "v1".to_string(), + target: root.to_string(), + }, &config, - &mut writer, ) .await .unwrap(); - let output = String::from_utf8(writer).unwrap(); - assert!(!output.contains("/data")); - } - #[tokio_test] - async fn test_diff() { - let (repo_alias, config, root, _temp) = setup_test_repo().await; + // resolves a raw snapshot id + assert_eq!(parse_reference(&repository, &root.to_string()).await.unwrap(), root); - let repository = open_repository(&repo_alias, &config).await.unwrap(); - let mut session = repository.writable_session("main").await.unwrap(); - session - .add_group(Path::try_from("/data").unwrap(), Bytes::copy_from_slice(b"")) - .await - .unwrap(); - let new_snap = - session.commit("add group").max_concurrent_nodes(8).execute().await.unwrap(); + // resolves a branch name + assert_eq!(parse_reference(&repository, "main").await.unwrap(), root); - let mut writer = Vec::new(); - diff( - &DiffArgs { - repo: repo_alias.clone(), - from: root.to_string(), - to: new_snap.to_string(), - }, - &config, - &mut writer, - ) - .await - .unwrap(); - let output = String::from_utf8(writer).unwrap(); + // resolves a tag name + assert_eq!(parse_reference(&repository, "v1").await.unwrap(), root); - assert!(output.contains("+ /data")); + // errors clearly on something that is none of the above + let err = parse_reference(&repository, "does-not-exist").await.unwrap_err(); + assert!(err.to_string().contains("does-not-exist")); } } From 4fc559da1fc1542120b074f347566e32a676baee Mon Sep 17 00:00:00 2001 From: DahnJ Date: Sat, 25 Jul 2026 00:50:36 +0200 Subject: [PATCH 8/8] fix: inline format arg to satisfy clippy::uninlined_format_args Co-Authored-By: Claude Sonnet 5 --- icechunk/src/cli/interface.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/icechunk/src/cli/interface.rs b/icechunk/src/cli/interface.rs index 84f7241e7..7e13b8bfe 100644 --- a/icechunk/src/cli/interface.rs +++ b/icechunk/src/cli/interface.rs @@ -754,7 +754,7 @@ mod tests { let mut writer = Vec::new(); ancestry(&args, &config, &mut writer).await.unwrap(); let output = String::from_utf8(writer).unwrap(); - println!("{}", output); + println!("{output}"); let re = Regex::new( r"Snapshot: [0-9A-Z]{20}