aihuaxu commented on code in PR #12105:
URL: https://github.com/apache/iceberg/pull/12105#discussion_r1931023505
##########
core/src/main/java/org/apache/iceberg/variants/ShreddedObject.java:
##########
@@ -35,22 +39,55 @@
* fields. This also does not allow updating or replacing the metadata for the
unshredded object,
* which could require recursively rewriting field IDs.
*/
-class ShreddedObject implements VariantObject {
- private final SerializedMetadata metadata;
- private final SerializedObject unshredded;
+public class ShreddedObject implements VariantObject {
+ private final VariantMetadata metadata;
+ private final VariantObject unshredded;
private final Map<String, VariantValue> shreddedFields = Maps.newHashMap();
+ private final Set<String> removedFields = Sets.newHashSet();
private SerializationState serializationState = null;
- ShreddedObject(SerializedMetadata metadata) {
+ ShreddedObject(VariantMetadata metadata) {
this.metadata = metadata;
this.unshredded = null;
}
- ShreddedObject(SerializedObject unshredded) {
- this.metadata = unshredded.metadata();
+ ShreddedObject(VariantMetadata metadata, VariantObject unshredded) {
+ this.metadata = metadata;
this.unshredded = unshredded;
}
+ @VisibleForTesting
+ VariantMetadata metadata() {
+ return metadata;
+ }
+
+ private Set<String> nameSet() {
+ Set<String> names = Sets.newHashSet(shreddedFields.keySet());
+
+ if (unshredded != null) {
+ Iterables.addAll(names, unshredded.fieldNames());
+ }
+
+ names.removeAll(removedFields);
+
+ return names;
+ }
+
+ @Override
+ public Iterable<String> fieldNames() {
+ return nameSet();
+ }
+
+ @Override
+ public int numFields() {
+ return nameSet().size();
+ }
+
+ public void remove(String field) {
Review Comment:
What does this remove() try to support or to be used?
##########
core/src/main/java/org/apache/iceberg/variants/PrimitiveWrapper.java:
##########
@@ -47,17 +48,23 @@ class PrimitiveWrapper<T> implements VariantPrimitive<T> {
private static final byte BINARY_HEADER =
VariantUtil.primitiveHeader(Primitives.TYPE_BINARY);
private static final byte STRING_HEADER =
VariantUtil.primitiveHeader(Primitives.TYPE_STRING);
- private final Variants.PhysicalType type;
+ private final PhysicalType type;
private final T value;
private ByteBuffer buffer = null;
- PrimitiveWrapper(Variants.PhysicalType type, T value) {
- this.type = type;
+ PrimitiveWrapper(PhysicalType type, T value) {
+ if (value instanceof Boolean
Review Comment:
nit: seems I prefer the existing implementation which is cleaner and
consistent with other types.
##########
core/src/main/java/org/apache/iceberg/variants/SerializedArray.java:
##########
@@ -61,8 +61,8 @@ private SerializedArray(SerializedMetadata metadata,
ByteBuffer value, int heade
this.array = new VariantValue[numElements];
}
- @VisibleForTesting
- int numElements() {
+ @Override
+ public int numElements() {
Review Comment:
Thanks for addressing it. I was trying to make Parquet change in
(https://github.com/apache/iceberg/pull/11653/files#diff-b8e8443fcec3843e538dbc702d4c131ff58359cb83ccdb211d8679c1d77c16bd)
and we need to expose this.
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
