snmvaughan commented on code in PR #7159:
URL: https://github.com/apache/datafusion-comet/pull/7159#discussion_r4360898491
##########
native/core/src/parquet/schema_adapter.rs:
##########
@@ -429,6 +429,23 @@ fn is_string_or_binary(dt: &DataType) -> bool {
)
}
+/// Whether `cast` reconciles a column's Parquet (file) type with its
table-schema type, as opposed
+/// to being part of the pushed-down expression itself.
+///
+/// DataFusion's `DefaultPhysicalExprAdapter` builds the former with
+/// `CastExpr::new_with_target_field`, passing the table schema's field, so
the target is named
+/// after the column (or nested field). Comet's planner builds the latter with
`CastExpr::new`, for
+/// example to coerce a scalar function's arguments to the types the function
requires:
+/// `substring(s, 1, 2)` casts its Int32 literals to Int64. `CastExpr::new`
produces an unnamed
+/// target field.
+///
+/// Only the former are Parquet read conversions that Spark's vectorized
reader validates. Applying
+/// those rules to the latter rejects valid queries, for example as a
disallowed `INT32 -> bigint`
+/// promotion on Spark 3.x when row-level filter pushdown evaluates the
predicate.
+fn is_schema_adaptation_cast(cast:
&datafusion::physical_expr::expressions::CastExpr) -> bool {
+ !cast.target_field().name().is_empty()
Review Comment:
Thanks for catching this, and for checking it against Spark 3.5.9. I
reproduced it: with the name check, an `INT64` column named `""` read as `int`
returned `[1, 2]`. Fixed in a5cbcd5. `replace_with_spark_cast` now checks
`CastExpr::has_explicit_metadata()` instead of the target name, and the new
`empty_named_column_long_read_as_int_errors` covers the empty-named column
under both type-promotion settings. It fails on the previous head and passes
now.
##########
native/core/src/parquet/schema_adapter.rs:
##########
@@ -429,6 +429,23 @@ fn is_string_or_binary(dt: &DataType) -> bool {
)
}
+/// Whether `cast` reconciles a column's Parquet (file) type with its
table-schema type, as opposed
+/// to being part of the pushed-down expression itself.
+///
+/// DataFusion's `DefaultPhysicalExprAdapter` builds the former with
+/// `CastExpr::new_with_target_field`, passing the table schema's field, so
the target is named
+/// after the column (or nested field). Comet's planner builds the latter with
`CastExpr::new`, for
+/// example to coerce a scalar function's arguments to the types the function
requires:
+/// `substring(s, 1, 2)` casts its Int32 literals to Int64. `CastExpr::new`
produces an unnamed
+/// target field.
+///
+/// Only the former are Parquet read conversions that Spark's vectorized
reader validates. Applying
+/// those rules to the latter rejects valid queries, for example as a
disallowed `INT32 -> bigint`
+/// promotion on Spark 3.x when row-level filter pushdown evaluates the
predicate.
+fn is_schema_adaptation_cast(cast:
&datafusion::physical_expr::expressions::CastExpr) -> bool {
+ !cast.target_field().name().is_empty()
+}
Review Comment:
Thanks, that's what a5cbcd5 does. I checked it in DataFusion 55.2.0: the
default adapter builds its schema casts with `new_with_target_field`
(`schema_rewriter.rs`), Comet only calls `CastExpr::new`, and
`with_new_children` keeps the flag. `is_schema_adaptation_cast` is gone.
##########
native/core/src/parquet/schema_adapter.rs:
##########
@@ -2350,6 +2375,220 @@ pub(crate) mod test {
Ok(())
}
+ /// `is_schema_adaptation_cast` tells the casts DataFusion's default
adapter inserts apart from
+ /// the casts Comet's planner puts in a predicate by whether the target
field is named. If a
+ /// DataFusion upgrade changes how `CastExpr::new` names its target, this
fails and the check
+ /// has to be revisited (newer DataFusion versions also expose an
explicit-target marker).
Review Comment:
Right, 55.2.0 already has it. The comment and the naming-convention test are
gone. `schema_casts_are_told_apart_by_explicit_target` pins the flag instead,
including for a `Field::new("", ..)` target built with `new_with_target_field`
and after `with_new_children`, and the scan-level case is
`empty_named_column_long_read_as_int_errors`. I've corrected the PR description
too.
##########
native/core/src/parquet/schema_adapter.rs:
##########
@@ -2350,6 +2375,220 @@ pub(crate) mod test {
Ok(())
}
+ /// `is_schema_adaptation_cast` tells the casts DataFusion's default
adapter inserts apart from
+ /// the casts Comet's planner puts in a predicate by whether the target
field is named. If a
+ /// DataFusion upgrade changes how `CastExpr::new` names its target, this
fails and the check
+ /// has to be revisited (newer DataFusion versions also expose an
explicit-target marker).
+ #[test]
+ fn schema_adaptation_cast_is_told_apart_by_target_field_name() {
+ use super::is_schema_adaptation_cast;
+ use datafusion::physical_expr::expressions::CastExpr;
+
+ // Built by the planner, from a bare type.
+ let planner_cast = CastExpr::new(
+ Arc::new(Literal::new(ScalarValue::Int32(Some(1)))),
+ DataType::Int64,
+ None,
+ );
+ assert!(!is_schema_adaptation_cast(&planner_cast));
+
+ // Built by the default adapter, from the table schema's field.
+ let adapter_cast = CastExpr::new_with_target_field(
+ Arc::new(Column::new("a", 0)),
+ Arc::new(Field::new("a", DataType::Int64, true)),
+ None,
+ );
+ assert!(is_schema_adaptation_cast(&adapter_cast));
+ }
+
+ /// Scans a Parquet file with a pushed-down `substring(s, 1, 2) = 'ab'`
data filter, the way
+ /// `CometNativeScan` sends it, and returns the values the scan produced.
+ ///
+ /// The file schema is identical to the requested schema, so there is no
schema evolution of any
+ /// kind. The predicate comes from the real planner, fed the proto
`CometSubstring` emits (Int32
+ /// literals for the position and length). `SparkSubstring` takes Int64
positions, so the planner
+ /// coerces those literals with `CastExpr::new`.
+ async fn scan_with_substring_filter(
Review Comment:
Good call. `scan_parquet_file` does reproduce it: with the check disabled,
the row-filter case fails with the original `ParquetSchemaConvert { column:
"[]", physical_type: "INT32", spark_type: "bigint" }`. So the custom scan setup
is gone, and the three tests are one table-driven
`pushed_substring_filter_keeps_planner_casts`, with comments on the two
controls.
--
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]
