[Spark] Fix stats with tightBounds check for AddFiles with deletionVectors #3633
Conversation
This file contains 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
c27kwan
approved these changes
Sep 2, 2024
| * | ||
| * This is abstract to force the implementers of all operations to think about this setting. | ||
| * All operations should add a comment justifying this setting. | ||
| * Any operation that sets this to false should add a test in TightBoundsSuite. |
There was a problem hiding this comment.
Is there a way to enforce the test? I feel like people might just cargo-cult the override + comment
There was a problem hiding this comment.
I am afraid that has to be up to good reviewing practice...
I have found examples of some tests that e.g. use reflection to check the number of fields in some class and then comment there what needs to be done if a new one is added.
I can imagine using reflection to find all subclasses of DeltaOperation, listing the ones that already define this as false and have a test, and instruct in that test that if you add a new subclass that overrides it to false then you should add a test, but I think it's a bit overkill...
bart-samwel
approved these changes
Sep 2, 2024
vkorukanti
pushed a commit
that referenced
this pull request
Sep 19, 2024
…letionVectorStatsAreNotTightBounds (#3641) Post merge review followup: update and fix some comments. Co-authored-by: Julek Sompolski <Juliusz Sompolski>
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 Delta project/connector is this regarding?
Description
Check
DeletionVectorFilesHaveWideBoundsCheckhas been disabled for COMPUTE STATS because it reintroduces stats with tight bound to files with Deletion Vectors. However, there are other operations that can then copy over these AddFile actions with DVs and tight stats. These operations resulted in DELTA_ADDING_DELETION_VECTORS_WITH_TIGHT_BOUNDS_DISALLOWED error, which was a false positive.In this PR we also attempt to introduce and discuss a "framework" for checks like that as a property of DeltaOperations, with DeltaOperations declaring as a member method whether a certain property and check should be performed. This is opposed to current practice, where many places in the code feature special cases like matching against a certain DeltaOperation and doing something special; this kind of code is very decentralized, and it's easy to miss if any new place or new operation needs such central handling. If this was centralized in DeltaOperations, this could lead to better discoverability of special cases and edge cases when implementing new operations.
How was this patch tested?
Tests added.
Does this PR introduce any user-facing changes?
No.