NoahKusaba opened a new pull request, #20:
URL: https://github.com/apache/datafusion-iceberg/pull/20
## Which issue does this PR close?
- No issue tracks this.
- Part of the Ballista-Iceberg integration (follows #14). This is the change
I outlined on apache/iceberg-rust#2862, where the question was whether exposing
it is intended; it is limited to what a plan codec needs.
## What changes are included in this PR?
A distributed engine sends physical plans to other processes through a
datafusion-proto `PhysicalExtensionCodec`. To serialize a node, the codec has
to name its type, read what it was built from, and build an equivalent node on
the other side. For the Iceberg nodes, none of that is possible from outside
this crate today: `IcebergCommitExec` and `IcebergWriteExec` are `pub(crate)`,
the modules of all four nodes are private, and none of them expose what they
hold. This makes them public, together with read-only accessors for their
parts, and changes no behavior of existing plans.
**Newly exported from `datafusion_iceberg::physical_plan`:**
`IcebergCommitExec`, `IcebergWriteExec` and `IcebergMetadataScan`, each with
documented constructors and an accessor for what it holds:
- `IcebergCommitExec::{new, table, catalog}`
- `IcebergWriteExec::{new, table}`
- `IcebergMetadataScan::{new, provider}`
**Providers:**
- `IcebergMetadataTableProvider::{new, table, metadata_type}`, and a
re-export at the crate root. Its fields were `pub(crate)`; they are now private
behind the constructor.
- `IcebergTableProvider::{catalog, table_ident}`.
- `IcebergStaticTableProvider::{table, snapshot_id}`.
`IcebergTableProvider::try_new` is left out, since #4 already proposes it.
**`IcebergTableScan::new_with_predicate`.** A codec can read a scan's
pushed-down `Predicate`, but cannot turn it back into the DataFusion filters
the existing constructor takes, so this constructor takes the predicate
directly. The providers report pushdown as `Inexact`, so DataFusion still
applies the filters above the scan, and the predicate only lets Iceberg skip
data files. It returns an error for a projection index outside the schema,
where the constructor used to panic (`schema.project(..).unwrap()`); the
crate-private `new` now delegates to it and returns `Result` too.
**`IcebergCommitExec` refuses an input with more than one partition.**
`execute` reads only partition 0 of its input and relies on
`required_input_distribution` for a coalesce the optimizer inserts. Built
directly, as a codec does, over a multi-partition input, it would commit only
the first partition's files and report success. With two input partitions of
one file each, it committed 1 file and reported 100 rows instead of 200. It now
returns an error before committing anything. Plans built by `insert_into`
always coalesce, so they are unaffected.
The constructor docs state what each node expects of its input (one
partition for the commit; columns matched by name, and the partition values
from `project_with_partition` for partitioned tables, for the write).
## Are these changes tested?
- `test_plan_nodes_are_inspectable` (integration):
- Reaches a catalog-backed provider through `IcebergCatalogProvider` and
checks its identifier and catalog.
- Plans an INSERT and checks the commit and write hold the table, with the
commit using the provider's catalog.
- Rebuilds a pinned scan with `new_with_predicate` from the original's
parts, and checks it returns the same rows, with the filter applied.
- Rebuilds a `$snapshots` scan from its provider's parts, and checks it
returns the same rows.
- `test_iceberg_commit_exec_rejects_multiple_input_partitions`: two input
partitions produce an error, and the table gets no snapshot.
- `test_scan_rejects_out_of_range_projection`: an out-of-range index is an
error, not a panic.
- A doctest on `new_with_predicate` shows the rebuild round trip.
`cargo fmt --all -- --check`, `cargo clippy --workspace --locked
--all-targets -- -D warnings`, `cargo test --workspace --locked` and
`RUSTDOCFLAGS="-D warnings" cargo doc --no-deps -p datafusion-iceberg` all pass
locally.
## AI Disclosure
- Used Claude Code to write the tests, find the multi-partition commit
issue, and draft this description. I reviewed the change.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
