You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2020/09/07 10:13:05 UTC
[GitHub] [iceberg] pvary opened a new pull request #1430: Core: Fix Metrics serialization
pvary opened a new pull request #1430:
URL: https://github.com/apache/iceberg/pull/1430
Metrics implements Serializable, but contains ByteBuffers which are not serializable.
This PR makes sure that the Metrics object remain serializable
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] pvary commented on a change in pull request #1430: API: Fix Metrics serialization
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #1430:
URL: https://github.com/apache/iceberg/pull/1430#discussion_r485027055
##########
File path: api/src/main/java/org/apache/iceberg/Metrics.java
##########
@@ -120,4 +124,83 @@ public Long recordCount() {
public Map<Integer, ByteBuffer> upperBounds() {
return upperBounds;
}
+
+ /**
+ * Implemented the method to enable serialization of ByteBuffers.
+ * @param out The stream where to write
+ * @throws IOException On serialization error
+ */
+ private void writeObject(ObjectOutputStream out) throws IOException {
+ out.writeObject(rowCount);
+ out.writeObject(columnSizes);
+ out.writeObject(valueCounts);
+ out.writeObject(nullValueCounts);
+
+ writeByteBufferMap(out, lowerBounds);
+ writeByteBufferMap(out, upperBounds);
+ }
+
+ private static void writeByteBufferMap(ObjectOutputStream out, Map<Integer, ByteBuffer> byteBufferMap)
+ throws IOException {
+ if (byteBufferMap == null) {
+ out.writeInt(-1);
+
+ } else {
+ // Write the size
+ out.writeInt(byteBufferMap.size());
+
+ for (Map.Entry<Integer, ByteBuffer> entry : byteBufferMap.entrySet()) {
+ // Write the key
+ out.writeObject(entry.getKey());
+
+ // Write the value
+ if (entry.getValue() != null) {
+ out.writeObject(entry.getValue().array());
Review comment:
I do not know how this ByteBuffer is used. I was considering the proposed solution, but I am not entirely sure that we only want to store the remaining part of the ByteBuffer, so I would like to know the opinions of people more familiar with the Iceberg usages of this stuff.
Maybe a check for specific implementation and error message for other options would be better. What do you think?
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] fbocse commented on a change in pull request #1430: API: Fix Metrics serialization
Posted by GitBox <gi...@apache.org>.
fbocse commented on a change in pull request #1430:
URL: https://github.com/apache/iceberg/pull/1430#discussion_r485729659
##########
File path: api/src/main/java/org/apache/iceberg/Metrics.java
##########
@@ -120,4 +124,90 @@ public Long recordCount() {
public Map<Integer, ByteBuffer> upperBounds() {
return upperBounds;
}
+
+ /**
+ * Implemented the method to enable serialization of ByteBuffers.
+ * @param out The stream where to write
+ * @throws IOException On serialization error
+ */
+ private void writeObject(ObjectOutputStream out) throws IOException {
+ out.writeObject(rowCount);
+ out.writeObject(columnSizes);
+ out.writeObject(valueCounts);
+ out.writeObject(nullValueCounts);
+
+ writeByteBufferMap(out, lowerBounds);
+ writeByteBufferMap(out, upperBounds);
+ }
+
+ private static void writeByteBufferMap(ObjectOutputStream out, Map<Integer, ByteBuffer> byteBufferMap)
+ throws IOException {
+ if (byteBufferMap == null) {
+ out.writeInt(-1);
+
+ } else {
+ // Write the size
+ out.writeInt(byteBufferMap.size());
+
+ for (Map.Entry<Integer, ByteBuffer> entry : byteBufferMap.entrySet()) {
+ // Write the key
+ out.writeObject(entry.getKey());
+
+ // Write the value
+ if (entry.getValue() != null) {
+ // Copy the actual values from the buffer
+ ByteBuffer bb = entry.getValue();
+ byte[] bytes = new byte[bb.remaining()];
+ bb.get(bytes);
+ bb.position(bb.position() - bytes.length); // Restores the buffer position
+
+ // Write out the data
+ out.writeObject(bytes);
+ } else {
+ out.writeObject(null);
+ }
+ }
+ }
+ }
+
+ /**
+ * Implemented the method to enable deserialization of ByteBuffers.
+ * @param in The stream to read from
+ * @throws IOException On serialization error
+ * @throws ClassNotFoundException If the class is not found
+ */
+ private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundException {
+ rowCount = (Long) in.readObject();
+ columnSizes = (Map<Integer, Long>) in.readObject();
+ valueCounts = (Map<Integer, Long>) in.readObject();
+ nullValueCounts = (Map<Integer, Long>) in.readObject();
+
+ lowerBounds = readByteBufferMap(in);
+ upperBounds = readByteBufferMap(in);
+ }
+
+ private static Map<Integer, ByteBuffer> readByteBufferMap(ObjectInputStream in)
+ throws IOException, ClassNotFoundException {
+ int size = in.readInt();
+
+ if (size == -1) {
+ return null;
+
+ } else {
+ Map<Integer, ByteBuffer> result = Maps.newHashMap();
Review comment:
Seems like we can probably just get away with a simple `new java.util.HashMap()`, no need for guava as a dependency for this class.
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] pvary commented on a change in pull request #1430: API: Fix Metrics serialization
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #1430:
URL: https://github.com/apache/iceberg/pull/1430#discussion_r485035182
##########
File path: api/src/main/java/org/apache/iceberg/Metrics.java
##########
@@ -120,4 +124,83 @@ public Long recordCount() {
public Map<Integer, ByteBuffer> upperBounds() {
return upperBounds;
}
+
+ /**
+ * Implemented the method to enable serialization of ByteBuffers.
+ * @param out The stream where to write
+ * @throws IOException On serialization error
+ */
+ private void writeObject(ObjectOutputStream out) throws IOException {
+ out.writeObject(rowCount);
+ out.writeObject(columnSizes);
+ out.writeObject(valueCounts);
+ out.writeObject(nullValueCounts);
+
+ writeByteBufferMap(out, lowerBounds);
+ writeByteBufferMap(out, upperBounds);
+ }
+
+ private static void writeByteBufferMap(ObjectOutputStream out, Map<Integer, ByteBuffer> byteBufferMap)
+ throws IOException {
+ if (byteBufferMap == null) {
+ out.writeInt(-1);
+
+ } else {
+ // Write the size
+ out.writeInt(byteBufferMap.size());
+
+ for (Map.Entry<Integer, ByteBuffer> entry : byteBufferMap.entrySet()) {
+ // Write the key
+ out.writeObject(entry.getKey());
+
+ // Write the value
+ if (entry.getValue() != null) {
+ out.writeObject(entry.getValue().array());
Review comment:
Maybe just checking the hasArray and checking that the offset is 0 would be enough. And throw an exception in other cases. Maybe we can avoid the costly buffer allocation. Or use them only when they are needed.
Good that you brought this up. Started to fix this warning, but then got sidetracked 😄
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] kbendick commented on a change in pull request #1430: API: Fix Metrics serialization
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #1430:
URL: https://github.com/apache/iceberg/pull/1430#discussion_r487585623
##########
File path: api/src/main/java/org/apache/iceberg/Metrics.java
##########
@@ -120,4 +124,90 @@ public Long recordCount() {
public Map<Integer, ByteBuffer> upperBounds() {
return upperBounds;
}
+
+ /**
+ * Implemented the method to enable serialization of ByteBuffers.
+ * @param out The stream where to write
+ * @throws IOException On serialization error
+ */
+ private void writeObject(ObjectOutputStream out) throws IOException {
+ out.writeObject(rowCount);
+ out.writeObject(columnSizes);
+ out.writeObject(valueCounts);
+ out.writeObject(nullValueCounts);
+
+ writeByteBufferMap(out, lowerBounds);
+ writeByteBufferMap(out, upperBounds);
+ }
+
+ private static void writeByteBufferMap(ObjectOutputStream out, Map<Integer, ByteBuffer> byteBufferMap)
+ throws IOException {
+ if (byteBufferMap == null) {
+ out.writeInt(-1);
+
+ } else {
+ // Write the size
+ out.writeInt(byteBufferMap.size());
+
+ for (Map.Entry<Integer, ByteBuffer> entry : byteBufferMap.entrySet()) {
+ // Write the key
+ out.writeObject(entry.getKey());
+
+ // Write the value
+ if (entry.getValue() != null) {
+ // Copy the actual values from the buffer
+ ByteBuffer bb = entry.getValue();
+ byte[] bytes = new byte[bb.remaining()];
+ bb.get(bytes);
+ bb.position(bb.position() - bytes.length); // Restores the buffer position
Review comment:
We have a `ByteBuffers` util located in the core module at `org.apache.iceberg.util.ByteBuffers` that handles the appropriate copying and restoration of ByteBuffers.
Please consider using that for consistency and for easier refactoring of ByteBuffer issues in the future. Here's a link to that class: https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/util/ByteBuffers.java
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] rdblue commented on a change in pull request #1430: API: Fix Metrics serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1430:
URL: https://github.com/apache/iceberg/pull/1430#discussion_r487594835
##########
File path: api/src/test/java/org/apache/iceberg/TestMetricsSerialization.java
##########
@@ -0,0 +1,123 @@
+/*
+ * 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;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.nio.ByteBuffer;
+import java.util.HashMap;
+import java.util.Map;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class TestMetricsSerialization {
+
+ @Test
+ public void testSerialization() throws IOException, ClassNotFoundException {
+ Metrics original = generateMetrics();
+
+ byte[] serialized = serialize(original);
+ Metrics result = deserialize(serialized);
+
+ assertEquals(original, result);
+ }
+
+ @Test
+ public void testSerializationWithNulls() throws IOException, ClassNotFoundException {
+ Metrics original = generateMetricsWithNulls();
+
+ byte[] serialized = serialize(original);
+ Metrics result = deserialize(serialized);
+
+ assertEquals(original, result);
+ }
+
+ private static byte[] serialize(Metrics metrics) throws IOException {
+ try (ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream()) {
+ ObjectOutputStream objectOutputStream = new ObjectOutputStream(byteArrayOutputStream);
+ objectOutputStream.writeObject(metrics);
Review comment:
Don't we need to flush the object output stream?
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] pvary commented on a change in pull request #1430: API: Fix Metrics serialization
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #1430:
URL: https://github.com/apache/iceberg/pull/1430#discussion_r487777666
##########
File path: api/src/test/java/org/apache/iceberg/TestMetricsSerialization.java
##########
@@ -0,0 +1,123 @@
+/*
+ * 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;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.nio.ByteBuffer;
+import java.util.HashMap;
+import java.util.Map;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class TestMetricsSerialization {
+
+ @Test
+ public void testSerialization() throws IOException, ClassNotFoundException {
+ Metrics original = generateMetrics();
+
+ byte[] serialized = serialize(original);
+ Metrics result = deserialize(serialized);
+
+ assertEquals(original, result);
+ }
+
+ @Test
+ public void testSerializationWithNulls() throws IOException, ClassNotFoundException {
+ Metrics original = generateMetricsWithNulls();
+
+ byte[] serialized = serialize(original);
+ Metrics result = deserialize(serialized);
+
+ assertEquals(original, result);
+ }
+
+ private static byte[] serialize(Metrics metrics) throws IOException {
+ try (ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream()) {
+ ObjectOutputStream objectOutputStream = new ObjectOutputStream(byteArrayOutputStream);
+ objectOutputStream.writeObject(metrics);
Review comment:
Good catch! Thanks!
Done
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #1430: API: Fix Metrics serialization
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #1430:
URL: https://github.com/apache/iceberg/pull/1430#discussion_r485887325
##########
File path: api/src/main/java/org/apache/iceberg/Metrics.java
##########
@@ -120,4 +124,83 @@ public Long recordCount() {
public Map<Integer, ByteBuffer> upperBounds() {
return upperBounds;
}
+
+ /**
+ * Implemented the method to enable serialization of ByteBuffers.
+ * @param out The stream where to write
+ * @throws IOException On serialization error
+ */
+ private void writeObject(ObjectOutputStream out) throws IOException {
+ out.writeObject(rowCount);
+ out.writeObject(columnSizes);
+ out.writeObject(valueCounts);
+ out.writeObject(nullValueCounts);
+
+ writeByteBufferMap(out, lowerBounds);
+ writeByteBufferMap(out, upperBounds);
+ }
+
+ private static void writeByteBufferMap(ObjectOutputStream out, Map<Integer, ByteBuffer> byteBufferMap)
+ throws IOException {
+ if (byteBufferMap == null) {
+ out.writeInt(-1);
+
+ } else {
+ // Write the size
+ out.writeInt(byteBufferMap.size());
+
+ for (Map.Entry<Integer, ByteBuffer> entry : byteBufferMap.entrySet()) {
+ // Write the key
+ out.writeObject(entry.getKey());
+
+ // Write the value
+ if (entry.getValue() != null) {
+ out.writeObject(entry.getValue().array());
Review comment:
lgtm!
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #1430: API: Fix Metrics serialization
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #1430:
URL: https://github.com/apache/iceberg/pull/1430#discussion_r485018606
##########
File path: api/src/main/java/org/apache/iceberg/Metrics.java
##########
@@ -120,4 +124,83 @@ public Long recordCount() {
public Map<Integer, ByteBuffer> upperBounds() {
return upperBounds;
}
+
+ /**
+ * Implemented the method to enable serialization of ByteBuffers.
+ * @param out The stream where to write
+ * @throws IOException On serialization error
+ */
+ private void writeObject(ObjectOutputStream out) throws IOException {
+ out.writeObject(rowCount);
+ out.writeObject(columnSizes);
+ out.writeObject(valueCounts);
+ out.writeObject(nullValueCounts);
+
+ writeByteBufferMap(out, lowerBounds);
+ writeByteBufferMap(out, upperBounds);
+ }
+
+ private static void writeByteBufferMap(ObjectOutputStream out, Map<Integer, ByteBuffer> byteBufferMap)
+ throws IOException {
+ if (byteBufferMap == null) {
+ out.writeInt(-1);
+
+ } else {
+ // Write the size
+ out.writeInt(byteBufferMap.size());
+
+ for (Map.Entry<Integer, ByteBuffer> entry : byteBufferMap.entrySet()) {
+ // Write the key
+ out.writeObject(entry.getKey());
+
+ // Write the value
+ if (entry.getValue() != null) {
+ out.writeObject(entry.getValue().array());
Review comment:
I think this is one of those possibly dangerous things if the underlying implementation of the byte buffer isn't backed by an array. I believe the safe way to handle this is
```
byte[] b = new byte[bb.remaining()];
bb.get(b);
```
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] kbendick commented on a change in pull request #1430: API: Fix Metrics serialization
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #1430:
URL: https://github.com/apache/iceberg/pull/1430#discussion_r487588916
##########
File path: api/src/main/java/org/apache/iceberg/Metrics.java
##########
@@ -120,4 +124,90 @@ public Long recordCount() {
public Map<Integer, ByteBuffer> upperBounds() {
return upperBounds;
}
+
+ /**
+ * Implemented the method to enable serialization of ByteBuffers.
+ * @param out The stream where to write
+ * @throws IOException On serialization error
+ */
+ private void writeObject(ObjectOutputStream out) throws IOException {
+ out.writeObject(rowCount);
+ out.writeObject(columnSizes);
+ out.writeObject(valueCounts);
+ out.writeObject(nullValueCounts);
+
+ writeByteBufferMap(out, lowerBounds);
+ writeByteBufferMap(out, upperBounds);
+ }
Review comment:
Oh, actually, I spoke to soon. There is documentation on the serialization of types when storing them as lower bounds / upper bounds in the maps of manifest files, namely in the `data_files` struct: https://iceberg.apache.org/spec/#manifests
There's also documentation on what single values can be stored in the upper bounds / lower bounds byte buffer maps and how they are serialized: https://iceberg.apache.org/spec/#appendix-d-single-value-serialization
Perhaps something from that documentation page can be used to make this serialization choice inline with the existing stuff? Or at the least I would recommend that documentation be added, though I think that you might want to consider submitting this PR to the dev mailing list at dv@iceberg.apache.org, as I feel that likely some people will have an opinion about the introduction of an additional standard for serialization (especially given that it's part of the public facing API). You can see the dev mailing list archive here: https://www.mail-archive.com/dev@iceberg.apache.org/
##########
File path: api/src/main/java/org/apache/iceberg/Metrics.java
##########
@@ -120,4 +124,90 @@ public Long recordCount() {
public Map<Integer, ByteBuffer> upperBounds() {
return upperBounds;
}
+
+ /**
+ * Implemented the method to enable serialization of ByteBuffers.
+ * @param out The stream where to write
+ * @throws IOException On serialization error
+ */
+ private void writeObject(ObjectOutputStream out) throws IOException {
+ out.writeObject(rowCount);
+ out.writeObject(columnSizes);
+ out.writeObject(valueCounts);
+ out.writeObject(nullValueCounts);
+
+ writeByteBufferMap(out, lowerBounds);
+ writeByteBufferMap(out, upperBounds);
+ }
Review comment:
Oh, actually, I spoke too soon. There is documentation on the serialization of types when storing them as lower bounds / upper bounds in the maps of manifest files, namely in the `data_files` struct: https://iceberg.apache.org/spec/#manifests
There's also documentation on what single values can be stored in the upper bounds / lower bounds byte buffer maps and how they are serialized: https://iceberg.apache.org/spec/#appendix-d-single-value-serialization
Perhaps something from that documentation page can be used to make this serialization choice inline with the existing stuff? Or at the least I would recommend that documentation be added, though I think that you might want to consider submitting this PR to the dev mailing list at dv@iceberg.apache.org, as I feel that likely some people will have an opinion about the introduction of an additional standard for serialization (especially given that it's part of the public facing API). You can see the dev mailing list archive here: https://www.mail-archive.com/dev@iceberg.apache.org/
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #1430: API: Fix Metrics serialization
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #1430:
URL: https://github.com/apache/iceberg/pull/1430#discussion_r485018606
##########
File path: api/src/main/java/org/apache/iceberg/Metrics.java
##########
@@ -120,4 +124,83 @@ public Long recordCount() {
public Map<Integer, ByteBuffer> upperBounds() {
return upperBounds;
}
+
+ /**
+ * Implemented the method to enable serialization of ByteBuffers.
+ * @param out The stream where to write
+ * @throws IOException On serialization error
+ */
+ private void writeObject(ObjectOutputStream out) throws IOException {
+ out.writeObject(rowCount);
+ out.writeObject(columnSizes);
+ out.writeObject(valueCounts);
+ out.writeObject(nullValueCounts);
+
+ writeByteBufferMap(out, lowerBounds);
+ writeByteBufferMap(out, upperBounds);
+ }
+
+ private static void writeByteBufferMap(ObjectOutputStream out, Map<Integer, ByteBuffer> byteBufferMap)
+ throws IOException {
+ if (byteBufferMap == null) {
+ out.writeInt(-1);
+
+ } else {
+ // Write the size
+ out.writeInt(byteBufferMap.size());
+
+ for (Map.Entry<Integer, ByteBuffer> entry : byteBufferMap.entrySet()) {
+ // Write the key
+ out.writeObject(entry.getKey());
+
+ // Write the value
+ if (entry.getValue() != null) {
+ out.writeObject(entry.getValue().array());
Review comment:
I think this is one of those possibly dangerous things if the underlying implementation of the byte buffer isn't backed by an array. I believe the safe way to handle this is
```
byte[] b = new byte[bb.remaining()];
bb.get(b);
```
http://errorprone.info/bugpattern/ByteBufferBackingArray
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] rdblue merged pull request #1430: API: Fix Metrics serialization
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #1430:
URL: https://github.com/apache/iceberg/pull/1430
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] pvary commented on a change in pull request #1430: API: Fix Metrics serialization
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #1430:
URL: https://github.com/apache/iceberg/pull/1430#discussion_r485539679
##########
File path: api/src/main/java/org/apache/iceberg/Metrics.java
##########
@@ -120,4 +124,83 @@ public Long recordCount() {
public Map<Integer, ByteBuffer> upperBounds() {
return upperBounds;
}
+
+ /**
+ * Implemented the method to enable serialization of ByteBuffers.
+ * @param out The stream where to write
+ * @throws IOException On serialization error
+ */
+ private void writeObject(ObjectOutputStream out) throws IOException {
+ out.writeObject(rowCount);
+ out.writeObject(columnSizes);
+ out.writeObject(valueCounts);
+ out.writeObject(nullValueCounts);
+
+ writeByteBufferMap(out, lowerBounds);
+ writeByteBufferMap(out, upperBounds);
+ }
+
+ private static void writeByteBufferMap(ObjectOutputStream out, Map<Integer, ByteBuffer> byteBufferMap)
+ throws IOException {
+ if (byteBufferMap == null) {
+ out.writeInt(-1);
+
+ } else {
+ // Write the size
+ out.writeInt(byteBufferMap.size());
+
+ for (Map.Entry<Integer, ByteBuffer> entry : byteBufferMap.entrySet()) {
+ // Write the key
+ out.writeObject(entry.getKey());
+
+ // Write the value
+ if (entry.getValue() != null) {
+ out.writeObject(entry.getValue().array());
Review comment:
@RussellSpitzer: Fixed with your suggested changes. I have chickened out from using different serialization method for different buffers because I think the added complexity might not worth the gained performance.
Could you please recheck the change?
Thanks,
Peter
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] pvary commented on pull request #1430: API: Fix Metrics serialization
Posted by GitBox <gi...@apache.org>.
pvary commented on pull request #1430:
URL: https://github.com/apache/iceberg/pull/1430#issuecomment-688281553
@rdblue: Could you please review this?
Thanks,
Peter
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] pvary commented on a change in pull request #1430: API: Fix Metrics serialization
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #1430:
URL: https://github.com/apache/iceberg/pull/1430#discussion_r485816006
##########
File path: api/src/main/java/org/apache/iceberg/Metrics.java
##########
@@ -120,4 +124,90 @@ public Long recordCount() {
public Map<Integer, ByteBuffer> upperBounds() {
return upperBounds;
}
+
+ /**
+ * Implemented the method to enable serialization of ByteBuffers.
+ * @param out The stream where to write
+ * @throws IOException On serialization error
+ */
+ private void writeObject(ObjectOutputStream out) throws IOException {
+ out.writeObject(rowCount);
+ out.writeObject(columnSizes);
+ out.writeObject(valueCounts);
+ out.writeObject(nullValueCounts);
+
+ writeByteBufferMap(out, lowerBounds);
+ writeByteBufferMap(out, upperBounds);
+ }
+
+ private static void writeByteBufferMap(ObjectOutputStream out, Map<Integer, ByteBuffer> byteBufferMap)
+ throws IOException {
+ if (byteBufferMap == null) {
+ out.writeInt(-1);
+
+ } else {
+ // Write the size
+ out.writeInt(byteBufferMap.size());
+
+ for (Map.Entry<Integer, ByteBuffer> entry : byteBufferMap.entrySet()) {
+ // Write the key
+ out.writeObject(entry.getKey());
+
+ // Write the value
+ if (entry.getValue() != null) {
+ // Copy the actual values from the buffer
+ ByteBuffer bb = entry.getValue();
+ byte[] bytes = new byte[bb.remaining()];
+ bb.get(bytes);
+ bb.position(bb.position() - bytes.length); // Restores the buffer position
+
+ // Write out the data
+ out.writeObject(bytes);
+ } else {
+ out.writeObject(null);
+ }
+ }
+ }
+ }
+
+ /**
+ * Implemented the method to enable deserialization of ByteBuffers.
+ * @param in The stream to read from
+ * @throws IOException On serialization error
+ * @throws ClassNotFoundException If the class is not found
+ */
+ private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundException {
+ rowCount = (Long) in.readObject();
+ columnSizes = (Map<Integer, Long>) in.readObject();
+ valueCounts = (Map<Integer, Long>) in.readObject();
+ nullValueCounts = (Map<Integer, Long>) in.readObject();
+
+ lowerBounds = readByteBufferMap(in);
+ upperBounds = readByteBufferMap(in);
+ }
+
+ private static Map<Integer, ByteBuffer> readByteBufferMap(ObjectInputStream in)
+ throws IOException, ClassNotFoundException {
+ int size = in.readInt();
+
+ if (size == -1) {
+ return null;
+
+ } else {
+ Map<Integer, ByteBuffer> result = Maps.newHashMap();
Review comment:
Good point.
Removed guava dependency.
Thanks,
Peter
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] rdblue commented on pull request #1430: API: Fix Metrics serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1430:
URL: https://github.com/apache/iceberg/pull/1430#issuecomment-692394134
Thanks for fixing this, @pvary!
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] kbendick commented on a change in pull request #1430: API: Fix Metrics serialization
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #1430:
URL: https://github.com/apache/iceberg/pull/1430#discussion_r487587290
##########
File path: api/src/main/java/org/apache/iceberg/Metrics.java
##########
@@ -120,4 +124,90 @@ public Long recordCount() {
public Map<Integer, ByteBuffer> upperBounds() {
return upperBounds;
}
+
+ /**
+ * Implemented the method to enable serialization of ByteBuffers.
+ * @param out The stream where to write
+ * @throws IOException On serialization error
+ */
+ private void writeObject(ObjectOutputStream out) throws IOException {
+ out.writeObject(rowCount);
+ out.writeObject(columnSizes);
+ out.writeObject(valueCounts);
+ out.writeObject(nullValueCounts);
+
+ writeByteBufferMap(out, lowerBounds);
+ writeByteBufferMap(out, upperBounds);
+ }
Review comment:
Is this serialization format, e.g. `rowCount, columnSizes, valueCounts, nullValueCounts` and then the `lowerBounds` followed by `upperBounds` currently used anywhere else?
If so, perhaps there's a utility class that can be used?
If not, I personally think that this serialization format should be documented somewhere. We might need to include the format version in it as well, like we do for other places (where the new v2 format specification will be what is used to support row level deletes).
Additionally, would it make sense to move this serialization logic into a utility class if there's nothing that currently meets this need?
For example, type conversions to get the lower bounds value from the ByteBuffer are currently done in `org.apache.iceberg.types.Conversion#fromByteBuffer` as mentioned elsehwere in this file. Here's a link to the code, perhaps something there might make this task easier / more standardized: https://github.com/apache/iceberg/blob/master/api/src/main/java/org/apache/iceberg/types/Conversions.java
And if there's not a utility class, would it make sense to add one (like how the actual conversion of `upperBounds` and `lowerBounds` from their ByteBuffers is deletegated to the `Conversions` class?
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] rdblue commented on a change in pull request #1430: API: Fix Metrics serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1430:
URL: https://github.com/apache/iceberg/pull/1430#discussion_r487595125
##########
File path: api/src/main/java/org/apache/iceberg/Metrics.java
##########
@@ -120,4 +124,90 @@ public Long recordCount() {
public Map<Integer, ByteBuffer> upperBounds() {
return upperBounds;
}
+
+ /**
+ * Implemented the method to enable serialization of ByteBuffers.
+ * @param out The stream where to write
+ * @throws IOException On serialization error
+ */
+ private void writeObject(ObjectOutputStream out) throws IOException {
+ out.writeObject(rowCount);
+ out.writeObject(columnSizes);
+ out.writeObject(valueCounts);
+ out.writeObject(nullValueCounts);
+
+ writeByteBufferMap(out, lowerBounds);
+ writeByteBufferMap(out, upperBounds);
+ }
+
+ private static void writeByteBufferMap(ObjectOutputStream out, Map<Integer, ByteBuffer> byteBufferMap)
+ throws IOException {
+ if (byteBufferMap == null) {
+ out.writeInt(-1);
+
+ } else {
+ // Write the size
+ out.writeInt(byteBufferMap.size());
+
+ for (Map.Entry<Integer, ByteBuffer> entry : byteBufferMap.entrySet()) {
+ // Write the key
+ out.writeObject(entry.getKey());
+
+ // Write the value
+ if (entry.getValue() != null) {
+ // Copy the actual values from the buffer
+ ByteBuffer bb = entry.getValue();
+ byte[] bytes = new byte[bb.remaining()];
+ bb.get(bytes);
+ bb.position(bb.position() - bytes.length); // Restores the buffer position
Review comment:
I agree with @kbendick. You can call `ByteBuffers.toByteArray` instead of adding the logic to read 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.
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
[GitHub] [iceberg] rdblue commented on a change in pull request #1430: API: Fix Metrics serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1430:
URL: https://github.com/apache/iceberg/pull/1430#discussion_r487594524
##########
File path: api/src/main/java/org/apache/iceberg/Metrics.java
##########
@@ -120,4 +124,90 @@ public Long recordCount() {
public Map<Integer, ByteBuffer> upperBounds() {
return upperBounds;
}
+
+ /**
+ * Implemented the method to enable serialization of ByteBuffers.
+ * @param out The stream where to write
+ * @throws IOException On serialization error
+ */
+ private void writeObject(ObjectOutputStream out) throws IOException {
+ out.writeObject(rowCount);
+ out.writeObject(columnSizes);
+ out.writeObject(valueCounts);
+ out.writeObject(nullValueCounts);
+
+ writeByteBufferMap(out, lowerBounds);
+ writeByteBufferMap(out, upperBounds);
+ }
Review comment:
This isn't really a serialization format that needs to be specified or reusable (besides maybe `writeByteBufferMap`). Java serialization should only be used within a running job that has just one version of the Iceberg library, so what matters is that a test can write an object and read an identical one.
We don't want to go too far down the path of documenting things like this that we don't intend to support as a format.
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] pvary commented on a change in pull request #1430: API: Fix Metrics serialization
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #1430:
URL: https://github.com/apache/iceberg/pull/1430#discussion_r487774166
##########
File path: api/src/main/java/org/apache/iceberg/Metrics.java
##########
@@ -120,4 +124,90 @@ public Long recordCount() {
public Map<Integer, ByteBuffer> upperBounds() {
return upperBounds;
}
+
+ /**
+ * Implemented the method to enable serialization of ByteBuffers.
+ * @param out The stream where to write
+ * @throws IOException On serialization error
+ */
+ private void writeObject(ObjectOutputStream out) throws IOException {
+ out.writeObject(rowCount);
+ out.writeObject(columnSizes);
+ out.writeObject(valueCounts);
+ out.writeObject(nullValueCounts);
+
+ writeByteBufferMap(out, lowerBounds);
+ writeByteBufferMap(out, upperBounds);
+ }
+
+ private static void writeByteBufferMap(ObjectOutputStream out, Map<Integer, ByteBuffer> byteBufferMap)
+ throws IOException {
+ if (byteBufferMap == null) {
+ out.writeInt(-1);
+
+ } else {
+ // Write the size
+ out.writeInt(byteBufferMap.size());
+
+ for (Map.Entry<Integer, ByteBuffer> entry : byteBufferMap.entrySet()) {
+ // Write the key
+ out.writeObject(entry.getKey());
+
+ // Write the value
+ if (entry.getValue() != null) {
+ // Copy the actual values from the buffer
+ ByteBuffer bb = entry.getValue();
+ byte[] bytes = new byte[bb.remaining()];
+ bb.get(bytes);
+ bb.position(bb.position() - bytes.length); // Restores the buffer position
Review comment:
Thanks @kbendick! Have not noticed that class.
Had to refactor that and move it to the API package to reuse it. Please check if you agree with the move, or you think there is a better place for it.
Thanks,
Peter
----------------------------------------------------------------
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.
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