laskoviymishka commented on code in PR #2519:
URL: https://github.com/apache/iceberg-rust/pull/2519#discussion_r3436988342
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -281,6 +281,25 @@ impl<'a> TableScanBuilder<'a> {
None
};
+ let name_mapping = self
+ .table
+ .metadata()
+ .properties()
+ .get(DEFAULT_SCHEMA_NAME_MAPPING)
+ .map(|raw| {
+ serde_json::from_str::<NameMapping>(raw).map_err(|e| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ format!(
+ "Failed to parse table property
{DEFAULT_SCHEMA_NAME_MAPPING} as a NameMapping"
Review Comment:
The message names the key but not the value that failed, and
`serde_json::Error` gives line/col but not content — debugging this in
production means guessing at the offending payload. I'd fold the raw value in:
`format!("Failed to parse table property {DEFAULT_SCHEMA_NAME_MAPPING}:
{raw:?}")`, or `.with_context("raw_value", raw.to_string())`. (Once a runtime
value is interpolated, `format!` is justified — as written it only wraps a
`&'static str`, where a bare literal would do.)
##########
crates/iceberg/src/scan/context.rs:
##########
@@ -133,8 +137,7 @@ impl ManifestEntryContext {
.with_partition(Some(self.manifest_entry.data_file.partition.clone()))
// TODO: Pass actual PartitionSpec through context chain for
native flow
.with_partition_spec(None)
- // TODO: Extract name_mapping from table metadata property
"schema.name-mapping.default"
- .with_name_mapping(None)
+ .with_name_mapping(self.name_mapping)
Review Comment:
This line is correct — but it's also the switch that makes a downstream bug
live, so flagging it here.
Once a `name_mapping` reaches the reader,
`apply_name_mapping_to_arrow_schema` assigns Iceberg field IDs into the Arrow
schema. But `pipeline.rs:225` still calls `get_arrow_projection_mask` with
`use_fallback = missing_field_ids`, which is `true` for exactly the no-field-ID
files name mapping exists to serve. So we fall into the positional fallback
(`field_id N -> column position N-1`) and the IDs we just assigned are ignored
— for a table whose files lack embedded IDs but define a mapping, projection
picks columns by position, not by name. Java's `ReadConf` runs
`applyNameMapping` then `pruneColumns` (field-ID-based), and only uses the
positional fallback when the mapping is null.
The fix lives in `pipeline.rs`, not here: something like `let use_fallback =
missing_field_ids && task.name_mapping.is_none();`. I'd either do that in this
PR, or land this with the limitation documented and a follow-up issue, so we're
not silently flipping on a wrong-data path. wdyt?
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -1320,6 +1340,112 @@ pub mod tests {
);
}
+ fn table_with_property(key: &str, value: &str) -> Table {
+ let fixture = TableTestFixture::new();
+ let mut metadata = fixture.table.metadata().clone();
+ metadata
+ .properties
+ .insert(key.to_string(), value.to_string());
+ Table::builder()
+ .metadata(metadata)
+ .identifier(fixture.table.identifier().clone())
+ .file_io(fixture.table.file_io().clone())
+
.metadata_location(fixture.table.metadata_location().unwrap().to_string())
+ .runtime(test_runtime())
+ .build()
+ .unwrap()
+ }
+
+ #[test]
+ fn test_table_scan_without_name_mapping_property() {
+ let table = TableTestFixture::new().table;
+
+ let table_scan = table.scan().build().unwrap();
+ assert!(
+ table_scan
+ .plan_context
+ .as_ref()
+ .unwrap()
+ .name_mapping
+ .is_none()
+ );
+ }
+
+ #[test]
+ fn test_table_scan_with_name_mapping_property() {
+ let mapping_json = r#"[{"field-id":1,"names":["id","record_id"]}]"#;
+ let table = table_with_property(DEFAULT_SCHEMA_NAME_MAPPING,
mapping_json);
+
+ let table_scan = table.scan().build().unwrap();
+ let mapping = table_scan
+ .plan_context
+ .as_ref()
+ .unwrap()
+ .name_mapping
+ .as_ref()
+ .expect("name_mapping should be parsed from the table property");
+ let fields = mapping.fields();
+ assert_eq!(fields.len(), 1);
+ assert_eq!(fields[0].field_id(), Some(1));
+ assert_eq!(fields[0].names(), &[
+ "id".to_string(),
+ "record_id".to_string()
+ ]);
+ }
+
+ #[test]
+ fn test_table_scan_with_malformed_name_mapping_property() {
+ let table = table_with_property(DEFAULT_SCHEMA_NAME_MAPPING, "{ not
valid json");
+
+ let err = table
+ .scan()
+ .build()
+ .expect_err("malformed name mapping should fail to parse");
+ assert_eq!(err.kind(), ErrorKind::DataInvalid);
+ }
+
+ #[tokio::test]
+ async fn test_plan_files_carries_name_mapping_into_file_scan_task() {
+ let mut fixture = TableTestFixture::new();
+ fixture.setup_manifest_files().await;
+
+ let mapping_json = r#"[{"field-id":1,"names":["id","record_id"]}]"#;
+ let mut metadata = fixture.table.metadata().clone();
+ metadata.properties.insert(
+ DEFAULT_SCHEMA_NAME_MAPPING.to_string(),
+ mapping_json.to_string(),
+ );
+ let table = Table::builder()
+ .metadata(metadata)
+ .identifier(fixture.table.identifier().clone())
+ .file_io(fixture.table.file_io().clone())
+
.metadata_location(fixture.table.metadata_location().unwrap().to_string())
+ .runtime(test_runtime())
+ .build()
+ .unwrap();
+
+ let tasks: Vec<_> = table
+ .scan()
+ .build()
+ .unwrap()
+ .plan_files()
Review Comment:
This test proves the mapping survives planning, but it stops at
`plan_files()` and never calls `to_arrow()` — so the actual unlocked behavior
(the reader assigning field IDs from the mapping for a no-field-ID file) is
never exercised. The fixture's Parquet files have embedded field IDs, so even
if we did call `to_arrow()` the mapping branch wouldn't fire.
For real coverage I'd write a Parquet file *without* field-ID metadata
(template off `test_open_parquet_no_deletions`, but `WriterProperties` with no
field IDs), set the property, call `to_arrow()`, and assert the data comes back
correct. That's also the test that would catch the `pipeline.rs` projection
issue flagged on `with_name_mapping`.
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -281,6 +281,25 @@ impl<'a> TableScanBuilder<'a> {
None
};
+ let name_mapping = self
+ .table
+ .metadata()
+ .properties()
+ .get(DEFAULT_SCHEMA_NAME_MAPPING)
+ .map(|raw| {
+ serde_json::from_str::<NameMapping>(raw).map_err(|e| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ format!(
+ "Failed to parse table property
{DEFAULT_SCHEMA_NAME_MAPPING} as a NameMapping"
+ ),
+ )
+ .with_source(e)
+ })
+ })
+ .transpose()?
Review Comment:
The `.transpose()?` makes a malformed property a hard error out of
`build()`, so a corrupted (or empty) `schema.name-mapping.default` breaks every
scan on the table — even scans that would never touch the mapping. Before this
PR those tables scanned fine, so this is a backward-compat regression, and it
diverges from Java/PyIceberg, which parse lazily and treat a bad/absent value
as no mapping rather than failing scan setup. Worth noting empty-string is a
real case — some catalogs (Glue) store `""` for "set but cleared", and that
would now hit `serde_json` and fail.
I'd log a warning and fall back to `name_mapping = None` on parse error (and
short-circuit empty string before parsing). If we deliberately want the hard
fail, that's a defensible choice too, but it should be the explicit decision
and the empty-string case handled either way.
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -1320,6 +1340,112 @@ pub mod tests {
);
}
+ fn table_with_property(key: &str, value: &str) -> Table {
Review Comment:
The async test below re-inlines this exact `Table::builder()` block instead
of reusing the helper, because the helper takes a pre-built table and
`setup_manifest_files()` has to run on the fixture first. I'd extend the helper
to accept a `TableTestFixture` (or add `TableTestFixture::with_property(key,
value)`) so the async test can use it too — right now the 10-line pattern
exists twice.
--
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]
