andygrove commented on PR #4799: URL: https://github.com/apache/datafusion-comet/pull/4799#issuecomment-5094047732
Zooming out from the individual file comments, I think this PR is doing several independent things and would be much easier to review and to revert if it were split. Right now it is 24 files touching 25 distinct legacy configs plus a `VariantType` change and a `Cast` dispatch change, and the pieces have very different risk profiles. A split I would find easy to review, roughly in dependency order: **1. Per-scan Parquet fallback.** `CometScanRule.parquetFallbackReason` plus the `CometScanRuleSuite` tests and the doc table. Fully self-contained, three configs, low risk. This one could go in almost as-is. **2. Session-wide fallback.** `LegacyConfFallback`, both `ShimLegacyConfFallback` files, `COMET_LEGACY_CONF_FALLBACK_ENABLED`, the `isCometLoaded` hook, the `CometSparkSessionExtensionsSuite` test, and the compat doc section. Thirteen analyzer and optimizer configs, all with the same shape. Also self-contained. **3. `CometCast` mixing in `CodegenDispatchFallback`.** This is the one I would most like to see on its own. It flips every `Unsupported` and non-opted-in `Incompatible` cast from "fall back to Spark" to "run Spark's `doGenCode` inside the Comet kernel". `Cast` is everywhere, so this is a broad behavior and performance change, and the test churn shows it: map to string casts lose `expect_fallback`, the `CalendarIntervalType` to `StringType` fallback assertion is deleted, and `CometCastSuite` swaps `checkSparkAnswerAndFallbackReason` for `checkSparkAnswerAndOperator`. As its own PR it could carry benchmark numbers and a clear before and after, and it would be revertable without losing the legacy-config work. **4. `VariantType` guard.** The `isVariantType` shim, the `CometCast` guard, and the two `dev/diffs` hunks. Unrelated to legacy configs, and it stands on its own merits. **5. Per-expression legacy gates.** These could go together or separately. I already noted on the individual files that `In`/`InSet` and `ArrayInsert` read as standalone fixes. The `In`/`InSet` one in particular is more than a legacy opt-in, since on 3.4 and 3.5 with ANSI off `legacyNullInEmptyBehavior` resolves to true by default, so it is fixing the default path. That deserves its own tests and its own line in the changelog rather than being folded into a legacy-config PR. Note that 3 has to land before the `castComplexTypesToString` change in 5, since that change depends on the dispatcher being available on `Cast`. The `timeParserPolicy` gate needs rework anyway per my other comment, so it would naturally drop out of the first pass. Happy to review each piece quickly as it comes. Splitting would also mean the low-risk parts (1 and 2) are not blocked on working out the `Cast` dispatch question. (Comment drafted with LLM assistance.) -- 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]
