feat: route map lookups through codegen dispatcher - #5875
feat: route map lookups through codegen dispatcher#5875LinSimon-901101 wants to merge 2 commits into
Conversation
sunchao
left a comment
There was a problem hiding this comment.
Correctness
At be8bf9cda932 against 3810936b40af, this changes the handling of map lookups that the native key gate rejects. Floating-point, collated and complex keys now use Spark's generated lookup inside Comet. The native MapKeySupport.keySupport implementation is unchanged, so the PR does not enable native equality for those key types.
I checked the map lookup helpers and generated equality on the maintained Spark 3.5 and 4.0 branches. Both lookup spellings use the first matching key, return NULL for a missing key or NULL value, and skip key evaluation when the map is NULL. Generated equality handles NaNs and signed zero, with collation-aware string equality on Spark 4.0. Dispatch serializes the whole expression, including its operands, and sends only referenced attributes as data arguments. This also preserves single evaluation for the ANSI nullable nondeterministic array/map case. The expression-enable check, dispatcher switch and supported-type gate remain in control. I found no new correctness defect in the reviewed changes.
The SQL fixtures pin dispatch for both lookup spellings and native execution for ordinary string keys. They cover both ANSI settings, floating-point boundaries, nested NULLs, complex keys and NULL-map short-circuiting. The Scala tests cover folded inputs and disabled configurations. I inspected the four attached author logs: they report 31 passing tests and five cancellations on each Spark 3.x run, and 36 passing tests on each Spark 4.x run. Those logs contain no commit stamp, so their association with this head remains the author's report. I ran source assertions and diff validation, but did not run Spark/JVM or native tests locally. The maintained Spark 3.4 and 4.1 branches were unavailable for independent source comparison. Product CI is awaiting approval and has not supplied test execution evidence.
Performance
The remaining P2 is benchmark evidence for the newly enabled dispatch routes. They can retain a larger Comet pipeline, but they also introduce expression transport, per-task kernel setup, per-batch output allocation and JVM map traversal where the projection previously used Spark. The balance depends on map size, key/value types and surrounding expressions. The existing dispatch benchmark already provides dispatcher-on, dispatcher-off and pure-Spark comparisons, but does not cover map lookups. Please add representative map cases and report the results as described in the inline comment. No benchmark was run during this review, and no slowdown is asserted.
Design
Reusing CodegenDispatchFallback is a small, coherent way to preserve Spark's lookup semantics without weakening the native key gate or adding a second equality implementation. Dispatching the entire ANSI nondeterministic expression is also the appropriate boundary because splitting its operands would lose the evaluation order this change needs to preserve. Ordinary native lookups retain their current implementation and null guard. I found no additional design change required.
Abstraction & complexity
The two existing serdes gain a marker trait and documentation reasons, with no new execution framework. Sharing MapKeySupport.unsupportedReasons keeps the two lookup spellings aligned, and the new test helper wraps existing answer/operator and dispatcher-activity assertions. These additions fit the existing abstractions and do not introduce unnecessary indirection. The expanded SQL coverage carries most of the change's size and directly exercises the routing decision.
| } | ||
|
|
||
| object CometMapExtract extends CometExpressionSerde[GetMapValue] { | ||
| object CometMapExtract extends CometExpressionSerde[GetMapValue] with CodegenDispatchFallback { |
There was a problem hiding this comment.
Performance
[P2] Could you add and run map-lookup cases in CometCodegenDispatchBenchmark before enabling these routes by default? This mixin makes previously falling-back projections pay the dispatcher's per-task kernel setup, per-batch Arrow output allocation and JVM map traversal. Whether retaining Comet outweighs those costs depends on the workload, particularly for small maps and lookup-only projections. The PR explicitly reports no benchmarks, and the existing driver has no map-lookup case. Please compare dispatcher on, dispatcher off and pure Spark using column inputs, short and larger maps, scalar and complex keys, and both lookup-only and mixed projections. Include first-use and warmed results with answer and route checks so the new default has measured support. This is a request for missing evidence, not a measured slowdown.
Which issue does this PR close?
Closes #5580.
Rationale for this change
Map lookups with floating-point, non-default collated or complex keys currently cause the enclosing projection to fall back to Spark. Routing supported cases through the JVM codegen dispatcher preserves Spark's lookup semantics while keeping the projection inside Comet.
Related work: #5867 also addresses #5580. This PR provides the implementation and cross-version validation for comparison and coordination. I am happy to contribute non-overlapping tests in a focused follow-up if the maintainers prefer to proceed with #5867.
What changes are included in this PR?
CodegenDispatchFallbacktoCometMapExtractandCometElementAt, preserving the existing native key-type restrictions.How are these changes tested?
After merging main and rebuilding the native library, revalidated
be8bf9cdalocally on macOS arm64 using focused SQL fixtures and the fullCometMapExpressionSuite:All four runs completed with
BUILD SUCCESS. The five canceled tests on Spark 3.x require Spark 4.0+ collation or MapSort support. Version-specific SQL exclusions remain in place.Coverage includes signed zero, NaN, infinities, subnormal values, NULL/empty maps, NULL keys and values, complex keys, collation where supported, and NULL short-circuiting. The new dispatch assertion failed before the implementation change.
Before merging main, validation on
a5aac026falso included the fullCometArrayExpressionSuiteacross all four Spark versions, plus native build, Scalastyle, Spotless, Apache RAT and Spark 4.0 Scalafix CHECK. The full array suite and Scalafix were not rerun onbe8bf9cda.These are local validation results, not CI results. No performance improvement is claimed; benchmarks were not run.
spark-3.4.txt
spark-3.5.txt
spark-4.0.txt
spark-4.1.txt