[lriggs]

Rationale for this change

I originally identified this issue in arrow-java and fixed it on the arrow-java side with a coarse grained lock on the Projector and Filter Make methods. That fixed the issue but adds some undesirable overhead in certain multi threaded workflows.
This issue captures the deeper problem that is occurring in the C++ side of Gandiva and was not touched by my previous fix though it is prevented from happening. I have a proposed solution for this and believe it is a better overall solution.

Projector::Make() and Filter::Make() read the shared expression cache once to decide the is_cached status and then call SetLLVMObjectCache(). That performs its own second, unsynchronized read of the same key before pre-loading a cached object into the LLJIT.

Those two reads could disagree. If another thread compiling the identical (schema, expressions, selection vector mode, configuration) tuple inserted between them, the first thread would:

1. take the is_cached == false path, generating expr_0_0 into its IR module, and
2. also see a hit on the second read and addObjectFile() a cached object that defines expr_0_0 as well.

Both then call JITDylib::define for the same symbol in the same JITDylib, and ORC's duplicate-symbol detection fires:

CodeGenError in Gandiva: Failed to add IR module to LLJIT:
In gdv_module_..., duplicate definition of symbol 'expr_0_0'

What changes are included in this PR?

Thread the single cache-lookup result (prev_cached_obj) that already determined is_cached directly into SetLLVMObjectCache(), instead of letting it perform an independent second lookup.

Engine::SetLLVMObjectCache() and LLVMGenerator::SetLLVMObjectCache() now take the resolved std::shared_ptr<llvm::MemoryBuffer> rather than a GandivaObjectCache&. Engine::SetLLVMObjectCache() passes a copy of the buffer to addObjectFile(), as the old GandivaObjectCache::getObject() did, because the shared cache still owns the original buffer.

No new locking — this removes the window rather than serializing around it.

Are these changes tested?

This adds a new deterministic regression test, cpp/src/gandiva/tests/cache_race_test.cc, which is part of gandiva-projector-test.

The test doesn't depend on thread timing. It registers an ExportedFuncsBase hook that inserts an entry for the key being built into the shared cache during Engine::Init(). That is after Make()'s first cache lookup and before the (pre-fix) second lookup in SetLLVMObjectCache(), so the race window is reproduced on every run. It covers both Projector::Make() and Filter::Make(), and checks that the built projector/filter evaluates correctly.

Verified the tests fail with the duplicate-symbol error without this fix.

Verified internally with our product stress tests after removing the arrow-java side of the fix. Once this change is in C++ I can make the java change to remove the synchronization.

Are there any user-facing changes?

This PR includes breaking changes to public APIs.

Engine::SetLLVMObjectCache and LLVMGenerator::SetLLVMObjectCache are GANDIVA_EXPORT so this changes an exported signature. In-tree there are no other callers, and the Gandiva Java side JNI only uses Projector::Make/Filter::Make, which are unchanged.

Was AI used for this PR?

AI was used to diagnose the problem, write code and tests over several iterations before being reviewed by two human developers. A human ran several manual and regression tests against the change. One human reviewed and heavily edited the PR description and also wrote some himself.

PR code and description written by:

• Human
• AI

Reviewed before submission by:

• Human
• AI
• Not reviewed

• GitHub Issue: [C++][Gandiva] Race condition with Gandiva expressions #51663

[dmitry-chirkov-dremio]

Change looks good and I’ll approve once my test-related comments are addressed.

[dmitry-chirkov-dremio]

LGTM

Nit: GH-601 reference is from arrow-java - not sure what are the policies on cross-repo references

[dmitry-chirkov-dremio]

@kou this is good to go.

[kou]

Could you update the PR description before we merge this?

[lriggs]

Could you update the PR description before we merge this?

Sorry, what update? If it's about the GH issues, the link in the description goes to arrow-java correctly. Although at this moment apache/arrow-java#601 gives an error for some reason, though its the right URL.

[kou]

It seems that the latest changes aren't reflected to the description.

I've updated the description. I'll merge this.