laskoviymishka commented on code in PR #3363:
URL: https://github.com/apache/iceberg-rust/pull/3363#discussion_r4234772823
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
@@ -28,12 +28,11 @@ use crate::spec::{
DataFile, DataFileFormat, FormatVersion, MAIN_BRANCH, ManifestContentType,
ManifestEntry,
ManifestFile, ManifestListWriter, ManifestWriter, ManifestWriterBuilder,
Operation, Snapshot,
SnapshotReference, SnapshotRetention, SnapshotSummaryCollector, Struct,
StructType, Summary,
- TableProperties, update_snapshot_summaries,
+ TableProperties, set_iceberg_version, update_snapshot_summaries,
};
use crate::table::Table;
use crate::transaction::ActionCommit;
use crate::{Error, ErrorKind, TableRequirement, TableUpdate};
-
/// A trait that defines how different table operations produce new snapshots.
Review Comment:
Looks like the blank line between the `use` block and this doc comment got
dropped — unrelated churn, I'd restore it.
##########
crates/iceberg/src/spec/snapshot_summary.rs:
##########
@@ -330,6 +331,14 @@ where T: PartialOrd + Default + ToString {
}
}
+pub(crate) fn set_iceberg_version(properties: &mut HashMap<String, String>) {
+ let version = env!("CARGO_PKG_VERSION");
+ properties.insert(
+ ICEBERG_VERSION_PROP.to_string(),
+ format!("Apache Iceberg Rust {version}"),
Review Comment:
Small parity thing: Java and Go seed this as `Apache Iceberg <ver>` (Java's
`IcebergBuild.fullVersion()` → `Apache Iceberg 1.10.0 (commit abc…)`), so a
parser matching `Apache Iceberg (\S+)` across the three clients would pull
`Rust` out as the version here. `Apache Iceberg {version} (rust)` keeps the
version in the same slot. It's informational-only today, so not blocking — just
easier to align now than after it's in the wild.
##########
crates/iceberg/src/spec/snapshot_summary.rs:
##########
@@ -330,6 +331,14 @@ where T: PartialOrd + Default + ToString {
}
}
+pub(crate) fn set_iceberg_version(properties: &mut HashMap<String, String>) {
Review Comment:
This always overwrites a caller-supplied `iceberg-version`, which is the
right precedence — Java and Go both apply environment context after user
snapshot properties, so the library value wins. Worth a one-line `///` here
saying it always overwrites, since silently dropping a caller's value is
otherwise a little surprising.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
