andygrove opened a new pull request, #6602: URL: https://github.com/apache/datafusion-comet/pull/6602
## Which issue does this PR close? Closes #6593. ## Rationale for this change `spark.comet.sparkToColumnar.enabled` and `spark.comet.sparkToColumnar.supportedOperatorList` decide which Spark leaf operators Comet converts to Arrow. The list holds Spark physical class names, and those change between Spark versions. Spark 4.1 plans a query without a `FROM` clause as a `OneRowRelationExec`. Before 4.1 it is an `RDDScanExec`, so the default `OneRowRelation` entry matches nothing there and the `RDDScan` entry converts it instead. A single switch also covers several cases with different trade-offs, so no case can have its own default or its own documentation. #6593 has the details. #832 gave Parquet, JSON and CSV their own `spark.comet.convert` configs for the same reason, and this PR does the same for the four operators the list names by default. ## What changes are included in this PR? It adds four configs, all off by default: | List entry | Config | | ------------------- | -------------------------------------------- | | `Range` | `spark.comet.convert.range.enabled` | | `InMemoryTableScan` | `spark.comet.convert.inMemoryCache.enabled` | | `RDDScan` | `spark.comet.convert.rdd.enabled` | | `OneRowRelation` | `spark.comet.convert.oneRowRelation.enabled` | `CometExecRule` checks each operator's own config first. A new shim, `ShimCometOneRowRelation`, recognizes a `OneRowRelation` on every Spark version. So before 4.1, the one-row config converts it and the RDD config does not. No default changes, so nothing is converted unless a config is set, as before. The list now defaults to empty. It remains the way to convert other leaf operators, such as the `BatchScan` of a Data Source V2 connector. The old settings keep working: - When the list is not set, `spark.comet.sparkToColumnar.enabled=true` still converts the four operators above. - A list that names one of them still converts it. Either way, Comet logs a warning once per JVM that names the config to use instead. The config docs, and the migration guide in a new 1.2.0 section, mark that use as deprecated. `Range` gets its own config rather than becoming the fallback inside `spark.comet.exec.range.enabled`, which #6593 lists as a possible follow-up. Today the switch converts a range even with `spark.comet.exec.range.enabled` off, and every Comet suite relies on that through `CometTestBase`. Folding it in would change which operator those tests run. Test changes: - `CometTestBase` now sets the four configs instead of the switch, which gives the same conversions on every Spark version. - Suites that turned the switch off, or narrowed the list to one operator, now set the matching configs. - `CometInMemoryCacheSuite` builds its own conf. Where it used to set the switch, a small helper now turns on the four configs. - `CometTaskMetricsSuite` still uses the list for `LocalTableScan`, which has no config of its own. ## How are these changes tested? There are three new tests in `CometExecRuleSuite`: - Each config converts its own operator and none of the others. This includes a query without a `FROM` clause, which Spark plans as an `RDDScanExec` before 4.1. - The old settings still convert what they converted before. The switch without a list converts all four operators, a list replaces them, and an operator without a config of its own is converted only when the list names it. - The deprecation warning appears once per config. It doesn't appear when the operator's own config is on, or when the list names an operator that has no config of its own. I checked that each test fails when its part of the change is reverted: - Without the one-row case, the RDD config converts `SELECT 1` on Spark 3.5. - Without the default for an unset list, the switch alone converts nothing. - Without the once-per-config guard, the warning appears twice. Local runs: - Spark 4.1: `CometExecRuleSuite`, `CometExecSuite`, `CometInMemoryCacheSuite`, `CometInMemoryCachePruningSuite`, `CometInMemoryCacheKryoSuite`, `CometArrayExpressionSuite`, `CometMapExpressionSuite`, `CometRangeExecSuite`, `CometEmptyRelationExecSuite`, `CometEmptyRelationParquetWriterSuite` and `CometTaskMetricsSuite` pass. - Spark 3.5: `CometExecRuleSuite`, `CometRangeExecSuite` and `CometInMemoryCachePruningSuite` pass. - Spark 4.0: `CometExecRuleSuite` passes. - Semantic scalafix, scalastyle and spotless on 3.5, the strict-warnings compile on 3.5, the Spark 4.0 test compile and prettier are all clean. I did not run the 3.4 and 4.2 profiles, which use the same shims as 3.5 and 4.1, or the plan stability suites. -- 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]
