peter-toth commented on code in PR #57357:
URL: https://github.com/apache/spark/pull/57357#discussion_r3628726920
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -7864,6 +7864,20 @@ object SQLConf {
.createWithDefault(false)
}
+ val LEGACY_ALLOW_NON_DETERMINISTIC_V2_FILTER_PUSHDOWN =
Review Comment:
@aokolnychyi on "is it even safe to push a non-det condition into any
connector?" — as I mentioned above, I think the answer is no, and not only
because of partial-accept double-evaluation. Even if we (a) document a
fully-accept-or-deny rule so nothing is evaluated twice, and (b) push the seed
so the RNG sequence is identical, a full push can still change results.
Consider a filter with a translatable non-det predicate (say `rand() > 0.5`)
next to a non-translatable one (say `monotonically_increasing_id() < 100`).
Only the first gets pushed: it's forced down to the scan and evaluated first,
over every scanned row, while the untranslatable one stays post-scan, over the
survivors. Splitting and reordering two non-det predicates this way changes
which rows the pushed one runs over, so the final result can differ — with a
single evaluation and the same seed. It's the same move the optimizer refuses
to make elsewhere (`CombineFilters`, `PushPredicateThroughNonJoin`, etc. all
gate on `deterministic`).
Given that, on the options:
- The capability method (`pushDownNondeterministicPredicates`) is the one I
like least — it's a permanent public API that lets a connector keep depending
on a behavior that isn't sound even for a fully-enforcing source. I'd rather
not add it.
- On "this can suddenly break all connectors": that's the intended,
documented breaking change (there's a migration-guide bullet in the PR). The
default now does the safe thing — keep non-det filters post-scan — so nothing
silently flips to a worse behavior. The legacy config only lets someone who
built on the old behavior restore it, and we can deprecate and remove it later.
So I'd like to land this either as-is (safe default + the legacy config as a
temporary, deprecatable escape hatch) or with no flag at all (just never push).
Either works for me — I'd just avoid the capability.
--
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]
