GH-50624: [C++][Compute] Tighten case_when exact dispatch for parameterized types - #50625
Conversation
There was a problem hiding this comment.
Pull request overview
This PR tightens case_when kernel selection so exact dispatch only accepts identical concrete DataTypes for value arguments (including parameterized/nested types), preventing incorrect kernel matches that could previously yield corrupted results.
Changes:
- Renames the exact-dispatch helper to
AllValueTypesMatchConstraint()and generalizes it to enforce identical value argumentDataTypes. - Applies the same exact-match constraint across all
case_whenvalue kernels (primitive, binary, fixed-size binary, decimals, and nested). - Adds regression tests covering exact-dispatch matching/failing for parameterized types and runtime failures for mismatched parameterized value types.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| cpp/src/arrow/compute/kernels/scalar_if_else.cc | Enforces identical concrete value DataTypes via a shared match constraint for all case_when kernels to prevent unsafe exact dispatch. |
| cpp/src/arrow/compute/kernels/scalar_if_else_test.cc | Adds coverage ensuring parameterized types only exact-match when identical and that mismatched cases fail as expected. |
ca07100 to
991fb5d
Compare
991fb5d to
463e5e4
Compare
| for (auto&& type : types) { | ||
| auto exec = GenerateTypeAgnosticPrimitive<CaseWhenFunctor>(*type); | ||
| AddCaseWhenKernel(scalar_function, type, std::move(exec)); | ||
| AddCaseWhenKernel(scalar_function, type, std::move(exec), constraint); |
There was a problem hiding this comment.
This is in a loop so I don't think std::move is proper here. I've changed the parameter type to const ref to eliminate the ambiguity. Thanks.
|
I'll merge this after CI turns green. Thanks for reviewing @pitrou ! |
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 974c3fc. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 9 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
case_whenexact dispatch was too permissive for parameterized value types. When the value arguments were registered by type id only,DispatchExactcould incorrectly accept incompatible concrete types and return successful but corrupted results instead of failing or falling back toDispatchBest.This fixes #50624.
What changes are included in this PR?
AllValueTypesMatchConstraint()to make its intent explicitDataTypesfixed_size_binary,list,fixed_size_list,struct, and dictionary exact-dispatch mismatchesAre these changes tested?
Yes.
This PR extends
TestCaseWhen.DispatchExactand addsTestCaseWhen.ParameterizedValueTypeMismatch. I also ran targetedarrow-compute-scalar-if-else-testcoverage for:TestCaseWhen.DispatchExactTestCaseWhen.DispatchBestTestCaseWhen.ParameterizedValueTypeMismatchAre there any user-facing changes?
Yes.
case_whennow rejects incompatible parameterized value types that previously could be incorrectly exact-dispatched, which could lead to corrupted results.This PR contains a "Critical Fix". It fixes a bug where
case_whencould return successful but corrupted results for incompatible parameterized value types.