andygrove commented on code in PR #7123:
URL: https://github.com/apache/datafusion-comet/pull/7123#discussion_r4357208464
##########
spark/src/main/scala/org/apache/comet/CometConf.scala:
##########
@@ -1269,16 +1281,34 @@ object CometConf extends ShimCometConf {
conf("spark.comet.convert.broadcastBuildSide.enabled")
.category(CATEGORY_EXEC)
.doc("When enabled, Comet converts a file or V2 scan that it cannot scan
natively (for " +
- "example a Text source) to Arrow when that scan feeds a broadcast
join's build side and " +
+ "example an ORC source) to Arrow when that scan feeds a broadcast
join's build side and " +
"every scan on the probe side is already native, so the broadcast join
runs natively " +
"over the large probe. The build side is usually small
(auto-broadcasts are capped by " +
"the broadcast threshold), so the row to Arrow copy is usually cheap -
though an " +
"explicit BROADCAST hint or a selective filter above the scan can make
it larger. With " +
"dynamic partition pruning the dimension is also read a second time
for the pruning " +
- "subquery. Set this to false to keep such build sides on Spark.")
+ "subquery. Parquet, JSON and CSV scans are converted only by their own
" +
+ "`spark.comet.convert` configs. A Text scan is converted only when " +
+ "`spark.comet.convert.broadcastBuildSide.text.enabled` is also
enabled, because Text " +
+ "files can hold bytes that are not valid UTF-8. Set this to false to
keep such build " +
+ "sides on Spark.")
.booleanConf
.createWithDefault(true)
+ val COMET_CONVERT_BROADCAST_BUILD_SIDE_TEXT_ENABLED: ConfigEntry[Boolean] =
Review Comment:
No, I don't see a workload that needs it. Converting the build side while
every other Text scan stays on Spark is only useful if the build side is the
one place a Latin-1 file is safe, and nothing makes that so. I dropped
`spark.comet.convert.broadcastBuildSide.text.enabled`. A Text scan, including
one on a build side, is now converted only by
`spark.comet.convert.text.enabled`, like CSV, JSON and Parquet. The fallback
reason now names that config. The datasources paragraph says "Parquet, JSON,
CSV and Text build sides are converted only by their own switches". The Latin-1
explanation now lives in the `spark.comet.convert.text.enabled` doc and the
compatibility guide. The #6008 allowlist case sets
`spark.comet.convert.text.enabled=true`. @parthchandra, please say if that
doesn't work for you.
This also changed the #6008 tests. With a global Text switch they can't use
Text any more, because a Text probe would be converted too and the "leaves no
bridge" assertions would stop meaning anything. They now use ORC, which is
still an unsupported format that gets bridged. The #7118 test keeps the Latin-1
Text file and covers the default and `spark.comet.convert.text.enabled`, for V1
and V2 with AQE on and off.
##########
spark/src/main/scala/org/apache/comet/CometConf.scala:
##########
@@ -227,6 +227,18 @@ object CometConf extends ShimCometConf {
.booleanConf
.createWithDefault(false)
+ val COMET_CONVERT_FROM_TEXT_ENABLED: ConfigEntry[Boolean] =
Review Comment:
Good catch on both. Text now goes through `isConversionEnabled`, like Range,
InMemoryTableScan, RDDScan and OneRowRelation. Naming `FileSourceScan` or
`BatchScan` for a Text scan still converts it, but logs the same deprecation
warning. The `supportedOperatorList` doc and the "To convert any other leaf
operator" paragraph in `datasources.md` now say "other than Parquet, JSON, CSV
and Text". Both also say that naming those operators to convert Text is
deprecated. Naming `FileSourceScan` for ORC and other formats without a config
of their own is unchanged.
--
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]
