1fanwang opened a new pull request, #25511: URL: https://github.com/apache/datafusion/pull/25511
## Which issue does this PR close? Closes https://github.com/apache/datafusion/issues/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. ## What is the testing strategy for this PR? The regression registers a counter UDF and runs both queries through the SQL engine. A separate check covers all function kinds, each volatility level, and aliased expressions. ### Testing Done | Query | Before | After | | --- | --- | --- | | Two counter calls in a projection | (0, 0), one invocation | (0, 1), two invocations | | Equality between two counter calls | true, no invocations | false, two invocations | I used Rust 1.98.1 on macOS arm64 with the installed macOS 15.4 SDK. From the PR checkout, these commands run the same regression source against the baseline and the fix: ```bash export SDKROOT=/Library/Developer/CommandLineTools/SDKs/MacOSX15.4.sdk export CARGO_BUILD_JOBS=2 git worktree add -b 1fannnw/repro-udf-volatility \ ../datafusion-before-25504 42f3888ca2fc82da8548dc4f13533af7af9b5a9f git diff 42f3888ca2fc82da8548dc4f13533af7af9b5a9f HEAD \ -- datafusion/core/tests/user_defined | git -C ../datafusion-before-25504 apply - cargo test --locked --profile ci \ --manifest-path ../datafusion-before-25504/Cargo.toml \ --target-dir target/volatility-before \ -p datafusion --test user_defined_integration volatility -- --nocapture cargo test --locked --profile ci \ --target-dir target/volatility-after \ -p datafusion --test user_defined_integration volatility -- --nocapture ``` <details> <summary>Raw logs</summary> Before the fix, the projection returned: ```text +-------+--------+ | first | second | +-------+--------+ | 0 | 0 | +-------+--------+ calls=1 ``` The equality comparison returned: ```text +-------+ | equal | +-------+ | true | +-------+ calls=0 ``` After the fix, the projection returned: ```text +-------+--------+ | first | second | +-------+--------+ | 0 | 1 | +-------+--------+ calls=2 ``` The equality comparison returned: ```text +-------+ | equal | +-------+ | false | +-------+ calls=2 ``` </details> - [x] Local code review completed ## Are there any user-facing changes? Queries affected by the incorrect volatility check return the results of separate function calls. There is no public API change. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
