andygrove commented on code in PR #6725:
URL: https://github.com/apache/datafusion-comet/pull/6725#discussion_r4277126000
##########
spark/src/main/scala/org/apache/comet/iceberg/IcebergReflection.scala:
##########
@@ -2358,11 +2440,24 @@ case class CometIcebergNativeScanMetadata(
nameMapping: Option[String],
@transient tasks: java.util.List[_],
scanSchema: Any,
+ prunedTaskSchema: Option[Any],
+ fullTaskSchema: Option[Any],
tableSchema: Any,
globalFieldIdMapping: Map[String, Int],
catalogProperties: Map[String, String],
catalogName: Option[String],
- fileFormat: String)
+ fileFormat: String) {
+
+ /**
+ * The schema every task of the scan reads with: the pruned task schema
while nested schema
+ * pruning is on and can apply, the full one otherwise. CometScanRule checks
this schema and
+ * CometIcebergNativeScan serializes it, so the two cannot disagree. None
falls back to Spark.
+ */
+ def taskSchema: Option[Any] =
Review Comment:
`taskSchema` reads
`spark.comet.scan.icebergNative.nestedSchemaPruning.enabled` every time it's
called. `CometScanRule` calls it while planning, but `serializePartitions`
calls it when the scan executes, so the two can still disagree if the config
changes in between. When planning sees `true` and execution sees `false`, the
scan reads the full schema, which the nested-evolution check never walked.
I hit this with a table shaped like the unpartitioned one in the struct
fallback test. Planning `SELECT id, c, s.a FROM t VERSION AS OF <snapshot>`
with the config on and collecting it with the config off fails with `Incorrect
number of arrays for StructArray fields, expected 2 got 1`. A cached
`DataFrame` gets there too, since `cache()` plans the query and only the first
action runs it.
The metadata is only built in `extract`, so what do you think about making
this a `val`? Then the choice is fixed during planning, and the check and the
read can't disagree, as the comment says. Could the struct fallback test also
plan that query with the config on and collect it with the config off?
##########
spark/src/test/scala/org/apache/comet/CometIcebergNativeSuite.scala:
##########
@@ -1801,22 +1827,61 @@ class CometIcebergNativeSuite
checkNestedFieldEvolutionFallback(
s"SELECT id FROM $table WHERE s IS NULL ORDER BY id",
"s.b (added)")
- // The native read asks for the column's full nested type even when
Spark prunes it.
+ // The native read asks only for s.a here, but the check also covers
the column's full
+ // type in the table schema, so it still falls back.
checkNestedFieldEvolutionFallback(
s"SELECT id, s.a FROM $table ORDER BY id",
"s.b (added)")
checkNestedFieldEvolutionFallback(
s"SELECT l.id, r.id FROM $table l JOIN $table r ON l.s = r.s ORDER
BY l.id",
"s.b (added)")
- // The old snapshot's files lack s.b too, and the native read uses the
current schema.
+ // The old snapshot's files lack s.b too, and the scan's full schema
is the current one,
+ // which has it.
checkNestedFieldEvolutionFallback(
s"SELECT id, s FROM $table VERSION AS OF $snapshotBeforeAdd ORDER BY
id",
"s.b (added)")
// A read that does not project the evolved column stays native.
checkIcebergNativeScan(s"SELECT id FROM $table ORDER BY id")
- spark.sql(s"DROP TABLE $table")
+ // A VERSION AS OF read whose full schema is older than the table's:
the pruned-away
Review Comment:
I ran this case on `main`. It reads natively there and matches Spark,
because `s.k` is appended at the top level of the scan schema. Here it falls
back. I think that's a fair trade for the name clash the guard fixes, and you
mentioned keeping it native would need `s.k` merged back into its pruned
struct. Could you file an issue for that follow-up and link it here, so it
doesn't get lost?
--
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]
