[GitHub] [iceberg] nastra commented on a diff in pull request #7133: API: Fix retainAll and removeAll in CharSequenceSet

["nastra (via GitHub)" <gi...@apache.org>]

nastra commented on code in PR #7133:
URL: https://github.com/apache/iceberg/pull/7133#discussion_r1141856105


##########
api/src/test/java/org/apache/iceberg/util/TestCharSequenceSet.java:
##########
@@ -35,4 +37,33 @@ public void testSearchingInCharSequenceCollection() {
     // this would fail with a normal Set<CharSequence>
     Assertions.assertThat(set.contains("def")).isTrue();
   }
+
+  @Test
+  public void testRetainAll() {
+    CharSequenceSet set = CharSequenceSet.of(ImmutableList.of("123", "456"));
+
+    Assert.assertTrue("Set should be changed", set.retainAll(ImmutableList.of("456", "789", 123)));
+    Assert.assertTrue("Should not retain element \"123\"", set.size() == 1);
+    Assert.assertTrue("Should retain element \"456\"", set.contains("456"));

Review Comment:
   let's rewrite L46+L47 to `Assertions.assertThat(set).hasSize(1).contains("456")`. Same for all the places below.
   
   Also let's try and avoid assertTrue / assertFalse as much as possible as it doesn't provide enough context if a check ever fails (See https://github.com/apache/iceberg/pull/7131 for some background)



##########
api/src/main/java/org/apache/iceberg/util/CharSequenceSet.java:
##########
@@ -139,16 +139,26 @@ public boolean addAll(Collection<? extends CharSequence> charSequences) {
   @Override
   public boolean retainAll(Collection<?> objects) {
     if (objects != null) {
-      return Iterables.removeAll(wrapperSet, objects);
+      Set<CharSequenceWrapper> toRetain =
+          objects.stream()
+              .filter(CharSequence.class::isInstance)
+              .map(CharSequence.class::cast)
+              .map(CharSequenceWrapper::wrap)
+              .collect(Collectors.toSet());
+
+      return Iterables.retainAll(wrapperSet, toRetain);
     }
+
     return false;
   }
 
   @Override
+  @SuppressWarnings("CollectionUndefinedEquality")
   public boolean removeAll(Collection<?> objects) {
     if (objects != null) {
-      return Iterables.removeAll(wrapperSet, objects);
+      return objects.stream().filter(this::remove).count() != 0;

Review Comment:
   why not do the same as in L142ff?



-- 
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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org