fix: route translate through codegen dispatcher by default - #5606
fix: route translate through codegen dispatcher by default#5606adibmbrk wants to merge 1 commit into
Conversation
Mix NativeOptInAvailable into CometStringTranslate so it runs natively by default via the JVM codegen dispatcher (Spark-compatible), instead of falling back to Spark unless allowIncompatible is set. The incompatible native DataFusion path becomes opt-in via allowIncompatible, matching CometInitCap and CometStringReplace. Docs updated to Hybrid. Closes apache#5585 Signed-off-by: adibmbrk <adibmbrk@gmail.com>
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
PR summary
The default JVM dispatcher path keeps translate inside the Comet projection while preserving Spark semantics. Keeping the native implementation behind the existing opt-in also makes the compatibility boundary clear.
CI and validation
Reviewed dd198bb3 against base 6dedcfee. This was a source review, without running builds or tests. The four current-head workflows show action_required, so they do not establish passing CI. The Spark 4.1 suite results in the description are author-reported. One P2 finding is attached inline about the existing default SQL fixture.
Performance validation
Could you add a focused microbenchmark comparing the BASE default Spark fallback with this HEAD's default dispatcher, with the native opt-in disabled? A literal translation map and a representative changing-map column would cover the main cache behaviors. Please consume and compare the results, verify the actual execution paths, and keep the build, Spark/JDK, hardware, data and batch settings matched. Separating first-use compilation from warmed throughput and allocation/GC would show whether avoiding fallback transitions offsets the new dispatch costs. I have not measured a slowdown.
| Some(incompatReason)) | ||
| override def getSupportLevel(expr: StringTranslate): SupportLevel = | ||
| if (!CometConf.isExprAllowIncompat(getExprConfigName(expr))) { | ||
| Compatible(nativeOptIn = |
There was a problem hiding this comment.
[P2] Update the default translate SQL fixture
Could you update the default cases in string_translate.sql along with this change? It still has four expect_fallback(is not fully compatible with Spark) blocks. The SQL-file suite registers these blocks and requires that fallback contract, but this Compatible branch now accepts the first ordinary three-string-column query and dispatches it by default. The successful new route therefore cannot satisfy the old fixture expectation, even when its result values are correct. Please check Comet execution and result parity for the default cases, and keep any dispatcher-disabled fallback case separate with its actual reason. This is source-derived, not an observed test failure.
2 participants
Which issue does this PR close?
Closes #5585.
Rationale for this change
CometStringTranslatereported itself asIncompatibleunconditionally, forcing whole projections to fall back to Spark unlessspark.comet.expression.StringTranslate.allowIncompatible=truewas set. Comparable string functions (CometInitCap,CometStringReplace) instead run natively by default through the JVM codegen dispatcher and treat their semantic differences as an opt-in caveat.What changes are included in this PR?
CometStringTranslatemixes inNativeOptInAvailable: default isCompatibleand routes through the codegen dispatcher (Spark-compatible); the incompatible native DataFusion path is opt-in viaallowIncompatible.translateupdated fromNativetoHybrid.How are these changes tested?
Ran the affected suites against Spark 4.1 (
-Dtest=none -Dsuites=...):CometFallbackInvarianceSuite—pass=25 fail=0 excused=0 vacuous=0(translate is now a real native-vs-fallback parity pass rather than the previous SKIPPED-VACUOUS case).CometStringExpressionSuite— 39 tests succeeded, 0 failed.