[dmitry-chirkov-dremio]

Rationale for this change

The cache key is built from Expression::ToString(). Different expressions can print the same text, so the second Projector::Make reuses the compiled code of the first. The result is wrong, with no error. Four cases on main:

• a AND (b OR c) and (a AND b) OR c print the same text.
• String literals are not escaped, so two IF expressions can print the same text.
• The key has no output field type. An int64 output skips validation after an int32 one was compiled, and returns garbage.
• The key ignores the nullability of the fields in the expression. A field node with the wrong nullability is rejected on the first Make, but accepted when a valid one was compiled before.

What changes are included in this PR?

The PR has the tests first, then the fix in expression_cache_key.h.

The fix builds the key with a visitor over the expression tree, with the output field type. It is a compact binary form. Every node starts with a tag, tokens carry their length, and child lists carry their count. Types and names that repeat are written once and then as a short reference. It also sorts IN lists and finds like by node, not by text. The key handles null child nodes, so it no longer crashes before validation.

Are these changes tested?

Yes. 15 new tests: key unit tests in cache_test.cc, and end-to-end tests for Projector and Filter in projector_test.cc, filter_test.cc, boolean_expr_test.cc, if_expr_test.cc and projector_build_validation_test.cc. They cover the cases above, IN value lists, type parameters, floating point literals, null child nodes, and two schemas that differ only in nullability.

On main, 12 of them fail (one crashes with SIGSEGV). Three pass on both keys: FloatingPointBitsAndNulls, because float and double literals already print their bits, and the two tests for schemas that differ only in nullability, which record current behavior.

See the comments for the time to build the key and its size.

Are there any user-facing changes?

No.

This PR contains a "Critical Fix". It fixes a bug that returned incorrect data without an error.

Was AI used for this PR?

AI tools helped investigate the bug and write the fix, tests, and description.

PR code and description written by:

• Human
• AI

Reviewed before submission by:

• Human
• AI
• Not reviewed

• GitHub Issue: [C++][Gandiva] Compiled expression cache returns wrong results for different expressions with the same ToString() #55614

[github-actions]

Thanks for opening a pull request!

This pull request has been automatically converted to a draft because its title doesn't match Arrow's required format.

If this is not a minor PR, could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

After updating the title, you can mark the pull request as ready for review.

See also:

• Other pull requests
• Contribution Guidelines - Contributing Overview

[dmitry-chirkov-dremio]

Benchmark of building the cache key.
The key is built on every Projector::Make, on a cache hit and on a miss, so this cost is paid each time.

Measured locally on an Apple M3 (macOS arm64), compiled with -O2. Each number is the median of repeated runs. The benchmark calls the key constructor only. It does not compile anything.

Expression	Old time	New time	Old key size	New key size
nested IF, 3 arms	0.003 ms	0.0009 ms	238 B	137 B
nested IF, 20 arms	0.021 ms	0.0027 ms	1.5 KB	0.7 KB
nested IF, 673 arms	2.44 ms	0.074 ms	51.6 KB	22.9 KB
nested IF, 5000 arms	71.5 ms	0.54 ms	393 KB	170 KB
flat OR, 100 KB	0.71 ms	0.11 ms	141 KB	121 KB
flat OR, 1 MB	7.2 ms	1.2 ms	1.43 MB	1.23 MB

What this shows:

• Building the key is faster in every case. The old ToString() is quadratic on a deep chain.
• The key is smaller in every case, so the cache does not use more memory than before. In the flat OR the size is mostly the string literals, which are stored as they are.

[github-actions]

⚠️ GitHub issue #55614 has been automatically assigned in GitHub to PR creator.

[dmitry-chirkov-dremio]

CI results for the new tests.

Tests-only commit, before the fix: 10 of the 11 new tests fail, as predicted. gandiva-internals-test and gandiva-projector-test fail on all four platforms (ARM64 Ubuntu 22.04 LLVM 18.1.8, AMD64 Ubuntu 24.04 ASAN UBSAN LLVM 18.1.3, AMD64 macOS 15-intel LLVM 18.1.8, ARM64 macOS 26 LLVM 23.1.4). The same tests fail everywhere:

• TestExpressionCacheKey: IfLiteralBoundaries, OutputTypeAndAlias, InSetsAreDelimitedAndOrdered, BooleanStructure, LikeThreadUniquifier, TypeParametersAreKept, RepeatedNamesAndTypesStayDistinct
• TestBooleanExpr.CacheKeepsAndOrStructure
• TestIfExpr.CacheKeepsStringLiteralBoundaries
• ValidationTestProjector.TestCachedOutputTypeValidation

None of them crash. They fail on an expectation, for example two different expressions with the same key, or wrong output after a cache hit.

TestExpressionCacheKey.FloatingPointBitsAndNulls passes on main, because float and double literals already print raw bits. It guards the new key.

With the fix commit, both test binaries pass on all four platforms.

I added four more regression tests after that, checked locally on the old key:

• ValidationTestProjector.TestCachedFieldNullabilityValidation fails on the old key. A field node with the wrong nullability is rejected on the first Make and accepted after a valid one was cached.
• TestExpressionCacheKey.NullChildNodes crashes the old key with SIGSEGV.
• TestProjector.TestProjectCacheSchemasDifferingInNullability and TestFilter.TestCacheSchemasDifferingInNullability pass on both keys. Schemas that differ only in nullability already get separate cache entries. They record that behavior.

On the current head (cb1781d33e), the Gandiva tests pass on all four platforms.

A few known failures seen previously also appeared. They are not related to this PR.