ziting-openai commented on code in PR #5262:
URL: https://github.com/apache/datafusion-comet/pull/5262#discussion_r3929822788
##########
native/Cargo.toml:
##########
@@ -62,8 +62,8 @@ object_store = { version = "0.13.2", features = ["gcp",
"azure", "aws", "http"]
url = "2.2"
aws-config = "1.8.18"
aws-credential-types = "1.2.13"
-iceberg = { git = "https://github.com/apache/iceberg-rust";, rev =
"8adaa872f31549dd5ad8255848715758228038bc" }
-iceberg-storage-opendal = { git = "https://github.com/apache/iceberg-rust";,
rev = "8adaa872f31549dd5ad8255848715758228038bc", features = ["opendal-memory",
"opendal-fs", "opendal-s3", "opendal-gcs", "opendal-oss", "opendal-azdls"] }
+iceberg = { git = "https://github.com/apache/iceberg-rust";, rev =
"665c64e48e8d33797ecb1a421f327edd9b024879" }
Review Comment:
[P2] Adapt Comet to the newly pinned Iceberg task API
At this revision, `FileScanTask` fields are private and
`FileScanTaskDeleteFile` requires `file_format`, but Comet still reads/mutates
`task.deletes` in `iceberg_scan.rs:270,323`, constructs `FileScanTask` with a
struct literal in `planner.rs`, and omits `file_format` from the delete-task
initializer. Iceberg is a nonoptional dependency and these modules are
unconditional, so this blocks native compilation even when no Iceberg query is
used. The [current-head Rust CI
job](https://github.com/apache/datafusion-comet/actions/runs/33820438498/job/100862299933)
reports E0616/E0063 for these call sites. Please migrate task
construction/access to the pinned builder/getter APIs and supply the
delete-file format, including the affected tests.
##########
native/core/src/parquet/schema_adapter.rs:
##########
@@ -77,6 +78,91 @@ fn schema_has_field_ids(schema: &SchemaRef) -> bool {
schema.fields().iter().any(|f| parse_field_id(f).is_some())
}
+/// Returns true when casting `physical_type` to `target_type` is a *pure*
structural
+/// narrowing (dropping unrequested struct/list fields, no leaf-level value
reinterpretation)
+/// that DataFusion's own `datafusion_common::nested_struct::cast_column`
already computes
+/// identically to Comet's `spark_parquet_convert`. `ColumnarValue::cast_to`
(the function a
+/// plain, un-swapped `CastExpr` runs at execution time; see
+/// datafusion/expr-common/src/columnar_value.rs) routes to that same function
whenever
+/// `datafusion_common::nested_struct::requires_nested_struct_cast` holds,
matching arbitrary
+/// struct/list fields by name, null-filling missing target fields, and
dropping extra source
+/// fields, exactly the shape apache/datafusion-comet#4859 needs pruned.
Confirmed
+/// byte-identical to `spark_parquet_convert` for the covered shapes by
+/// `test::nested_struct_narrowing_cast_matches_datafusion_generic_cast`.
+///
+/// When this returns `true`, `replace_with_spark_cast` leaves DataFusion's
`CastExpr` in
+/// place instead of swapping in `CometCastColumnExpr`, so DataFusion's
leaf-pruning
+/// (`build_projection_read_plan`'s cast-clipping, apache/datafusion#24090)
can see the cast
+/// and read only the requested Parquet leaves, instead of falling back to a
full-column read
+/// because it can't recognize `CometCastColumnExpr`.
+///
+/// This is deliberately an allow list, not a deny list: it only recurses
through the two
+/// container shapes `nested_struct::cast_column` actually implements (Struct,
List /
+/// LargeList), and requires every leaf it bottoms out at to be an *exact*
type match. Pruning
+/// (the only case this predicate needs to cover, see
apache/datafusion-comet#4859) only
+/// changes which struct/list fields are kept, never a leaf's type, so
exact-match leaves are
+/// sufficient. A deny list here (enumerate every case where Comet's nested
cast differs from
+/// Arrow's, allow everything else) would fail open: a future addition to
+/// `parquet_convert_array` that this predicate does not know to also exclude
would silently
+/// start producing wrong results instead of just missing an optimization.
+fn is_pure_structural_narrowing(
+ physical_type: &DataType,
+ target_type: &DataType,
+ parquet_options: &SparkParquetOptions,
+) -> bool {
+ match (physical_type, target_type) {
+ (DataType::Struct(source_fields), DataType::Struct(target_fields)) => {
+ // Comet matches by Parquet field id first when the target carries
one;
+ // DataFusion's generic cast has no field-id concept, so any
field-id-bearing
+ // target field is a potential divergence.
+ if parquet_options.use_field_id
+ && target_fields.iter().any(|f| parse_field_id(f).is_some())
+ {
+ return false;
+ }
+ target_fields.iter().all(|target_field| {
+ // Require an *exact* (case-sensitive) name match for every
target field.
+ // `nested_struct::cast_column` always matches by exact name;
Comet
+ // additionally matches case-insensitively when
`case_sensitive` is false,
+ // which could resolve a field DataFusion would instead treat
as missing (and
+ // null-fill). Requiring an exact match sidesteps that
divergence regardless
+ // of the `case_sensitive` setting, and also sidesteps the
missing-field
+ // nullability divergence: DataFusion errors when a
non-nullable target field
+ // is missing from the source, whereas Comet null-fills
unconditionally.
+ source_fields
+ .iter()
+ .find(|f| f.name() == target_field.name())
+ .is_some_and(|source_field| {
Review Comment:
[P2] Reject ambiguous nested names before retaining the generic cast
With `spark.sql.caseSensitive=false`, a Parquet column
`s:struct<ID:bigint,id:bigint>` read through the explicit schema
`s:struct<id:bigint>` passes this exact-name lookup. I traced the adapter
retaining `CastExpr`, which can prune `ID` and bypass the duplicate-field check
in `spark_parquet_convert`; Spark and the previous Comet path reject this
ambiguous match instead of returning one field. The existing `CAFÉ`/`café`
native-reader regression requests `Café`, so it misses the case where one
sibling matches exactly. Please require uniqueness under the configured
case-insensitive resolver at each nested struct level before allowing this
cast, and add the same native-scan regression requesting `café` (or `id`)
exactly.
##########
native/Cargo.toml:
##########
@@ -35,18 +35,18 @@ license = "Apache-2.0"
edition = "2021"
# Comet uses the same minimum Rust version as DataFusion
-rust-version = "1.88"
+rust-version = "1.94.0"
[workspace.dependencies]
-arrow = { version = "58.4.0", features = ["prettyprint", "ffi", "chrono-tz"] }
-arrow-select = { version = "58.4.0" }
+arrow = { version = "59.2.0", features = ["prettyprint", "ffi", "chrono-tz"] }
+arrow-select = { version = "59.2.0" }
async-trait = { version = "0.1" }
bytes = { version = "1.11.1" }
-parquet = { version = "58.4.0", default-features = false, features =
["experimental"] }
-datafusion = { version = "54.1.0", default-features = false, features =
["unicode_expressions", "crypto_expressions", "nested_expressions", "parquet"] }
-datafusion-datasource = { version = "54.1.0" }
-datafusion-physical-expr-adapter = { version = "54.1.0" }
-datafusion-spark = { version = "54.1.0", features = ["core"] }
+parquet = { version = "59.2.0", default-features = false, features =
["experimental"] }
+datafusion = { version = "55.0.0", default-features = false, features =
["unicode_expressions", "crypto_expressions", "nested_expressions", "parquet"] }
Review Comment:
[P2] Finish migrating the remaining DF55/Arrow consumers
The native library no longer compiles with this version: `ExplodeExec` and
`IcebergWriteExec` still omit the now-required
`ExecutionPlan::apply_expressions`, and `explode.rs:898` still accesses
`UnnestOptions::preserve_nulls` as a field. Both operators are compiled
unconditionally. The [current-head Rust CI
job](https://github.com/apache/datafusion-comet/actions/runs/33820438498/job/100862299933)
confirms E0046/E0615, plus stale `preserve_nulls` test initializers and
`CompressionContext` in the shuffle-scan test. Please port these remaining
consumers and test helpers to the new APIs; updating the other execution plans
is not enough to make this dependency bump build.
--
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]
