Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #25511 +/- ##
==========================================
- Coverage 82.50% 82.50% -0.01%
==========================================
Files 1140 1140
Lines 438050 438057 +7
Branches 438050 438057 +7
==========================================
+ Hits 361415 361417 +2
Misses 54829 54829
- Partials 21806 21811 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
There's also #25513 for the same bug |
Contributor
Author
|
#25513 is closed without merging. This PR includes SQL regressions for projection reuse and equality simplification. |
neilconway
reviewed
Sep 23, 2026
neilconway
left a comment
Contributor
There was a problem hiding this comment.
Overall looks reasonable!
Can we cleanup/simplify is_duplicate_insensitive_aggregate, please?
Contributor
Author
|
Done in 97efd3a. |
1fanwang
force-pushed
the
1fannnw/fix-udf-volatility
branch
from
September 24, 2026 06:16
97efd3a to
624078d
Compare
Signed-off-by: 1fanwang <1fannnw@gmail.com>
The shared expression walk checks aggregate volatility, so a separate signature check is redundant. Signed-off-by: 1fanwang <1fannnw@gmail.com>
1fanwang
force-pushed
the
1fannnw/fix-udf-volatility
branch
from
September 24, 2026 06:54
624078d to
b2707e9
Compare
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Sep 24, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Sep 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #25504.
Rationale for this change
Repeated calls to a volatile higher-order UDF can reuse the first call's result. Comparing two calls can also return true without invoking the function. Both queries now execute each call.
What changes are included in this PR?
The volatility check reads the declared signature for aggregate, window and higher-order expressions, as it already does for scalar expressions. This includes aggregates used as window functions. The join optimizer uses the shared repeatability check instead of checking aggregate volatility separately.
What is the testing strategy for this PR?
The regression registers a counter UDF and runs both queries through the SQL engine. It also checks every function kind, volatility level and aliased expression. Join-optimizer controls preserve DISTINCT handling and reject volatile expressions and subqueries.
On macOS arm64 with Rust 1.98.1 and the pinned test data:
Raw logs
Before the fix:
After:
Are there any user-facing changes?
The affected queries return the results of separate function calls. There is no public API change.