dmitry-chirkov-dremio commented on code in PR #50372:
URL: https://github.com/apache/arrow/pull/50372#discussion_r3593647924
##########
cpp/src/gandiva/tests/projector_test.cc:
##########
@@ -39,6 +40,45 @@ using arrow::float32;
using arrow::int32;
using arrow::int64;
+namespace {
+
+int CountOccurrences(const std::string& text, const std::string& needle) {
+ int count = 0;
Review Comment:
Also asked Codex to walk me through large expression and performance
coverage:
There is no new large expression test or benchmark in the PR.
The repository already has:
* TimedTestBigNested — approximately 20 nested if expressions, but Projector
construction occurs outside the timed loop, so it measures evaluation rather
than CSE/build cost.
* TimedTestExprCompilation — measures Projector construction, but its
expressions do not contain meaningfully large repeated subtrees.
Neither benchmark was changed, and the PR does not report running either one.
Seems like a meaningful gap.
The PR provides no evidence that:
* Projector/Filter construction becomes faster for repeated expressions.
* Generated IR becomes smaller.
* Runtime evaluation improves.
* The pass does not regress ordinary expressions with no duplicates.
I'd love to see before/after measurements for at least:
1. A deep expression with no duplicates - worst-case overhead with no benefit
2. A balanced expression containing many repeated safe subtrees
3. Repeated unsafe subtrees that must not fold
4. Several sizes, such as 10, 100, and 1,000 nodes
5. Projector/Filter build time and generated IR size; evaluation time
separately
ps I'll ask our team if we have real world examples of "large expressions" -
we typically see them in/around CASE expressions where individual branches are
semi-repeated expanded nested calcs
##########
cpp/src/gandiva/tests/projector_test.cc:
##########
@@ -39,6 +40,45 @@ using arrow::float32;
using arrow::int32;
using arrow::int64;
+namespace {
+
+int CountOccurrences(const std::string& text, const std::string& needle) {
Review Comment:
Asked Codex to walk me through testing of the changes:
The important missing tests are precisely the “do not fold” rules:
* kResultNullInternal
* NeedsContext()
* NeedsFunctionHolder()
* CanReturnErrors()
* A safe parent containing one unsafe child
* Unknown/custom registry functions
* Opaque InExpressionNode
* Structurally similar but unequal expressions
* Different literal values/types
* Operand order, such as add(a,b) versus add(b,a)
* Multiple output expressions sharing a subtree
* Custom volatile or nondeterministic functions with default flags
There are no focused unit tests for FoldCommonSubexpressions() itself.
Everything is tested indirectly through Projector/Filter and IR strings. The
positive rewrite coverage is respectable, but the conservative safety contract
is largely untested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
