sunchao commented on code in PR #7159:
URL: https://github.com/apache/datafusion-comet/pull/7159#discussion_r4354015278
##########
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:
[P2] Preserve schema validation for empty-named columns. Spark supports a
Parquet column named `""`, so an empty target name does not identify a planner
cast. For a file field `Field("", Int64)` containing `[1, 2]`, requested as
`Field("", Int32)`, the default adapter creates a genuine schema cast with an
empty target name. This predicate now skips `check_conversion`, and the native
scan returns `[1, 2]` instead of Spark's schema-conversion error. This occurs
with row-filter pushdown disabled and with either promotion setting. Could this
use DataFusion 55.2.0's existing explicit-target accessor, such as
`has_explicit_metadata()`, and retain a regression test for empty field names?
Evidence: Ran a disposable native test through the existing `scan_parquet`
helper. With `allow_type_promotion=false` and `true`, column `a` produced
`ParquetSchemaConvert { physical_type: "INT64", spark_type: "int" }`, while
column `""` returned `Int32 [1, 2]`. The native run reported 92 passed and only
this regression test failed. Stock Spark 3.5.9 successfully wrote and read an
empty-named BIGINT column containing 1, then rejected an IntegerType read with
`SchemaColumnConvertNotSupportedException: column: [], physicalType: INT64,
logicalType: int`. The base implementation reaches `check_conversion` without
the new name guard. Reproduction source and logs are retained in
`/tmp/comet-review-7159-a1350a73/`.
--
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]
