andygrove opened a new pull request, #6662: URL: https://github.com/apache/datafusion-comet/pull/6662
## Which issue does this PR close? No issue. This is a follow-up to #6602, which gave `Range`, `InMemoryTableScan`, `RDDScan` and `OneRowRelation` their own `spark.comet.convert` configs. ## Rationale for this change `RowDataSourceScanExec` is the scan Spark plans for Data Source V1 relations that are not file-based, such as JDBC tables. Today the only way to have Comet convert its output to Arrow, so that the operators above it can run in Comet, is to set `spark.comet.sparkToColumnar.enabled=true` and name `RowDataSourceScan` in `spark.comet.sparkToColumnar.supportedOperatorList`. That list is matched against Spark class names. #6602 moved the operators it named by default to configs of their own, and this does the same for this scan. ## What changes are included in this PR? - A new config, `spark.comet.convert.rowDataSource.enabled`, off by default. `CometExecRule` checks it for `RowDataSourceScanExec` the same way it checks the configs from #6602. - A list that names `RowDataSourceScan` still converts it, as before. That use is now deprecated, and Comet logs the same once-per-JVM warning naming the new config. The list never named `RowDataSourceScan` by default, so `spark.comet.sparkToColumnar.enabled=true` without a list still does not convert it. - The list's config doc, the data sources page and the 1.2.0 section of the upgrade guide include the new config. - A test helper, `CometTestBase.rowDataSourceDataFrame`, builds a DataFrame over a `BaseRelation with TableScan`, which Spark plans as a `RowDataSourceScanExec`. `CometTestBase` does not turn the new config on, so the existing suites convert what they converted before. No default changes. With the new config off, `RowDataSourceScanExec` is converted exactly when it was before, and the Spark SQL test diffs set none of these keys. ## How are these changes tested? - `CometExecRuleSuite`: the existing conversion tests now include the new config. Each `spark.comet.convert` config converts only its own operator, including the new one. The switch without a list does not convert `RowDataSourceScanExec`. A list that names it does, and logs the deprecation warning naming the new config. - `CometExecSuite`: a new test, `SparkToColumnar over RowDataSourceScanExec`, runs a filter and an aggregate over the scan with AQE on and off. It checks the results, that the plan runs in Comet above a `CometSparkToColumnarExec` over the scan, and that nothing is converted with the config off. I checked that the tests fail when the change is reverted. Without the new `CometExecRule` case, the rule test and the new end-to-end test fail. Checking the config without going through the deprecation helper makes the warning test fail. Local runs: - `CometExecRuleSuite` and the new test pass on Spark 3.5, 4.0 and 4.1. - The other `SparkToColumnar` tests in `CometExecSuite` pass on 3.5 and 4.1. - Semantic scalafix, scalastyle and spotless on 3.5, and prettier, are clean. I did not run the 3.4 and 4.2 profiles, which plan these queries like 3.5 and 4.1, or Spark's SQL tests, which set none of these configs. -- 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]
