andygrove commented on code in PR #5732:
URL: https://github.com/apache/datafusion-comet/pull/5732#discussion_r3996920031
##########
spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala:
##########
@@ -845,30 +844,29 @@ case class CometScanRule(session: SparkSession)
} else {
val readSchema = scanExec.scan.readSchema()
- // Identify complex type columns that would trigger accessor
creation failures
- val complexColumns = readSchema
- .filter(field => isComplexType(field.dataType))
+ // A struct inside a list/map does not make the container null
check a struct check.
+ val structColumns = readSchema
+ .filter(field => field.dataType.isInstanceOf[StructType])
.map(_.name)
.toSet
- // Detect IS NULL/NOT NULL on complex columns (pattern:
is_null(ref(name="col")))
+ // Detect IS NULL/NOT NULL on struct columns (pattern:
is_null(ref(name="col")))
// Nested field filters use different patterns and don't trigger
this issue
- val hasComplexNullCheck = filters.asScala.exists { expr =>
+ val hasStructNullCheck = filters.asScala.exists { expr =>
val exprStr = expr.toString
val isNullCheck = exprStr.contains("is_null") ||
exprStr.contains("not_null")
if (isNullCheck) {
- complexColumns.exists { colName =>
+ structColumns.exists { colName =>
exprStr.contains(s"""ref(name="$colName")""")
}
} else {
false
}
}
- if (hasComplexNullCheck) {
- fallbackReasons += "IS NULL / IS NOT NULL predicates on
complex type columns " +
- "(struct/array/map) are not yet supported by iceberg-rust " +
- "(nested field filters like address.city = 'NYC' are
supported)"
+ if (hasStructNullCheck) {
Review Comment:
Under the mechanism this PR now documents, a struct column looks like it
should be safe too. iceberg-rust builds no accessor for the struct field id
itself, so `is_null(ref("address"))` fails to bind and gets skipped the same
way the list case does, with the post-scan filter enforcing it.
Is there a concrete failure you observed with struct null checks on the
current pin? If so, could the reason string say what it is, since it appears in
EXPLAIN output? If not, it might be simpler to drop the struct guard here as
well and flip `struct_filter_test` to `checkIcebergNativeScan`. Either way, if
the guard stays, could you file a tracking issue and reference it in the reason?
##########
native/spark-expr/src/array_funcs/get_array_struct_fields.rs:
##########
@@ -135,14 +142,14 @@ impl PhysicalExpr for GetArrayStructFields {
fn get_array_struct_fields<O: OffsetSizeTrait>(
list_array: &GenericListArray<O>,
ordinal: usize,
+ field: FieldRef,
Review Comment:
Since this PR changes exactly the field handed to `GenericListArray::new`
below, could that call use `GenericListArray::try_new` with `?` instead? Any
future field/value mismatch would then surface as a DataFusion error rather
than a panic across the JNI boundary.
--
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]
