-
Notifications
You must be signed in to change notification settings - Fork 4k
GH-48761: [C++] Fix duplicate Substrait function URI registration #48792
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Thanks for opening a pull request! 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? or See also: |
|
Just my two cents:
I think we should probably add a test to prevent the regression. If it's difficult to add one, would be best to describe why and how you tested 👍 |
|
|
||
| auto scalar_fn = | ||
| std::make_unique<substrait::Expression::ScalarFunction>(); | ||
|
|
||
| // Encode function ONCE per scalar function (FIX) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please explain how does constructing the scalar function before encoding the call help avoid the repeated URIs as the rest of the changes seem to be just formatting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review and for pointing this out. @ahsanabbas123 , @HyukjinKwon
I want to clarify that the earlier discussion can be ignored — I’ve updated the implementation since then.
What changed:
The fix now ensures that EncodeFunction(call.id()) is invoked exactly once per ScalarFunction, by constructing the ScalarFunction object before recursively encoding arguments. This prevents repeated registration of the same function URI when serializing nested expressions (e.g. large OR chains), which was the root cause of the exponential growth.
Why this fixes the issue:
Previously, function encoding could occur during recursive argument serialization, causing duplicate URI registrations. With this change, the function reference is encoded once per scalar function, regardless of nesting depth.
Testing:
I reproduced the original issue using a Python script that serializes expressions with increasing OR conditions and verified that:
serialization still succeeds
the number of registered extension URIs no longer grows exponentially
no regressions were observed
Please let me know if you’d like me to add a regression test for this, or if the explanation can be improved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to update the PR description. I would want to follow how you tested and verify the PR 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I verified this change by building Arrow C++ from source after the fix (make -j$(nproc)) and confirmed the Substrait code compiles successfully.
What does this PR do?
This PR fixes an issue where Substrait scalar expressions could cause
duplicate registration of the same function URI during serialization.
The function reference was being encoded multiple times while building
nested expressions (e.g. large OR chains), leading to exponential growth
in extension URIs and serialized plan size.
What was changed?
EncodeFunction(call.id())is invoked exactly once perScalarFunctionencoding.Why is this needed?
Without this fix, expressions with many nested logical operators
(e.g. OR conditions) cause the Substrait plan size to grow exponentially,
which can severely impact memory usage and performance.
Testing
increasing OR conditions.
introduce regressions.
Related issue
Fixes #48761