-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Core: Remove unused code for streaming position deletes #11175
Core: Remove unused code for streaming position deletes #11175
Conversation
@amogh-jahagirdar @szehon-ho @aokolnychyi please review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @wypoon but I think we should deprecate these APIs and remove at a future point (2.0) instead of removing outright.
Implement them the same way that the non-streaming versions are implemented so we can remove the classes that are no longer needed.
@amogh-jahagirdar I have restored the removed public static methods in iceberg/data/src/main/java/org/apache/iceberg/data/DeleteFilter.java Lines 248 to 260 in a3849bf
and iceberg/data/src/main/java/org/apache/iceberg/data/DeleteFilter.java Lines 262 to 267 in a3849bf
for reference. The same change is in #11177 except there the tests using Deletes.streamingFilter and Deletes.streamingMarker have not been removed, so you can see that the tests pass (for myself, I ran the tests locally). Since the objective is to remove code, I did not want to restore the tests here. #11177 is not intended for merging.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -110,155 +106,6 @@ public void testPositionMerging() { | |||
.containsExactly(0L, 3L, 3L, 9L, 16L, 19L, 19L, 22L, 22L, 56L, 63L, 70L, 91L); | |||
} | |||
|
|||
@Test | |||
public void testPositionStreamRowFilter() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to prefer not removing the tests until the deprecated method is actually removed from the code, but I think in this case it's fine since it's not a typical public API and rather a utility.
The Flink CI issues are unrelated to this change. All tests passed prior to my last commit, and the only change in the last commit was updating javadoc comments. |
Thanks @wypoon , merging! |
Follow up to #9117.
Prior to that PR, there were two code paths in
DeleteFilter
for applying position deletes: if the number of position deletes were below a certain number, we use thePositionDeleteIndex
bitmap and otherwise we use a streaming filter. That PR makes it so that we always use the first code path, but did not remove the now unused code for the second code path.This PR is to remove most of the unused code. It includes removing tests which were only added to test that second code path as well as bugs found in that code path (#8132). The
streamingFilter
andstreamingMarker
public methods inDeletes
are not removed but are deprecated; they can be removed in the version after 1.7. However, the implementation of the methods is replaced by the much simpler implementation using thePositionDeleteIndex
bitmap (first code path mentioned above) and the backing classes for the streaming implementation is removed.