laskoviymishka commented on code in PR #2791:
URL: https://github.com/apache/iceberg-rust/pull/2791#discussion_r3959287129
##########
crates/iceberg/src/spec/manifest/metadata.rs:
##########
@@ -72,22 +57,39 @@ impl ManifestMetadata {
})
.transpose()?
.unwrap_or(0);
+ let raw_schema = meta.get("schema").ok_or_else(|| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ "schema is required in manifest metadata but not found",
+ )
+ })?;
+ let partition_fields = {
+ let bs = meta.get("partition-spec").ok_or_else(|| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ "partition-spec is required in manifest metadata but not
found",
+ )
+ })?;
+ serde_json::from_slice::<Vec<PartitionField>>(bs).map_err(|err| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ "Fail to parse partition spec in manifest metadata",
+ )
+ .with_source(err)
+ })?
+ };
+ let schema =
Arc::new(parse_manifest_table_schema(raw_schema).or_else(|err| {
Review Comment:
I think the fallback quietly misses its own target case. It only fires when
`parse_manifest_table_schema` returns Err, which here only happens because
DuckDB's payload carries the Avro `array`+`items` shape that `SerdeType` can't
parse.
A DuckDB manifest-entry schema with only scalar/struct fields — `{status:
int, snapshot_id: long, data_file: struct{...}}` — parses cleanly as a valid
Iceberg schema, so the `or_else` never runs and we silently keep the wrong
schema instead of blanking it. That's worse than the fallback. Fingerprinting
the shape before parsing (see the sibling comment) closes this one too — wdyt?
##########
crates/iceberg/src/spec/manifest/metadata.rs:
##########
@@ -72,22 +57,39 @@ impl ManifestMetadata {
})
.transpose()?
.unwrap_or(0);
+ let raw_schema = meta.get("schema").ok_or_else(|| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ "schema is required in manifest metadata but not found",
+ )
+ })?;
+ let partition_fields = {
Review Comment:
minor one, but moving the partition-spec parse ahead of the schema parse
changes which error surfaces. A manifest with both an unparseable `schema` and
a missing `partition-spec` used to report "Fail to parse schema"; now it
reports "partition-spec is required" first and the schema error is never
reached. If anything downstream matches on these strings it'll point at the
wrong root cause — I'd keep the schema parse first, or leave a note that the
reorder is intentional.
##########
crates/iceberg/src/spec/manifest/metadata.rs:
##########
@@ -72,22 +57,39 @@ impl ManifestMetadata {
})
.transpose()?
.unwrap_or(0);
+ let raw_schema = meta.get("schema").ok_or_else(|| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ "schema is required in manifest metadata but not found",
+ )
+ })?;
+ let partition_fields = {
+ let bs = meta.get("partition-spec").ok_or_else(|| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ "partition-spec is required in manifest metadata but not
found",
+ )
+ })?;
+ serde_json::from_slice::<Vec<PartitionField>>(bs).map_err(|err| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ "Fail to parse partition spec in manifest metadata",
+ )
+ .with_source(err)
+ })?
+ };
+ let schema =
Arc::new(parse_manifest_table_schema(raw_schema).or_else(|err| {
+ if partition_fields.is_empty() {
+ // Some writers store the manifest-entry schema under the
+ // `schema` metadata key. For unpartitioned manifests we can
+ // still plan file scans; column bounds that require the table
+ // schema are ignored when their field ids cannot be resolved.
+ Schema::builder().with_schema_id(schema_id).build()
Review Comment:
Even once this is fingerprinted, I'd `tracing::warn!` with the original
`err` before falling back — the module already pulls in `tracing`. Right now
`err` is dropped, so a manifest silently degrades to no column stats with zero
signal in the logs until someone notices pruning stopped working.
##########
crates/iceberg/src/spec/manifest/metadata.rs:
##########
@@ -72,22 +57,39 @@ impl ManifestMetadata {
})
.transpose()?
.unwrap_or(0);
+ let raw_schema = meta.get("schema").ok_or_else(|| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ "schema is required in manifest metadata but not found",
+ )
+ })?;
+ let partition_fields = {
+ let bs = meta.get("partition-spec").ok_or_else(|| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ "partition-spec is required in manifest metadata but not
found",
+ )
+ })?;
+ serde_json::from_slice::<Vec<PartitionField>>(bs).map_err(|err| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ "Fail to parse partition spec in manifest metadata",
+ )
+ .with_source(err)
+ })?
+ };
+ let schema =
Arc::new(parse_manifest_table_schema(raw_schema).or_else(|err| {
+ if partition_fields.is_empty() {
Review Comment:
The guard here is just `partition_fields.is_empty()`, which is true for
every unpartitioned manifest — not just DuckDB's. So a genuinely corrupt or
truncated `schema`, or one from some other buggy writer, now silently resolves
to an empty schema instead of a `DataInvalid`, and every column bound on that
manifest gets dropped downstream.
I'd gate the fallback on a positive fingerprint of the manifest-entry shape
— parse the raw bytes as a `Value`, confirm the root struct has
`status`/`snapshot_id`/`data_file`, and only then substitute; otherwise
re-raise `err`. That keeps corruption loud and scopes the tolerance to the
actual bug.
##########
crates/iceberg/src/spec/manifest/mod.rs:
##########
@@ -169,6 +169,62 @@ mod tests {
use crate::io::FileIO;
use crate::spec::{Literal, NestedField, PrimitiveType, Struct, Transform,
Type};
+ #[test]
+ fn
test_manifest_metadata_with_manifest_entry_schema_for_unpartitioned_table() {
+ let mut meta = HashMap::new();
+ meta.insert("schema-id".to_string(), b"0".to_vec());
+ meta.insert("partition-spec".to_string(), b"[]".to_vec());
+ meta.insert("partition-spec-id".to_string(), b"0".to_vec());
+ meta.insert("format-version".to_string(), b"2".to_vec());
+ meta.insert("content".to_string(), b"data".to_vec());
+ meta.insert(
+ "schema".to_string(),
+ br#"{
+ "type": "struct",
+ "schema-id": 0,
+ "fields": [
+ {"id": 0, "name": "status", "required": true, "type": "int"},
+ {"id": 1, "name": "snapshot_id", "required": false, "type":
"long"},
+ {
+ "id": 2,
+ "name": "data_file",
+ "required": true,
+ "type": {
+ "type": "struct",
+ "fields": [
+ {"id": 100, "name": "file_path", "required": true,
"type": "string"},
+ {
+ "id": 125,
+ "name": "lower_bounds",
+ "required": false,
+ "type": {
+ "type": "array",
+ "items": {
+ "type": "record",
+ "name": "k126_k127",
+ "fields": [
+ {"name": "key", "type": "int", "id": 126},
+ {"name": "value", "type": "binary", "id": 127}
+ ]
+ }
+ }
+ }
+ ]
+ }
+ }
+ ]
+ }"#
+ .to_vec(),
+ );
+
+ let metadata = ManifestMetadata::parse(&meta).unwrap();
+ assert_eq!(metadata.schema_id(), 0);
+ assert!(metadata.schema().as_struct().fields().is_empty());
Review Comment:
This assertion passes whether the fallback fired or the schema simply parsed
into an empty struct — it doesn't actually prove we took the fallback path. I'd
assert against the helper directly (that this payload returns `Err` from
`parse_manifest_table_schema`), or add a contrasting case, so the test pins the
behavior it's meant to.
##########
crates/iceberg/src/spec/manifest/mod.rs:
##########
@@ -169,6 +169,62 @@ mod tests {
use crate::io::FileIO;
use crate::spec::{Literal, NestedField, PrimitiveType, Struct, Transform,
Type};
+ #[test]
+ fn
test_manifest_metadata_with_manifest_entry_schema_for_unpartitioned_table() {
Review Comment:
Two gaps I'd want covered before this lands. There's no negative test — a
partitioned manifest carrying this same payload should hit the `else` branch
and return `.is_err()`; as it stands the `partition_fields.is_empty()` guard
could be inverted or deleted and nothing here would fail.
And there's no regression test that a normal, valid table schema still
parses into non-empty fields — if the fallback ever fired unconditionally, no
test would catch it.
--
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]
