You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nastra (via GitHub)" <gi...@apache.org> on 2024/09/24 08:49:00 UTC
[PR] Core: Add ContentFileSet and ContentFileWrapper [iceberg]
nastra opened a new pull request, #11195:
URL: https://github.com/apache/iceberg/pull/11195
(no comment)
--
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
Re: [PR] Core: Add ContentFileSet and ContentFileWrapper [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11195:
URL: https://github.com/apache/iceberg/pull/11195#discussion_r1776041008
##########
api/src/main/java/org/apache/iceberg/util/ContentFileSet.java:
##########
@@ -0,0 +1,212 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.io.Serializable;
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterators;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+
+/**
+ * A custom set for {@link ContentFile} that maintains insertion order
+ *
+ * @param <F> the concrete Java class of a ContentFile instance
+ */
+public class ContentFileSet<F extends ContentFile<F>> implements Set<F>, Serializable {
Review Comment:
We may need this logic for other types of files like `ManifestFile` or `DeleteFileHolder`. The wrapping logic is not that unique. What do you think about doing something like this?
```
abstract class WrapperSet<T> implements Set<T> {
private final Set<Wrapper<T>> wrappers;
protected WrapperSet(Iterable<Wrapper<T>> wrappers) {
this.wrappers = Sets.newLinkedHashSet(wrappers);
}
abstract Wrapper<T> wrapper();
abstract Wrapper<T> wrap(T file);
// common implementation without supporting NULL elements and objects of types other than T
}
```
Then we can have trivial `DeleteFileSet` and `DataFileSet` classes.
```
public class DeleteFileSet extends WrapperSet<DeleteFile> {
private static final ThreadLocal<Wrapper<DeleteFile>> WRAPPERS =
ThreadLocal.withInitial(() -> DeleteFileWrapper.wrap(null));
private DeleteFileSet(Iterable<Wrapper<DeleteFile>> wrappers) {
super(wrappers);
}
public static DeleteFileSet create() {
return new DeleteFileSet(...);
}
public static DeleteFileSet of(Iterable<? extends DeleteFile> files) {
return new DeleteFileSet(...);
}
@Override
Wrapper<DeleteFile> wrapper() {
return WRAPPERS.get();
}
@Override
Wrapper<DeleteFile> wrap(DeleteFile file) {
return new DeleteFileWrapper(file);
}
private static class DeleteFileWrapper implements Wrapper<DeleteFile> {
private DeleteFile file;
private DeleteFileWrapper(DeleteFile file) {
this.file = file;
}
// equals and hashCode
}
}
```
--
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
Re: [PR] Core: Add DataFileSet / DeleteFileSet [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11195:
URL: https://github.com/apache/iceberg/pull/11195#discussion_r1778251131
##########
api/src/main/java/org/apache/iceberg/util/WrapperSet.java:
##########
@@ -0,0 +1,185 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.io.Serializable;
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterators;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+
+/**
+ * A custom set for a {@link Wrapper} of the given type that maintains insertion order
+ *
+ * @param <T> The type to wrap in a {@link Wrapper} instance.
+ */
+abstract class WrapperSet<T> implements Set<T>, Serializable {
+ private final Set<Wrapper<T>> set;
+
+ protected WrapperSet(Iterable<Wrapper<T>> wrappers) {
+ this.set = Sets.newLinkedHashSet(wrappers);
+ }
+
+ protected abstract Wrapper<T> wrapper();
+
+ protected abstract Wrapper<T> wrap(T file);
+
+ protected abstract boolean isInstance(Object obj);
+
+ protected interface Wrapper<T> {
+ T get();
+
+ Wrapper<T> set(T object);
+ }
+
+ @Override
+ public int size() {
+ return set.size();
+ }
+
+ @Override
+ public boolean isEmpty() {
+ return set.isEmpty();
+ }
+
+ @SuppressWarnings("unchecked")
+ @Override
+ public boolean contains(Object obj) {
+ if (isInstance(obj)) {
+ Wrapper<T> wrapper = wrapper();
+ boolean result = set.contains(wrapper.set((T) obj));
+ wrapper.set(null); // don't hold a reference to the value
+ return result;
+ }
+
+ return false;
+ }
+
+ @Override
+ public Iterator<T> iterator() {
+ return Iterators.transform(set.iterator(), Wrapper::get);
+ }
+
+ @Override
+ public Object[] toArray() {
+ return Lists.newArrayList(iterator()).toArray();
+ }
+
+ @Override
+ public <X> X[] toArray(X[] destArray) {
+ return Lists.newArrayList(iterator()).toArray(destArray);
+ }
+
+ @Override
+ public boolean add(T obj) {
+ return set.add(wrap(obj));
Review Comment:
you're right and I don't think we want to support nulls inside the set. I've updated the impl to throw a NPE when adding/removing nulls or when a null collection is passed. I was initially aligning the impl fully with `CharSequenceSet` but `CharSequenceSet` has the same inconsistency issue
--
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
Re: [PR] Core: Add DataFileSet / DeleteFileSet [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11195:
URL: https://github.com/apache/iceberg/pull/11195#discussion_r1778454236
##########
api/src/main/java/org/apache/iceberg/util/DataFileSet.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.util.Objects;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.types.Comparators;
+
+public class DataFileSet extends WrapperSet<DataFile> {
Review Comment:
good point, I've added that and realized that Kryo ser/de wasn't working for `DataFileSet`/ `DeleteFileSet`. The same is actually true for `CharSequenceSet`. I will follow-up with the `CharSequenceSet` inconsistencies in a separate PR
--
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
Re: [PR] Core: Add DataFileSet / DelteFileSet [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11195:
URL: https://github.com/apache/iceberg/pull/11195#discussion_r1777108131
##########
api/src/main/java/org/apache/iceberg/util/ContentFileSet.java:
##########
@@ -0,0 +1,212 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.io.Serializable;
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterators;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+
+/**
+ * A custom set for {@link ContentFile} that maintains insertion order
+ *
+ * @param <F> the concrete Java class of a ContentFile instance
+ */
+public class ContentFileSet<F extends ContentFile<F>> implements Set<F>, Serializable {
Review Comment:
@aokolnychyi I was assuming that we'd only have `ContentFile` in a set and thus made the Set use a generic type but if we're going to use this set for other types as well it makes sense to generalize the wrapper set.
I've updated the PR and added `DataFileSet` / `DeleteFileSet`
--
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
Re: [PR] Core: Add DataFileSet / DeleteFileSet [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11195:
URL: https://github.com/apache/iceberg/pull/11195#discussion_r1778100059
##########
api/src/main/java/org/apache/iceberg/util/DataFileSet.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.util.Objects;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.types.Comparators;
+
+public class DataFileSet extends WrapperSet<DataFile> {
+ private static final ThreadLocal<DataFileWrapper> WRAPPERS =
+ ThreadLocal.withInitial(() -> DataFileWrapper.wrap(null));
+
+ protected DataFileSet(Iterable<Wrapper<DataFile>> wrappers) {
+ super(wrappers);
+ }
+
+ public static DataFileSet empty() {
+ return new DataFileSet(Sets.newLinkedHashSet());
Review Comment:
I'm fine either way but I named it `empty()` to be consistent with `CharSequenceSet`. However, looking at `CharSequenceMap` it uses `create()` so let's go with `create()` then
##########
api/src/main/java/org/apache/iceberg/util/DataFileSet.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.util.Objects;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.types.Comparators;
+
+public class DataFileSet extends WrapperSet<DataFile> {
+ private static final ThreadLocal<DataFileWrapper> WRAPPERS =
+ ThreadLocal.withInitial(() -> DataFileWrapper.wrap(null));
+
+ protected DataFileSet(Iterable<Wrapper<DataFile>> wrappers) {
+ super(wrappers);
+ }
+
+ public static DataFileSet empty() {
+ return new DataFileSet(Sets.newLinkedHashSet());
Review Comment:
yep we could also just pass `Collections.emptySet()` here
--
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
Re: [PR] Core: Add ContentFileSet and ContentFileWrapper [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #11195:
URL: https://github.com/apache/iceberg/pull/11195#issuecomment-2372638766
Will take a look tomorrow. Thanks, @nastra!
--
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
Re: [PR] Core: Add DataFileSet / DeleteFileSet [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11195:
URL: https://github.com/apache/iceberg/pull/11195#discussion_r1779304973
##########
api/src/main/java/org/apache/iceberg/util/WrapperSet.java:
##########
@@ -0,0 +1,185 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.io.Serializable;
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterators;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+
+/**
+ * A custom set for a {@link Wrapper} of the given type that maintains insertion order
+ *
+ * @param <T> The type to wrap in a {@link Wrapper} instance.
+ */
+abstract class WrapperSet<T> implements Set<T>, Serializable {
+ private final Set<Wrapper<T>> set;
+
+ protected WrapperSet(Iterable<Wrapper<T>> wrappers) {
+ this.set = Sets.newLinkedHashSet(wrappers);
+ }
+
+ protected abstract Wrapper<T> wrapper();
+
+ protected abstract Wrapper<T> wrap(T file);
+
+ protected abstract boolean isInstance(Object obj);
+
+ protected interface Wrapper<T> {
Review Comment:
Makes sense.
--
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
Re: [PR] Core: Add DataFileSet / DeleteFileSet [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11195:
URL: https://github.com/apache/iceberg/pull/11195#discussion_r1777123356
##########
core/src/test/java/org/apache/iceberg/util/TestDataFileSet.java:
##########
@@ -0,0 +1,209 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Collections;
+import java.util.Set;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.DataFiles;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Testing {@link DataFileSet} is easier in iceberg-core since the data file builders are located
+ * here
+ */
+public class TestDataFileSet {
Review Comment:
I think for now it's probably fine to keep two separate test classes and not introduce a generic test class with subclasses for testing (we can do that once we have more subclasses of `WrapperSet`)
--
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
Re: [PR] Core: Add DataFileSet / DeleteFileSet [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11195:
URL: https://github.com/apache/iceberg/pull/11195#discussion_r1778254044
##########
api/src/main/java/org/apache/iceberg/util/DataFileSet.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.util.Objects;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.types.Comparators;
+
+public class DataFileSet extends WrapperSet<DataFile> {
+ private static final ThreadLocal<DataFileWrapper> WRAPPERS =
+ ThreadLocal.withInitial(() -> DataFileWrapper.wrap(null));
+
+ protected DataFileSet(Iterable<Wrapper<DataFile>> wrappers) {
+ super(wrappers);
+ }
+
+ public static DataFileSet empty() {
+ return new DataFileSet(Sets.newLinkedHashSet());
+ }
+
+ public static DataFileSet of(Iterable<DataFile> iterable) {
+ return new DataFileSet(
+ Sets.newLinkedHashSet(Iterables.transform(iterable, DataFileWrapper::wrap)));
+ }
+
+ @Override
+ protected Wrapper<DataFile> wrapper() {
+ return WRAPPERS.get();
+ }
+
+ @Override
+ protected Wrapper<DataFile> wrap(DataFile dataFile) {
+ return DataFileWrapper.wrap(dataFile);
+ }
+
+ @Override
+ protected boolean isInstance(Object obj) {
+ return obj instanceof DataFile;
+ }
+
+ private static class DataFileWrapper implements Wrapper<DataFile> {
+ private DataFile file;
+
+ private DataFileWrapper(DataFile file) {
+ this.file = file;
+ }
+
+ private static DataFileWrapper wrap(DataFile dataFile) {
+ return new DataFileWrapper(dataFile);
+ }
+
+ @Override
+ public DataFile get() {
+ return file;
+ }
+
+ @Override
+ public Wrapper<DataFile> set(DataFile dataFile) {
+ this.file = dataFile;
+ return this;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+
+ if (!(o instanceof DataFileWrapper)) {
+ return false;
+ }
+
+ DataFileWrapper that = (DataFileWrapper) o;
+ if (null == file && null == that.file) {
Review Comment:
I don't think we should allow nulls in the set itself, but the Wrapper's value could be `null` and so I think it's better to be defensive here (even though we're fully controlling the impl), wdyt?
##########
api/src/main/java/org/apache/iceberg/util/DataFileSet.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.util.Objects;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.types.Comparators;
+
+public class DataFileSet extends WrapperSet<DataFile> {
+ private static final ThreadLocal<DataFileWrapper> WRAPPERS =
+ ThreadLocal.withInitial(() -> DataFileWrapper.wrap(null));
+
+ protected DataFileSet(Iterable<Wrapper<DataFile>> wrappers) {
+ super(wrappers);
+ }
+
+ public static DataFileSet empty() {
+ return new DataFileSet(Sets.newLinkedHashSet());
+ }
+
+ public static DataFileSet of(Iterable<DataFile> iterable) {
+ return new DataFileSet(
+ Sets.newLinkedHashSet(Iterables.transform(iterable, DataFileWrapper::wrap)));
+ }
+
+ @Override
+ protected Wrapper<DataFile> wrapper() {
+ return WRAPPERS.get();
+ }
+
+ @Override
+ protected Wrapper<DataFile> wrap(DataFile dataFile) {
+ return DataFileWrapper.wrap(dataFile);
+ }
+
+ @Override
+ protected boolean isInstance(Object obj) {
+ return obj instanceof DataFile;
+ }
+
+ private static class DataFileWrapper implements Wrapper<DataFile> {
+ private DataFile file;
+
+ private DataFileWrapper(DataFile file) {
+ this.file = file;
+ }
+
+ private static DataFileWrapper wrap(DataFile dataFile) {
+ return new DataFileWrapper(dataFile);
+ }
+
+ @Override
+ public DataFile get() {
+ return file;
+ }
+
+ @Override
+ public Wrapper<DataFile> set(DataFile dataFile) {
+ this.file = dataFile;
+ return this;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+
+ if (!(o instanceof DataFileWrapper)) {
+ return false;
+ }
+
+ DataFileWrapper that = (DataFileWrapper) o;
+ if (null == file && null == that.file) {
Review Comment:
I don't think we should allow nulls in the set itself, but the Wrapper's value could be `null` and so I think it's better to be defensive here, wdyt?
--
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
Re: [PR] Core: Add DataFileSet / DeleteFileSet [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11195:
URL: https://github.com/apache/iceberg/pull/11195#discussion_r1778103857
##########
api/src/main/java/org/apache/iceberg/util/WrapperSet.java:
##########
@@ -0,0 +1,185 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.io.Serializable;
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterators;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+
+/**
+ * A custom set for a {@link Wrapper} of the given type that maintains insertion order
+ *
+ * @param <T> The type to wrap in a {@link Wrapper} instance.
+ */
+abstract class WrapperSet<T> implements Set<T>, Serializable {
+ private final Set<Wrapper<T>> set;
+
+ protected WrapperSet(Iterable<Wrapper<T>> wrappers) {
+ this.set = Sets.newLinkedHashSet(wrappers);
+ }
+
+ protected abstract Wrapper<T> wrapper();
+
+ protected abstract Wrapper<T> wrap(T file);
+
+ protected abstract boolean isInstance(Object obj);
+
+ protected interface Wrapper<T> {
Review Comment:
yes this was put here on purpose as it's easier to spot/see what exactly needs to be implemented.
--
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
Re: [PR] Core: Add DataFileSet / DeleteFileSet [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11195:
URL: https://github.com/apache/iceberg/pull/11195#discussion_r1778254044
##########
api/src/main/java/org/apache/iceberg/util/DataFileSet.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.util.Objects;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.types.Comparators;
+
+public class DataFileSet extends WrapperSet<DataFile> {
+ private static final ThreadLocal<DataFileWrapper> WRAPPERS =
+ ThreadLocal.withInitial(() -> DataFileWrapper.wrap(null));
+
+ protected DataFileSet(Iterable<Wrapper<DataFile>> wrappers) {
+ super(wrappers);
+ }
+
+ public static DataFileSet empty() {
+ return new DataFileSet(Sets.newLinkedHashSet());
+ }
+
+ public static DataFileSet of(Iterable<DataFile> iterable) {
+ return new DataFileSet(
+ Sets.newLinkedHashSet(Iterables.transform(iterable, DataFileWrapper::wrap)));
+ }
+
+ @Override
+ protected Wrapper<DataFile> wrapper() {
+ return WRAPPERS.get();
+ }
+
+ @Override
+ protected Wrapper<DataFile> wrap(DataFile dataFile) {
+ return DataFileWrapper.wrap(dataFile);
+ }
+
+ @Override
+ protected boolean isInstance(Object obj) {
+ return obj instanceof DataFile;
+ }
+
+ private static class DataFileWrapper implements Wrapper<DataFile> {
+ private DataFile file;
+
+ private DataFileWrapper(DataFile file) {
+ this.file = file;
+ }
+
+ private static DataFileWrapper wrap(DataFile dataFile) {
+ return new DataFileWrapper(dataFile);
+ }
+
+ @Override
+ public DataFile get() {
+ return file;
+ }
+
+ @Override
+ public Wrapper<DataFile> set(DataFile dataFile) {
+ this.file = dataFile;
+ return this;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+
+ if (!(o instanceof DataFileWrapper)) {
+ return false;
+ }
+
+ DataFileWrapper that = (DataFileWrapper) o;
+ if (null == file && null == that.file) {
Review Comment:
I don't think we should allow nulls in the set itself, but the Wrapper's value could be `null` and so it's better to be defensive here
--
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
Re: [PR] Core: Add DataFileSet / DeleteFileSet [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11195:
URL: https://github.com/apache/iceberg/pull/11195#discussion_r1778689365
##########
api/src/main/java/org/apache/iceberg/util/DataFileSet.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.util.Objects;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.types.Comparators;
+
+public class DataFileSet extends WrapperSet<DataFile> {
+ private static final ThreadLocal<DataFileWrapper> WRAPPERS =
+ ThreadLocal.withInitial(() -> DataFileWrapper.wrap(null));
+
+ protected DataFileSet(Iterable<Wrapper<DataFile>> wrappers) {
+ super(wrappers);
+ }
+
+ public static DataFileSet empty() {
+ return new DataFileSet(Sets.newLinkedHashSet());
Review Comment:
also yes, we could also just pass `Collections.emptySet()` here
--
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
Re: [PR] Core: Add DataFileSet / DeleteFileSet [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11195:
URL: https://github.com/apache/iceberg/pull/11195#discussion_r1778103857
##########
api/src/main/java/org/apache/iceberg/util/WrapperSet.java:
##########
@@ -0,0 +1,185 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.io.Serializable;
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterators;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+
+/**
+ * A custom set for a {@link Wrapper} of the given type that maintains insertion order
+ *
+ * @param <T> The type to wrap in a {@link Wrapper} instance.
+ */
+abstract class WrapperSet<T> implements Set<T>, Serializable {
+ private final Set<Wrapper<T>> set;
+
+ protected WrapperSet(Iterable<Wrapper<T>> wrappers) {
+ this.set = Sets.newLinkedHashSet(wrappers);
+ }
+
+ protected abstract Wrapper<T> wrapper();
+
+ protected abstract Wrapper<T> wrap(T file);
+
+ protected abstract boolean isInstance(Object obj);
+
+ protected interface Wrapper<T> {
Review Comment:
yes this was put here on purpose as it's easier to spot/see what exactly needs to be implemented. But I'm also fine moving this to the bottom if you think that's better here
--
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
Re: [PR] Core: Add DataFileSet / DeleteFileSet [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11195:
URL: https://github.com/apache/iceberg/pull/11195#discussion_r1777999151
##########
api/src/main/java/org/apache/iceberg/util/WrapperSet.java:
##########
@@ -0,0 +1,185 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.io.Serializable;
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterators;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+
+/**
+ * A custom set for a {@link Wrapper} of the given type that maintains insertion order
+ *
+ * @param <T> The type to wrap in a {@link Wrapper} instance.
+ */
+abstract class WrapperSet<T> implements Set<T>, Serializable {
Review Comment:
I'd consider implementing `Serializable` in each child separately as children won't be necessarily serializable.
##########
api/src/main/java/org/apache/iceberg/util/WrapperSet.java:
##########
@@ -0,0 +1,185 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.io.Serializable;
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterators;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+
+/**
+ * A custom set for a {@link Wrapper} of the given type that maintains insertion order
+ *
+ * @param <T> The type to wrap in a {@link Wrapper} instance.
+ */
+abstract class WrapperSet<T> implements Set<T>, Serializable {
+ private final Set<Wrapper<T>> set;
+
+ protected WrapperSet(Iterable<Wrapper<T>> wrappers) {
+ this.set = Sets.newLinkedHashSet(wrappers);
+ }
+
+ protected abstract Wrapper<T> wrapper();
+
+ protected abstract Wrapper<T> wrap(T file);
+
+ protected abstract boolean isInstance(Object obj);
+
+ protected interface Wrapper<T> {
Review Comment:
Question: We usually have static classes/interfaces at the bottom. Do we put it here to co-locate with the rest of APIs that have to be implemented? Just making sure it is intentional.
##########
api/src/main/java/org/apache/iceberg/util/DataFileSet.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.util.Objects;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.types.Comparators;
+
+public class DataFileSet extends WrapperSet<DataFile> {
+ private static final ThreadLocal<DataFileWrapper> WRAPPERS =
+ ThreadLocal.withInitial(() -> DataFileWrapper.wrap(null));
+
+ protected DataFileSet(Iterable<Wrapper<DataFile>> wrappers) {
+ super(wrappers);
+ }
+
+ public static DataFileSet empty() {
+ return new DataFileSet(Sets.newLinkedHashSet());
+ }
+
+ public static DataFileSet of(Iterable<DataFile> iterable) {
+ return new DataFileSet(
+ Sets.newLinkedHashSet(Iterables.transform(iterable, DataFileWrapper::wrap)));
+ }
+
+ @Override
+ protected Wrapper<DataFile> wrapper() {
+ return WRAPPERS.get();
+ }
+
+ @Override
+ protected Wrapper<DataFile> wrap(DataFile dataFile) {
+ return DataFileWrapper.wrap(dataFile);
+ }
+
+ @Override
+ protected boolean isInstance(Object obj) {
+ return obj instanceof DataFile;
+ }
+
+ private static class DataFileWrapper implements Wrapper<DataFile> {
+ private DataFile file;
+
+ private DataFileWrapper(DataFile file) {
+ this.file = file;
+ }
+
+ private static DataFileWrapper wrap(DataFile dataFile) {
+ return new DataFileWrapper(dataFile);
+ }
+
+ @Override
+ public DataFile get() {
+ return file;
+ }
+
+ @Override
+ public Wrapper<DataFile> set(DataFile dataFile) {
+ this.file = dataFile;
+ return this;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+
+ if (!(o instanceof DataFileWrapper)) {
+ return false;
+ }
+
+ DataFileWrapper that = (DataFileWrapper) o;
+ if (null == file && null == that.file) {
+ return true;
+ }
+
+ if (null == file || null == that.file) {
+ return false;
+ }
+
+ return 0 == Comparators.charSequences().compare(file.location(), that.file.location());
Review Comment:
Locations are Strings, can we simply use `Objects.equals()`?
##########
api/src/main/java/org/apache/iceberg/util/DataFileSet.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.util.Objects;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.types.Comparators;
+
+public class DataFileSet extends WrapperSet<DataFile> {
+ private static final ThreadLocal<DataFileWrapper> WRAPPERS =
+ ThreadLocal.withInitial(() -> DataFileWrapper.wrap(null));
+
+ protected DataFileSet(Iterable<Wrapper<DataFile>> wrappers) {
+ super(wrappers);
+ }
+
+ public static DataFileSet empty() {
+ return new DataFileSet(Sets.newLinkedHashSet());
+ }
+
+ public static DataFileSet of(Iterable<DataFile> iterable) {
+ return new DataFileSet(
+ Sets.newLinkedHashSet(Iterables.transform(iterable, DataFileWrapper::wrap)));
Review Comment:
Doesn't the parent materialize it as `Set`? Should be enough to just call `transform`.
##########
api/src/main/java/org/apache/iceberg/util/DeleteFileSet.java:
##########
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.util.Objects;
+import org.apache.iceberg.DeleteFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.types.Comparators;
+
+public class DeleteFileSet extends WrapperSet<DeleteFile> {
Review Comment:
Similar comments apply here.
##########
api/src/main/java/org/apache/iceberg/util/WrapperSet.java:
##########
@@ -0,0 +1,185 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.io.Serializable;
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterators;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+
+/**
+ * A custom set for a {@link Wrapper} of the given type that maintains insertion order
Review Comment:
Minor: Do we need `.` at the end?
##########
api/src/main/java/org/apache/iceberg/util/DataFileSet.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.util.Objects;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.types.Comparators;
+
+public class DataFileSet extends WrapperSet<DataFile> {
+ private static final ThreadLocal<DataFileWrapper> WRAPPERS =
+ ThreadLocal.withInitial(() -> DataFileWrapper.wrap(null));
+
+ protected DataFileSet(Iterable<Wrapper<DataFile>> wrappers) {
+ super(wrappers);
+ }
+
+ public static DataFileSet empty() {
+ return new DataFileSet(Sets.newLinkedHashSet());
+ }
+
+ public static DataFileSet of(Iterable<DataFile> iterable) {
Review Comment:
What about `Iterable<? extends DataFile>` so that we can pass any subclasses?
##########
api/src/main/java/org/apache/iceberg/util/WrapperSet.java:
##########
@@ -0,0 +1,185 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.io.Serializable;
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterators;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+
+/**
+ * A custom set for a {@link Wrapper} of the given type that maintains insertion order
+ *
+ * @param <T> The type to wrap in a {@link Wrapper} instance.
+ */
+abstract class WrapperSet<T> implements Set<T>, Serializable {
+ private final Set<Wrapper<T>> set;
+
+ protected WrapperSet(Iterable<Wrapper<T>> wrappers) {
+ this.set = Sets.newLinkedHashSet(wrappers);
+ }
+
+ protected abstract Wrapper<T> wrapper();
+
+ protected abstract Wrapper<T> wrap(T file);
+
+ protected abstract boolean isInstance(Object obj);
Review Comment:
I would either call it like `canStore` to make the name more generic (to allow some sets support null values, for instance) or remove this method and always cast. The `Set` API allows `ClassCastException` in all methods where we currently invoke this one.
```
ClassCastException – if the class of an element of this set is incompatible with the specified collection (optional)
```
##########
api/src/main/java/org/apache/iceberg/util/DataFileSet.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.util.Objects;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.types.Comparators;
+
+public class DataFileSet extends WrapperSet<DataFile> {
+ private static final ThreadLocal<DataFileWrapper> WRAPPERS =
+ ThreadLocal.withInitial(() -> DataFileWrapper.wrap(null));
+
+ protected DataFileSet(Iterable<Wrapper<DataFile>> wrappers) {
+ super(wrappers);
+ }
+
+ public static DataFileSet empty() {
+ return new DataFileSet(Sets.newLinkedHashSet());
+ }
+
+ public static DataFileSet of(Iterable<DataFile> iterable) {
+ return new DataFileSet(
+ Sets.newLinkedHashSet(Iterables.transform(iterable, DataFileWrapper::wrap)));
+ }
+
+ @Override
+ protected Wrapper<DataFile> wrapper() {
+ return WRAPPERS.get();
+ }
+
+ @Override
+ protected Wrapper<DataFile> wrap(DataFile dataFile) {
+ return DataFileWrapper.wrap(dataFile);
+ }
+
+ @Override
+ protected boolean isInstance(Object obj) {
+ return obj instanceof DataFile;
Review Comment:
Careful with nulls.
##########
api/src/main/java/org/apache/iceberg/util/DataFileSet.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.util.Objects;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.types.Comparators;
+
+public class DataFileSet extends WrapperSet<DataFile> {
+ private static final ThreadLocal<DataFileWrapper> WRAPPERS =
+ ThreadLocal.withInitial(() -> DataFileWrapper.wrap(null));
+
+ protected DataFileSet(Iterable<Wrapper<DataFile>> wrappers) {
+ super(wrappers);
+ }
+
+ public static DataFileSet empty() {
+ return new DataFileSet(Sets.newLinkedHashSet());
Review Comment:
This can be any iterable too as the parent always creates a set.
##########
api/src/main/java/org/apache/iceberg/util/WrapperSet.java:
##########
@@ -0,0 +1,185 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.io.Serializable;
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterators;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+
+/**
+ * A custom set for a {@link Wrapper} of the given type that maintains insertion order
+ *
+ * @param <T> The type to wrap in a {@link Wrapper} instance.
+ */
+abstract class WrapperSet<T> implements Set<T>, Serializable {
+ private final Set<Wrapper<T>> set;
+
+ protected WrapperSet(Iterable<Wrapper<T>> wrappers) {
+ this.set = Sets.newLinkedHashSet(wrappers);
+ }
+
+ protected abstract Wrapper<T> wrapper();
+
+ protected abstract Wrapper<T> wrap(T file);
+
+ protected abstract boolean isInstance(Object obj);
+
+ protected interface Wrapper<T> {
+ T get();
+
+ Wrapper<T> set(T object);
+ }
+
+ @Override
+ public int size() {
+ return set.size();
+ }
+
+ @Override
+ public boolean isEmpty() {
+ return set.isEmpty();
+ }
+
+ @SuppressWarnings("unchecked")
+ @Override
+ public boolean contains(Object obj) {
+ if (isInstance(obj)) {
+ Wrapper<T> wrapper = wrapper();
+ boolean result = set.contains(wrapper.set((T) obj));
+ wrapper.set(null); // don't hold a reference to the value
+ return result;
+ }
+
+ return false;
+ }
+
+ @Override
+ public Iterator<T> iterator() {
+ return Iterators.transform(set.iterator(), Wrapper::get);
+ }
+
+ @Override
+ public Object[] toArray() {
+ return Lists.newArrayList(iterator()).toArray();
+ }
+
+ @Override
+ public <X> X[] toArray(X[] destArray) {
+ return Lists.newArrayList(iterator()).toArray(destArray);
+ }
+
+ @Override
+ public boolean add(T obj) {
+ return set.add(wrap(obj));
Review Comment:
I suspect that we can add `null` but not retrieve it back. It is up to us whether we want to support nulls but it has to be consistent. It is valid to throw `NullPointerException` if the set does not support nulls.
##########
api/src/main/java/org/apache/iceberg/util/DataFileSet.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.util.Objects;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.types.Comparators;
+
+public class DataFileSet extends WrapperSet<DataFile> {
+ private static final ThreadLocal<DataFileWrapper> WRAPPERS =
+ ThreadLocal.withInitial(() -> DataFileWrapper.wrap(null));
+
+ protected DataFileSet(Iterable<Wrapper<DataFile>> wrappers) {
+ super(wrappers);
+ }
+
+ public static DataFileSet empty() {
Review Comment:
What about calling it `create()` as the returned set is mutable?
##########
api/src/main/java/org/apache/iceberg/util/DataFileSet.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.util.Objects;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.types.Comparators;
+
+public class DataFileSet extends WrapperSet<DataFile> {
+ private static final ThreadLocal<DataFileWrapper> WRAPPERS =
+ ThreadLocal.withInitial(() -> DataFileWrapper.wrap(null));
+
+ protected DataFileSet(Iterable<Wrapper<DataFile>> wrappers) {
+ super(wrappers);
+ }
+
+ public static DataFileSet empty() {
+ return new DataFileSet(Sets.newLinkedHashSet());
+ }
+
+ public static DataFileSet of(Iterable<DataFile> iterable) {
+ return new DataFileSet(
+ Sets.newLinkedHashSet(Iterables.transform(iterable, DataFileWrapper::wrap)));
+ }
+
+ @Override
+ protected Wrapper<DataFile> wrapper() {
+ return WRAPPERS.get();
+ }
+
+ @Override
+ protected Wrapper<DataFile> wrap(DataFile dataFile) {
+ return DataFileWrapper.wrap(dataFile);
+ }
+
+ @Override
+ protected boolean isInstance(Object obj) {
+ return obj instanceof DataFile;
+ }
+
+ private static class DataFileWrapper implements Wrapper<DataFile> {
+ private DataFile file;
+
+ private DataFileWrapper(DataFile file) {
+ this.file = file;
+ }
+
+ private static DataFileWrapper wrap(DataFile dataFile) {
+ return new DataFileWrapper(dataFile);
+ }
+
+ @Override
+ public DataFile get() {
+ return file;
+ }
+
+ @Override
+ public Wrapper<DataFile> set(DataFile dataFile) {
+ this.file = dataFile;
+ return this;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+
+ if (!(o instanceof DataFileWrapper)) {
+ return false;
+ }
+
+ DataFileWrapper that = (DataFileWrapper) o;
+ if (null == file && null == that.file) {
Review Comment:
I wonder whether we want to allow null files in this set. Any thoughts?
##########
api/src/main/java/org/apache/iceberg/util/DataFileSet.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.util.Objects;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.types.Comparators;
+
+public class DataFileSet extends WrapperSet<DataFile> {
Review Comment:
Can we, please, add tests that these sets are serializable using both Java and Kryo serialization? We will use them in a Spark broadcast later. We already have `TestDataFileSerialization` and similar classes.
##########
core/src/test/java/org/apache/iceberg/util/TestDataFileSet.java:
##########
@@ -0,0 +1,209 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Collections;
+import java.util.Set;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.DataFiles;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Testing {@link DataFileSet} is easier in iceberg-core since the data file builders are located
+ * here
+ */
+public class TestDataFileSet {
Review Comment:
Yeah, I wouldn't complicate it too much.
##########
api/src/main/java/org/apache/iceberg/util/WrapperSet.java:
##########
@@ -0,0 +1,185 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.io.Serializable;
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterators;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+
+/**
+ * A custom set for a {@link Wrapper} of the given type that maintains insertion order
+ *
+ * @param <T> The type to wrap in a {@link Wrapper} instance.
+ */
+abstract class WrapperSet<T> implements Set<T>, Serializable {
+ private final Set<Wrapper<T>> set;
+
+ protected WrapperSet(Iterable<Wrapper<T>> wrappers) {
+ this.set = Sets.newLinkedHashSet(wrappers);
+ }
+
+ protected abstract Wrapper<T> wrapper();
+
+ protected abstract Wrapper<T> wrap(T file);
+
+ protected abstract boolean isInstance(Object obj);
Review Comment:
One more alternative is to have `abstract Class<T> elementClass();` to rely on `isInstance` and use various utils like `Iterators.toArray` that accept `Class`. Up to you.
--
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
Re: [PR] Core: Add DataFileSet / DeleteFileSet [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11195:
URL: https://github.com/apache/iceberg/pull/11195#discussion_r1778090237
##########
api/src/main/java/org/apache/iceberg/util/WrapperSet.java:
##########
@@ -0,0 +1,185 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.io.Serializable;
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterators;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+
+/**
+ * A custom set for a {@link Wrapper} of the given type that maintains insertion order
Review Comment:
I think we're inconsistent across the codebase but I've added . at the end
--
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
Re: [PR] Core: Add DataFileSet / DeleteFileSet [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11195:
URL: https://github.com/apache/iceberg/pull/11195#discussion_r1778100059
##########
api/src/main/java/org/apache/iceberg/util/DataFileSet.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.util.Objects;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.types.Comparators;
+
+public class DataFileSet extends WrapperSet<DataFile> {
+ private static final ThreadLocal<DataFileWrapper> WRAPPERS =
+ ThreadLocal.withInitial(() -> DataFileWrapper.wrap(null));
+
+ protected DataFileSet(Iterable<Wrapper<DataFile>> wrappers) {
+ super(wrappers);
+ }
+
+ public static DataFileSet empty() {
+ return new DataFileSet(Sets.newLinkedHashSet());
Review Comment:
I'm fine either way but I named it `empty()` to be consistent with `CharSequenceSet`. However, looking at `CharSequenceMap` it uses `create()` so let's go with `create()` then
--
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
Re: [PR] Core: Add DataFileSet / DeleteFileSet [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11195:
URL: https://github.com/apache/iceberg/pull/11195#discussion_r1778263784
##########
api/src/main/java/org/apache/iceberg/util/WrapperSet.java:
##########
@@ -0,0 +1,185 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.io.Serializable;
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterators;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+
+/**
+ * A custom set for a {@link Wrapper} of the given type that maintains insertion order
+ *
+ * @param <T> The type to wrap in a {@link Wrapper} instance.
+ */
+abstract class WrapperSet<T> implements Set<T>, Serializable {
+ private final Set<Wrapper<T>> set;
+
+ protected WrapperSet(Iterable<Wrapper<T>> wrappers) {
+ this.set = Sets.newLinkedHashSet(wrappers);
+ }
+
+ protected abstract Wrapper<T> wrapper();
+
+ protected abstract Wrapper<T> wrap(T file);
+
+ protected abstract boolean isInstance(Object obj);
Review Comment:
yeah this is another place where I wanted to be fully consistent with `CharSequenceSet` which also doesn't throw a CCE. I think I'd prefer using `elementClass()` here since that gives us slightly more flexibility
--
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
Re: [PR] Core: Add DataFileSet / DeleteFileSet [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11195:
URL: https://github.com/apache/iceberg/pull/11195#discussion_r1778454236
##########
api/src/main/java/org/apache/iceberg/util/DataFileSet.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.util.Objects;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.types.Comparators;
+
+public class DataFileSet extends WrapperSet<DataFile> {
Review Comment:
good point, I've added that and realized that Kryo ser/de wasn't working for `DataFileSet`/ `DeleteFileSet`. The same is actually true for `CharSequenceSet`. I will follow-up with the `CharSequenceSet` impl in a separate PR
--
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
Re: [PR] Core: Add DataFileSet / DeleteFileSet [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11195:
URL: https://github.com/apache/iceberg/pull/11195#discussion_r1779311673
##########
api/src/main/java/org/apache/iceberg/util/DataFileSet.java:
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.io.Serializable;
+import java.util.Objects;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+
+public class DataFileSet extends WrapperSet<DataFile> implements Serializable {
+ private static final ThreadLocal<DataFileWrapper> WRAPPERS =
+ ThreadLocal.withInitial(() -> DataFileWrapper.wrap(null));
+
+ private DataFileSet() {
+ // needed for serialization/deserialization
+ }
+
+ private DataFileSet(Iterable<Wrapper<DataFile>> wrappers) {
+ super(wrappers);
+ }
+
+ public static DataFileSet create() {
+ return new DataFileSet();
+ }
+
+ public static DataFileSet of(Iterable<? extends DataFile> iterable) {
+ return new DataFileSet(
+ Iterables.transform(Iterables.filter(iterable, Objects::nonNull), DataFileWrapper::wrap));
+ }
+
+ @Override
+ protected Wrapper<DataFile> wrapper() {
+ return WRAPPERS.get();
+ }
+
+ @Override
+ protected Wrapper<DataFile> wrap(DataFile dataFile) {
+ return DataFileWrapper.wrap(dataFile);
+ }
+
+ @Override
+ protected Class<DataFile> elementClass() {
+ return DataFile.class;
+ }
+
+ /**
+ * Since {@link WrapperSet} itself isn't {@link Serializable}, this requires custom logic to write
+ * {@link DataFileSet} to the given stream.
+ *
+ * @param out The output stream to write to
+ * @throws IOException in case the object can't be written
+ */
+ private void writeObject(ObjectOutputStream out) throws IOException {
Review Comment:
It was my bad to suggest moving `Serializable` to children. It is not worth the extra complexity. Let's simply make the parent `Serializable` and remove this logic. Sorry!
##########
api/src/main/java/org/apache/iceberg/util/DataFileSet.java:
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.io.Serializable;
+import java.util.Objects;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+
+public class DataFileSet extends WrapperSet<DataFile> implements Serializable {
+ private static final ThreadLocal<DataFileWrapper> WRAPPERS =
+ ThreadLocal.withInitial(() -> DataFileWrapper.wrap(null));
+
+ private DataFileSet() {
+ // needed for serialization/deserialization
+ }
+
+ private DataFileSet(Iterable<Wrapper<DataFile>> wrappers) {
+ super(wrappers);
+ }
+
+ public static DataFileSet create() {
+ return new DataFileSet();
+ }
+
+ public static DataFileSet of(Iterable<? extends DataFile> iterable) {
+ return new DataFileSet(
+ Iterables.transform(Iterables.filter(iterable, Objects::nonNull), DataFileWrapper::wrap));
Review Comment:
I think the extra filtering logic for null values may be confusing. I would prefer simplicity and an NPE in this case. I would also remove the extra logic in the wrapper to check for nulls in equals and hashCode. NPE is a perfectly valid exception in set implementations that don't support null values.
The following code would be easier to understand.
```
public static DataFileSet of(Iterable<? extends DataFile> iterable) {
return new DataFileSet(Iterables.transform(iterable, DataFileWrapper::wrap));
}
...
@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (!(o instanceof DataFileWrapper)) {
return false;
}
DataFileWrapper that = (DataFileWrapper) o;
return Objects.equals(file.location(), that.file.location());
}
```
##########
api/src/main/java/org/apache/iceberg/util/WrapperSet.java:
##########
@@ -0,0 +1,192 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterators;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.relocated.com.google.common.collect.Streams;
+
+/**
+ * A custom set for a {@link Wrapper} of the given type that maintains insertion order and does not
+ * allow null elements.
+ *
+ * @param <T> The type to wrap in a {@link Wrapper} instance.
+ */
+abstract class WrapperSet<T> implements Set<T> {
+ private final Set<Wrapper<T>> set = Sets.newLinkedHashSet();
+
+ protected WrapperSet(Iterable<Wrapper<T>> wrappers) {
+ wrappers.forEach(set::add);
+ }
+
+ protected WrapperSet() {}
+
+ protected abstract Wrapper<T> wrapper();
+
+ protected abstract Wrapper<T> wrap(T file);
+
+ protected abstract Class<T> elementClass();
+
+ protected interface Wrapper<T> {
+ T get();
+
+ Wrapper<T> set(T object);
+ }
+
+ protected Set<Wrapper<T>> set() {
+ return set;
+ }
+
+ @Override
+ public int size() {
+ return set.size();
+ }
+
+ @Override
+ public boolean isEmpty() {
+ return set.isEmpty();
+ }
+
+ @SuppressWarnings("unchecked")
+ @Override
+ public boolean contains(Object obj) {
Review Comment:
Given that `Set` allows `ClassCastException` in the API, I wonder whether we should simplify this.
```
@Override
public boolean contains(Object obj) {
Preconditions.checkNotNull(obj, "Invalid object: null");
Wrapper<T> wrapper = wrapper();
boolean result = set.contains(wrapper.set(elementClass().cast(obj)));
wrapper.set(null); // don't hold a reference to the value
return result;
}
```
Up to you. Either way, we should use `elementClass().cast(obj)` to get rid of `@SuppressWarnings`.
##########
api/src/main/java/org/apache/iceberg/util/WrapperSet.java:
##########
@@ -0,0 +1,192 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterators;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.relocated.com.google.common.collect.Streams;
+
+/**
+ * A custom set for a {@link Wrapper} of the given type that maintains insertion order and does not
+ * allow null elements.
+ *
+ * @param <T> The type to wrap in a {@link Wrapper} instance.
+ */
+abstract class WrapperSet<T> implements Set<T> {
+ private final Set<Wrapper<T>> set = Sets.newLinkedHashSet();
+
+ protected WrapperSet(Iterable<Wrapper<T>> wrappers) {
+ wrappers.forEach(set::add);
+ }
+
+ protected WrapperSet() {}
+
+ protected abstract Wrapper<T> wrapper();
+
+ protected abstract Wrapper<T> wrap(T file);
+
+ protected abstract Class<T> elementClass();
+
+ protected interface Wrapper<T> {
+ T get();
+
+ Wrapper<T> set(T object);
+ }
+
+ protected Set<Wrapper<T>> set() {
+ return set;
+ }
+
+ @Override
+ public int size() {
+ return set.size();
+ }
+
+ @Override
+ public boolean isEmpty() {
+ return set.isEmpty();
+ }
+
+ @SuppressWarnings("unchecked")
+ @Override
+ public boolean contains(Object obj) {
+ Preconditions.checkNotNull(obj, "Invalid object: null");
+ if (elementClass().isInstance(obj)) {
+ Wrapper<T> wrapper = wrapper();
+ boolean result = set.contains(wrapper.set((T) obj));
+ wrapper.set(null); // don't hold a reference to the value
+ return result;
+ }
+
+ return false;
+ }
+
+ @Override
+ public Iterator<T> iterator() {
+ return Iterators.transform(set.iterator(), Wrapper::get);
+ }
+
+ @Override
+ public Object[] toArray() {
+ return Lists.newArrayList(iterator()).toArray();
+ }
+
+ @Override
+ public <X> X[] toArray(X[] destArray) {
+ return Lists.newArrayList(iterator()).toArray(destArray);
+ }
+
+ @Override
+ public boolean add(T obj) {
+ Preconditions.checkNotNull(obj, "Invalid object: null");
+ return set.add(wrap(obj));
+ }
+
+ @SuppressWarnings("unchecked")
+ @Override
+ public boolean remove(Object obj) {
+ Preconditions.checkNotNull(obj, "Invalid object: null");
+ if (elementClass().isInstance(obj)) {
+ Wrapper<T> wrapper = wrapper();
+ boolean result = set.remove(wrapper.set((T) obj));
+ wrapper.set(null); // don't hold a reference to the value
+ return result;
+ }
+
+ return false;
+ }
+
+ @Override
+ public boolean containsAll(Collection<?> collection) {
+ Preconditions.checkNotNull(collection, "Invalid collection: null");
+ return Iterables.all(collection, this::contains);
+ }
+
+ @Override
+ public boolean addAll(Collection<? extends T> collection) {
+ Preconditions.checkNotNull(collection, "Invalid collection: null");
+ return collection.stream().filter(this::add).count() != 0;
+ }
+
+ @SuppressWarnings("unchecked")
+ @Override
+ public boolean retainAll(Collection<?> collection) {
+ Preconditions.checkNotNull(collection, "Invalid collection: null");
+ Set<Wrapper<T>> toRetain =
+ collection.stream()
+ .map(obj -> Preconditions.checkNotNull(obj, "Invalid object: null"))
+ .filter(elementClass()::isInstance)
+ .map(obj -> (T) obj)
Review Comment:
`(T) obj` -> `elementClass()::cast` to get rid of `@SuppressWarnings`.
##########
api/src/main/java/org/apache/iceberg/util/DataFileSet.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.util.Objects;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.types.Comparators;
+
+public class DataFileSet extends WrapperSet<DataFile> {
+ private static final ThreadLocal<DataFileWrapper> WRAPPERS =
+ ThreadLocal.withInitial(() -> DataFileWrapper.wrap(null));
+
+ protected DataFileSet(Iterable<Wrapper<DataFile>> wrappers) {
+ super(wrappers);
+ }
+
+ public static DataFileSet empty() {
+ return new DataFileSet(Sets.newLinkedHashSet());
+ }
+
+ public static DataFileSet of(Iterable<DataFile> iterable) {
+ return new DataFileSet(
+ Sets.newLinkedHashSet(Iterables.transform(iterable, DataFileWrapper::wrap)));
+ }
+
+ @Override
+ protected Wrapper<DataFile> wrapper() {
+ return WRAPPERS.get();
+ }
+
+ @Override
+ protected Wrapper<DataFile> wrap(DataFile dataFile) {
+ return DataFileWrapper.wrap(dataFile);
+ }
+
+ @Override
+ protected boolean isInstance(Object obj) {
+ return obj instanceof DataFile;
+ }
+
+ private static class DataFileWrapper implements Wrapper<DataFile> {
+ private DataFile file;
+
+ private DataFileWrapper(DataFile file) {
+ this.file = file;
+ }
+
+ private static DataFileWrapper wrap(DataFile dataFile) {
+ return new DataFileWrapper(dataFile);
+ }
+
+ @Override
+ public DataFile get() {
+ return file;
+ }
+
+ @Override
+ public Wrapper<DataFile> set(DataFile dataFile) {
+ this.file = dataFile;
+ return this;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+
+ if (!(o instanceof DataFileWrapper)) {
+ return false;
+ }
+
+ DataFileWrapper that = (DataFileWrapper) o;
+ if (null == file && null == that.file) {
Review Comment:
I would go for simpler logic as we already guard against null elements.
--
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
Re: [PR] Core: Add ContentFileSet and ContentFileWrapper [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11195:
URL: https://github.com/apache/iceberg/pull/11195#discussion_r1776041636
##########
api/src/main/java/org/apache/iceberg/util/ContentFileSet.java:
##########
@@ -0,0 +1,212 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.io.Serializable;
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterators;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+
+/**
+ * A custom set for {@link ContentFile} that maintains insertion order
+ *
+ * @param <F> the concrete Java class of a ContentFile instance
+ */
+public class ContentFileSet<F extends ContentFile<F>> implements Set<F>, Serializable {
Review Comment:
This will make wrappers simpler too, we won't need to implement `ContentFile` or reason about type bounds.
--
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
Re: [PR] Core: Add ContentFileSet and ContentFileWrapper [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11195:
URL: https://github.com/apache/iceberg/pull/11195#discussion_r1776041636
##########
api/src/main/java/org/apache/iceberg/util/ContentFileSet.java:
##########
@@ -0,0 +1,212 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.io.Serializable;
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterators;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+
+/**
+ * A custom set for {@link ContentFile} that maintains insertion order
+ *
+ * @param <F> the concrete Java class of a ContentFile instance
+ */
+public class ContentFileSet<F extends ContentFile<F>> implements Set<F>, Serializable {
Review Comment:
This will make wrappers simpler too, we won't need to implement `ContentFile`.
--
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
Re: [PR] Core: Add DataFileSet / DeleteFileSet [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11195:
URL: https://github.com/apache/iceberg/pull/11195#discussion_r1778092264
##########
api/src/main/java/org/apache/iceberg/util/DataFileSet.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.util.Objects;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.types.Comparators;
+
+public class DataFileSet extends WrapperSet<DataFile> {
+ private static final ThreadLocal<DataFileWrapper> WRAPPERS =
+ ThreadLocal.withInitial(() -> DataFileWrapper.wrap(null));
+
+ protected DataFileSet(Iterable<Wrapper<DataFile>> wrappers) {
+ super(wrappers);
+ }
+
+ public static DataFileSet empty() {
+ return new DataFileSet(Sets.newLinkedHashSet());
+ }
+
+ public static DataFileSet of(Iterable<DataFile> iterable) {
+ return new DataFileSet(
+ Sets.newLinkedHashSet(Iterables.transform(iterable, DataFileWrapper::wrap)));
+ }
+
+ @Override
+ protected Wrapper<DataFile> wrapper() {
+ return WRAPPERS.get();
+ }
+
+ @Override
+ protected Wrapper<DataFile> wrap(DataFile dataFile) {
+ return DataFileWrapper.wrap(dataFile);
+ }
+
+ @Override
+ protected boolean isInstance(Object obj) {
+ return obj instanceof DataFile;
+ }
+
+ private static class DataFileWrapper implements Wrapper<DataFile> {
+ private DataFile file;
+
+ private DataFileWrapper(DataFile file) {
+ this.file = file;
+ }
+
+ private static DataFileWrapper wrap(DataFile dataFile) {
+ return new DataFileWrapper(dataFile);
+ }
+
+ @Override
+ public DataFile get() {
+ return file;
+ }
+
+ @Override
+ public Wrapper<DataFile> set(DataFile dataFile) {
+ this.file = dataFile;
+ return this;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+
+ if (!(o instanceof DataFileWrapper)) {
+ return false;
+ }
+
+ DataFileWrapper that = (DataFileWrapper) o;
+ if (null == file && null == that.file) {
+ return true;
+ }
+
+ if (null == file || null == that.file) {
+ return false;
+ }
+
+ return 0 == Comparators.charSequences().compare(file.location(), that.file.location());
Review Comment:
are sorry that was a left-over from the previous impl that used `path()` here. I've switched this to `Objects.equals()` now
--
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
Re: [PR] Core: Add DataFileSet / DeleteFileSet [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11195:
URL: https://github.com/apache/iceberg/pull/11195#discussion_r1778100745
##########
api/src/main/java/org/apache/iceberg/util/DataFileSet.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.util.Objects;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.types.Comparators;
+
+public class DataFileSet extends WrapperSet<DataFile> {
+ private static final ThreadLocal<DataFileWrapper> WRAPPERS =
+ ThreadLocal.withInitial(() -> DataFileWrapper.wrap(null));
+
+ protected DataFileSet(Iterable<Wrapper<DataFile>> wrappers) {
+ super(wrappers);
+ }
+
+ public static DataFileSet empty() {
Review Comment:
I'm fine either way but I named it `empty()` to be consistent with `CharSequenceSet`. However, looking at `CharSequenceMap` it uses `create()` so let's go with `create()` then
--
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
Re: [PR] Core: Add DataFileSet / DeleteFileSet [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11195:
URL: https://github.com/apache/iceberg/pull/11195#discussion_r1778689365
##########
api/src/main/java/org/apache/iceberg/util/DataFileSet.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.util.Objects;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.types.Comparators;
+
+public class DataFileSet extends WrapperSet<DataFile> {
+ private static final ThreadLocal<DataFileWrapper> WRAPPERS =
+ ThreadLocal.withInitial(() -> DataFileWrapper.wrap(null));
+
+ protected DataFileSet(Iterable<Wrapper<DataFile>> wrappers) {
+ super(wrappers);
+ }
+
+ public static DataFileSet empty() {
+ return new DataFileSet(Sets.newLinkedHashSet());
Review Comment:
also yes, we could also just pass `Collections.emptyList()` here
--
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