You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "LinSimon-901101 (via GitHub)" <gi...@apache.org> on 2026/09/12 10:22:47 UTC
[PR] feat: route map lookups through codegen dispatcher [datafusion-comet]
LinSimon-901101 opened a new pull request, #5875:
URL: https://github.com/apache/datafusion-comet/pull/5875
## 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 these cases through the JVM codegen dispatcher preserves Spark's lookup semantics while keeping supported projections inside Comet.
Related work: #5867 also addresses #5580. I claimed this issue and completed this implementation locally before opening this PR. This draft makes the implementation and cross-version validation available for comparison and coordination. I'm 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?
- Add `CodegenDispatchFallback` to `CometMapExtract` and `CometElementAt`, preserving the existing native key-type restrictions.
- Extend SQL and Scala regression coverage, including dispatch/native assertions, constant-folded inputs, configuration switches and ANSI nullable nondeterministic operands.
- Update expression support documentation and compatibility audit notes.
## How are these changes tested?
Validated locally on macOS arm64 with Spark 3.4.3, 3.5.9, 4.0.4 and 4.1.3 using focused SQL fixtures and the full `CometMapExpressionSuite` and `CometArrayExpressionSuite`. Version-specific exclusions remain in place.
Coverage includes signed zero, NaN, infinities, subnormal values, NULL/empty maps, NULL keys and values, complex keys, collation and NULL short-circuiting. The new dispatch assertion failed before the implementation change.
Native build, Scalastyle, Spotless, Apache RAT and Spark 4.0 Scalafix CHECK passed locally. No performance improvement is claimed; benchmarks were not run.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route map lookups through codegen dispatcher [datafusion-comet]
Posted by "LinSimon-901101 (via GitHub)" <gi...@apache.org>.
LinSimon-901101 commented on PR #5875:
URL: https://github.com/apache/datafusion-comet/pull/5875#issuecomment-5971145821
@andygrove I've opened #6570 to track the dispatcher partition-index issue separately, as suggested.
Please let me know if I missed anything.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route map lookups through codegen dispatcher [datafusion-comet]
Posted by "LinSimon-901101 (via GitHub)" <gi...@apache.org>.
LinSimon-901101 commented on PR #5875:
URL: https://github.com/apache/datafusion-comet/pull/5875#issuecomment-5675641313
Thanks for the review. Fixed in 78e60cd0b: I removed the two non-dispatch reasons from `getUnsupportedReasons` and updated the helper to assert that `element_at` itself dispatches.
All 36 tests passed locally on Spark 4.1.3; CI validation remains pending.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route map lookups through codegen dispatcher [datafusion-comet]
Posted by "LinSimon-901101 (via GitHub)" <gi...@apache.org>.
LinSimon-901101 commented on PR #5875:
URL: https://github.com/apache/datafusion-comet/pull/5875#issuecomment-5971007961
@andygrove Thanks for the review and for pointing out the clearer follow-up directions. I’ve marked this PR as a draft.
I’ll also update `expressions.md`, `map_funcs.md`, the Spark 4.2 notes, and add the validation and test coverage.
For the dispatcher partition-index issue, I agree that opening a separate issue is a better approach than handling it in this PR.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route map lookups through codegen dispatcher [datafusion-comet]
Posted by "LinSimon-901101 (via GitHub)" <gi...@apache.org>.
LinSimon-901101 commented on code in PR #5875:
URL: https://github.com/apache/datafusion-comet/pull/5875#discussion_r3999030284
##########
spark/src/main/scala/org/apache/comet/serde/maps.scala:
##########
@@ -114,7 +117,9 @@ object CometMapValues extends CometExpressionSerde[MapValues] {
}
}
-object CometMapExtract extends CometExpressionSerde[GetMapValue] {
+object CometMapExtract extends CometExpressionSerde[GetMapValue] with CodegenDispatchFallback {
Review Comment:
Thanks for the review. I've added and run the requested map-lookup matrix in `CometCodegenDispatchBenchmark` on Spark 4.1.3, with two warmed runs, fresh-JVM first-use measurements, and answer/route checks.
The results show a tradeoff: small-map mixed projections improve, while lookup-only and larger DOUBLE-key maps can be slower than dispatcher-off.
The current routing keeps projections in Comet at the cost of 8.7–11.4% longer execution times for the measured DOUBLE-key lookup-only cases at 65,536 rows; small-map mixed projections benefit.
Logs and the full comparison are attached.
[pr-5875-benchmark-evidence.zip](https://github.com/user-attachments/files/32156330/pr-5875-benchmark-evidence.zip)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route map lookups through codegen dispatcher [datafusion-comet]
Posted by "LinSimon-901101 (via GitHub)" <gi...@apache.org>.
LinSimon-901101 commented on PR #5875:
URL: https://github.com/apache/datafusion-comet/pull/5875#issuecomment-5731198003
Thanks for the coordination. I'll let #5867 carry the overlapping implementation and shared lookup fixture changes.
Once it lands, I'll rebase this PR onto `main` and keep only the non-overlapping coverage, collated-column tests, and benchmark updates.
I'll also rerun the benchmark, including first-use measurements, on the rebased head and revalidate the focused tests.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route map lookups through codegen dispatcher [datafusion-comet]
Posted by "LinSimon-901101 (via GitHub)" <gi...@apache.org>.
LinSimon-901101 commented on PR #5875:
URL: https://github.com/apache/datafusion-comet/pull/5875#issuecomment-5675795987
Regarding the earlier discussion about coordinating with #5867, thanks for the suggestion, @andygrove. Once #5867 is merged, I’ll rebase this PR onto the updated main branch and keep it focused on the additional `map_lookup_dispatch.sql` coverage, collated-column tests, and benchmark.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route map lookups through codegen dispatcher [datafusion-comet]
Posted by "LinSimon-901101 (via GitHub)" <gi...@apache.org>.
LinSimon-901101 commented on code in PR #5875:
URL: https://github.com/apache/datafusion-comet/pull/5875#discussion_r3999030284
##########
spark/src/main/scala/org/apache/comet/serde/maps.scala:
##########
@@ -114,7 +117,9 @@ object CometMapValues extends CometExpressionSerde[MapValues] {
}
}
-object CometMapExtract extends CometExpressionSerde[GetMapValue] {
+object CometMapExtract extends CometExpressionSerde[GetMapValue] with CodegenDispatchFallback {
Review Comment:
Thanks for the review. I've added and run the requested map-lookup matrix in `CometCodegenDispatchBenchmark` on Spark 4.1.3, with two warmed runs, fresh-JVM first-use measurements, and answer/route checks.
The results show a tradeoff: small-map mixed projections improve, while lookup-only and larger DOUBLE-key maps can be slower than dispatcher-off.
Logs and the full comparison are attached. The routing remains unchanged pending review of these measured tradeoffs.
[pr-5875-benchmark-evidence.zip](https://github.com/user-attachments/files/32156330/pr-5875-benchmark-evidence.zip)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route map lookups through codegen dispatcher [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #5875:
URL: https://github.com/apache/datafusion-comet/pull/5875#discussion_r3997504094
##########
spark/src/main/scala/org/apache/comet/serde/maps.scala:
##########
@@ -114,7 +117,9 @@ object CometMapValues extends CometExpressionSerde[MapValues] {
}
}
-object CometMapExtract extends CometExpressionSerde[GetMapValue] {
+object CometMapExtract extends CometExpressionSerde[GetMapValue] with CodegenDispatchFallback {
Review 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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route map lookups through codegen dispatcher [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #5875:
URL: https://github.com/apache/datafusion-comet/pull/5875#issuecomment-5955541661
I would like to do a deeper review of this PR before it is merged.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route map lookups through codegen dispatcher [datafusion-comet]
Posted by "LinSimon-901101 (via GitHub)" <gi...@apache.org>.
LinSimon-901101 commented on PR #5875:
URL: https://github.com/apache/datafusion-comet/pull/5875#issuecomment-5933154232
Fixed the compilation failure in `74e8cc95b`.
Validated the fix together with main `9a41a11a1` in [[fork CI](https://github.com/LinSimon-901101/datafusion-comet/actions/runs/36862087919)].
The run passed, including compilation on Spark 3.5 / JDK 17, Rust tests, Spark 4.1 Comet suites, and TPC-H/TPC-DS result checks.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route map lookups through codegen dispatcher [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #5875:
URL: https://github.com/apache/datafusion-comet/pull/5875#issuecomment-5719747109
Thanks for calling out #5867 in the description instead of leaving us to find it in review. Confirming the overlap is real: you are both adding `CodegenDispatchFallback` to `CometMapExtract` and `CometElementAt` in `maps.scala`, and both touching the `element_at` / `get_map_value` fixtures.
The difference I see is that #5867 also closes #5781, which fixes the stateful child being serialized twice under the `CASE WHEN child IS NOT NULL` guard in `size`, `array_append`, `arrays_zip` and `map_from_arrays`. That has no equivalent here, so #5867 is the wider change and is probably the one to land. Would you be willing to take up your own offer and rebase the extra coverage and the benchmark cases on top of it?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route map lookups through codegen dispatcher [datafusion-comet]
Posted by "LinSimon-901101 (via GitHub)" <gi...@apache.org>.
LinSimon-901101 commented on code in PR #5875:
URL: https://github.com/apache/datafusion-comet/pull/5875#discussion_r3999030284
##########
spark/src/main/scala/org/apache/comet/serde/maps.scala:
##########
@@ -114,7 +117,9 @@ object CometMapValues extends CometExpressionSerde[MapValues] {
}
}
-object CometMapExtract extends CometExpressionSerde[GetMapValue] {
+object CometMapExtract extends CometExpressionSerde[GetMapValue] with CodegenDispatchFallback {
Review Comment:
Thanks for the review. I've added and run the requested map-lookup matrix in `CometCodegenDispatchBenchmark` on Spark 4.1.3, with two warmed runs, fresh-JVM first-use measurements, and answer/route checks.
The results show a tradeoff: small-map mixed projections improve, while lookup-only and larger DOUBLE-key maps can be slower than dispatcher-off.
Logs and the full comparison are attached.
The current routing keeps projections in Comet at the cost of 8.7–11.4% longer execution times for the measured DOUBLE-key lookup-only cases at 65,536 rows; small-map mixed projections benefit.
[pr-5875-benchmark-evidence.zip](https://github.com/user-attachments/files/32156330/pr-5875-benchmark-evidence.zip)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org