Skip to content

Use avro for datafiles collection from write-exec - #54

Merged
mbutrovich merged 6 commits into
apache:mainfrom
hareshkh:use-avro-data-files
Oct 12, 2026
Merged

mbutrovich merged 6 commits into
apache:mainfrom
hareshkh:use-avro-data-files

Conversation

@hareshkh

@hareshkh hareshkh commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Replace JSON with Avro for data-file metadata passed from IcebergWriteExec to IcebergCommitExec. The JSON path could reject valid float partition values or silently commit NaN and ±Infinity as null for float and double partitions.

Each write task groups its files into one Avro container to avoid repeating the schema. Bump iceberg-rust to apache/iceberg-rust@5b82ab99a to support partitions with repeated fixed-width types.

Upstream JSON helper issue: apache/iceberg-rust#3409

@hareshkh
hareshkh force-pushed the use-avro-data-files branch 2 times, most recently from 1768a71 to 77e6b84 Compare October 8, 2026 19:06
@hareshkh
hareshkh force-pushed the use-avro-data-files branch from 77e6b84 to 5576ec3 Compare October 8, 2026 19:11
@hareshkh
hareshkh marked this pull request as ready for review October 8, 2026 19:13
Comment thread crates/datafusion/src/physical_plan/write.rs Outdated
Comment thread crates/datafusion/src/write/exec.rs
@xanderbailey

Copy link
Copy Markdown
Contributor

@mbutrovich interested to know your thoughts here

@mbutrovich
mbutrovich self-requested a review October 9, 2026 16:53

@mbutrovich mbutrovich left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @hareshkh. Passing the data files to the commit in the same Avro encoding that the manifest uses makes sense to me.

The JSON helpers this PR stops using are still public in iceberg-rust, and the bug is still there. On the base commit, the new integration test fails at the insert with Unable to convert raw literal (double) fail convert to type float. That comes from RawLiteral, which rejects narrowing an f64 to an f32 when the difference exceeds f32::EPSILON. That is an absolute bound, and 123.456 exceeds it. With a value that f32 holds exactly, such as 1.5, the insert succeeds, but the manifest records the NaN, Infinity, and -Infinity partition values as null. The same happens for a double partition column. As I read it, that part comes from serde_json writing non-finite floats as null in serialize_data_file_to_json. Could you file an issue on iceberg-rust for these helpers and link it from the description? Could the description also say that the old path committed wrong partition values without an error? Today it reads as if the old path only failed.

Comment thread crates/datafusion/tests/integration_datafusion_test.rs Outdated
Comment thread crates/datafusion/src/write/commit.rs
Comment thread crates/datafusion/src/write/exec.rs
@hareshkh

hareshkh commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review @mbutrovich. iceberg-rust json helpers bug report - apache/iceberg-rust#3409

@hareshkh

Copy link
Copy Markdown
Contributor Author

@mbutrovich / @xanderbailey : The PR should be ready for another pass

@mbutrovich
mbutrovich self-requested a review October 12, 2026 13:35

@mbutrovich mbutrovich left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @hareshkh!

@mbutrovich
mbutrovich merged commit 20327db into apache:main Oct 12, 2026
5 checks passed
@hareshkh
hareshkh deleted the use-avro-data-files branch October 12, 2026 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants