Uh oh!
There was an error while loading. Please reload this page.
API: Align CharSequenceSet impl with Data/DeleteFileSet - #11322
Conversation
887f641 to
2b4592cCompare| import org.apache.iceberg.relocated.com.google.common.collect.Streams; | ||
| public class CharSequenceSet implements Set<CharSequence>, Serializable { | ||
| public class CharSequenceSet extends WrapperSet<CharSequence> { |
There was a problem hiding this comment.
might be easier to just look at the resulting file instead of the diff
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
2b4592c to
d4cc0bdComparehuaxingao
commented
Dec 10, 2025
@nastra Thanks for the PR, this is a nice cleanup and it is consistent with the goal of aligning |
nastra
commented
Dec 10, 2025
@huaxingao yes the behavioral changes are expected, since we want to align with the expectations of the |
| } | ||
| return false; | ||
| public static CharSequenceSet empty() { | ||
| return new CharSequenceSet(); |
There was a problem hiding this comment.
minor : I wonder if we should make this a static member
private static CharSequenceSet EMPTY = new CharSequenceSet()
public static CharSequenceSet empty() {
return EMPTY;
}
There was a problem hiding this comment.
we can't do that because empty() is expected to be a modifiable set and making this static will lead to reusing the same Set instance across different places. The equivalent in DataFileSet is create(), but here we historically have empty(). We could however deprecate that method and introduce create(), but this would cause a non-minor diff so I'd leave that to a separate PR
nastra
commented
Dec 10, 2025
thanks for the reviews @huaxingao and @singhpk234 |
Uh oh!
There was an error while loading. Please reload this page.
This aligns the
CharSequenceSetimplementation with the same behavior that we have forDataFileSetandDeleteFileSet.Most notably, the current impl of
CharSequenceSetisn't consistently handling nulls according to theSetAPI