szehon-ho commented on PR #57727: URL: https://github.com/apache/spark/pull/57727#issuecomment-5186467445
Thanks @cloud-fan and @peter-toth for the reviews. @peter-toth, these were excellent catches, and the non-determinism one in particular -- a predicate that the source evaluates on its own roll of `rand()` and Spark then re-rolls is a silent wrong-answer bug, and it only becomes a correctness issue once `fullyPushedFilterAttributes` removes the post-scan `FilterExec`, which is new in this PR. Thank you for chasing it to the root and fixing it upstream in #57760 instead of handing it back. Pushed, addressing everything: - The test fixture now binds and interprets pushed predicates over partition columns and prunes accordingly, instead of only recording them, and the fully-pushed test uses matching and nonmatching partitions so an incorrect `FilterExec` removal fails the test. - The Catalyst branch screens runtime filters with `isPushablePartitionFilter`, matching the V2 `PartitionPredicate` pass. Redundant once #57760 lands and this rebases, but harmless. - `RowLevelOperationRuntimeGroupFiltering` now recognizes the new interface for both group-based and delta-based operations, so adopters keep runtime group filtering for DELETE, UPDATE and MERGE. Added `RowLevelOperationCatalystRuntimeFilterSuiteBase` with a group-based and a delta-based subclass to cover it. - Javadoc: carried over the partitioning-preservation contract, and documented that filter attributes must be top-level fields present in `readSchema` -- on the new trait and on the existing `SupportsRuntimeFiltering` / `SupportsRuntimeV2Filtering` as well, since they share the requirement. - Dropped `pushedPredicates()` from the trait; the fixture exposes its own accessor. - Test helper fails on an unexpected scan type rather than returning empty. - Grammar fix. The one thing I did not take is the longer `fullyPushedFilterAttributes` wording, discussed inline -- I think "fully evaluates" already carries the obligation and the expanded version is quite technical for a trait Javadoc. Happy to revisit if you disagree. -- 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]
