GH-41329: [C++][Gandiva] Fix gandiva cache size env var #41330
Conversation
|
cc @pitrou |
|
|
github-actions
bot
added
Component: C++
Component: Gandiva
Component: Documentation
awaiting review
| namespace internal { | ||
| // Only called once by GetCapacity(). | ||
| // Do the actual work of getting the capacity from env var. | ||
| // Also makes the testing easier. |
There was a problem hiding this comment.
It is not possible in google test to re-initialize a static variable. So have this dedicated function to do the actual work eagerly, then we can test it instead of GetCapacity (which contains the static variable).
github-actions
bot
added
awaiting committer review
cpp/src/gandiva/cache_test.cc
Outdated
| TEST(TestCache, TestGetCacheCapacityEnvVar) { | ||
| // Uncleared env var may have side-effect to subsequent tests. Use a structure to help | ||
| // clearing the env var when leaving the scope. | ||
| struct ScopedEnvVar { |
There was a problem hiding this comment.
Can use the existing EnvVarGuard from our testing utilities.
There was a problem hiding this comment.
Ah, that's great to know. Will do. Thank you!
| GANDIVA_EXPORT | ||
| int GetCapacityFromEnvVar(); | ||
| } // namespace internal | ||
|
|
||
| GANDIVA_EXPORT | ||
| int GetCapacity(); |
There was a problem hiding this comment.
Can this be called GetCacheCapacity? The current name is too imprecise.
There was a problem hiding this comment.
Yes, it can. But is this public API?
There was a problem hiding this comment.
I have no idea :-) You could deprecate the old API if we want to ensure a smoother migration.
There was a problem hiding this comment.
Addressed with deprecating old API and adding renamed one.
| bool ok = ::arrow::internal::ParseValue<::arrow::Int32Type>( | ||
| env_value.c_str(), env_value.size(), &capacity); |
There was a problem hiding this comment.
Updated the parsing according to the recommendation from #41335 (comment)
|
Shall we move on with this? @pitrou |
Co-authored-by: Antoine Pitrou <[email protected]>
|
Sorry for forgetting about this PR. I've rebased and will merge if CI is green. |
|
After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit e6ab174. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 11 possible false positives for unstable benchmarks that are known to sometimes produce them. |
…#41330) ### Rationale for this change Gandiva cache size validity checks are not robust enough (the negativity test is broken), and they are not currently tested. ### What changes are included in this PR? 1. Fix checking gandiva cache size env var. 2. Make cache size static so it only gets evaluated once. 3. Add test cases. 4. Enrich the description in the document about this env var. ### Are these changes tested? UT included. ### Are there any user-facing changes? None. * GitHub Issue: apache#41329 Lead-authored-by: Ruoxi Sun <[email protected]> Co-authored-by: Rossi Sun <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>
…#41330) ### Rationale for this change Gandiva cache size validity checks are not robust enough (the negativity test is broken), and they are not currently tested. ### What changes are included in this PR? 1. Fix checking gandiva cache size env var. 2. Make cache size static so it only gets evaluated once. 3. Add test cases. 4. Enrich the description in the document about this env var. ### Are these changes tested? UT included. ### Are there any user-facing changes? None. * GitHub Issue: apache#41329 Lead-authored-by: Ruoxi Sun <[email protected]> Co-authored-by: Rossi Sun <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>
Rationale for this change
Gandiva cache size validity checks are not robust enough (the negativity test is broken), and they are not currently tested.
What changes are included in this PR?
Are these changes tested?
UT included.
Are there any user-facing changes?
None.