andygrove commented on code in PR #6754:
URL: https://github.com/apache/datafusion-comet/pull/6754#discussion_r4222176407
##########
spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala:
##########
@@ -394,6 +394,16 @@ case class CometScanRule(session: SparkSession)
return withFallbackReason(scanExec, "Iceberg Metadata tables are not
supported")
}
+ // As in transformV1Scan: these expressions read InputFileBlockHolder,
which the source's own
+ // reader sets per file. Comet's native V2 scans (Iceberg, CSV) do not, so
they would return
+ // empty/default values
(https://github.com/apache/datafusion-comet/issues/6707).
+ if (CometScanRule.readsInputFileBlock(plan)) {
Review Comment:
This also changes the native CSV V2 scan, since it goes through the same
check. I tried it locally. With `spark.comet.scan.csv.v2.enabled=true` and an
empty `spark.sql.sources.useV1SourceList`, a CSV read that selects
`input_file_name()` returns different results from Spark without this check.
With it, the scan falls back and matches. Could we add a case like that to
`CometCsvNativeReadSuite`, with a few small CSV files and a check for the new
fallback reason? The CSV scan is off by default, so if a later change moved
this check into the Iceberg branch, nothing would catch it. Could we also add a
sentence to the CSV section of `datasources.md` saying the native CSV scan
falls back for these functions, the way `iceberg.md` now does?
--
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]
