[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #9538: Improve primary key serialization performance

[GitBox]

Jackie-Jiang commented on code in PR #9538:
URL: https://github.com/apache/pinot/pull/9538#discussion_r989694085


##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/readers/PrimaryKey.java:
##########
@@ -37,7 +41,57 @@ public Object[] getValues() {
   }
 
   public byte[] asBytes() {
-    return SerializationUtils.serialize(_values);
+    int sizeInBytes = 0;
+    byte[][] cache = new byte[_values.length][];
+    for (int i = 0; i < _values.length; i++) {
+      Object value = _values[i];
+      if (value == null) {
+        continue;
+      }
+
+      if (value instanceof Integer) {
+        sizeInBytes += Integer.BYTES;
+      } else if (value instanceof Long) {
+        sizeInBytes += Long.BYTES;
+      } else if (value instanceof String) {
+        cache[i] = ((String) value).getBytes(StandardCharsets.UTF_8);
+        sizeInBytes += cache[i].length + Integer.BYTES;
+      } else if (value instanceof ByteArray) {
+        cache[i] = ((ByteArray) value).getBytes();
+        sizeInBytes += ((ByteArray) value).length() + Integer.BYTES;
+      } else if (value instanceof Float) {
+        sizeInBytes += Float.BYTES;
+      } else if (value instanceof Double) {
+        sizeInBytes += Double.BYTES;
+      } else if (value instanceof BigDecimal) {
+        cache[i] = BigDecimalUtils.serialize((BigDecimal) value);
+        sizeInBytes += cache[i].length + Integer.BYTES;
+      } else {
+        throw new IllegalStateException("Data type not supported for 
serializing Primary Key with value: " + value);
+      }
+    }
+
+    ByteBuffer byteBuffer = ByteBuffer.allocate(sizeInBytes);
+    for (int i = 0; i < _values.length; i++) {
+      Object value = _values[i];
+      if (value == null) {

Review Comment:
   No need to check null here if checked above



##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/readers/PrimaryKey.java:
##########
@@ -37,7 +41,57 @@ public Object[] getValues() {
   }
 
   public byte[] asBytes() {
-    return SerializationUtils.serialize(_values);
+    int sizeInBytes = 0;
+    byte[][] cache = new byte[_values.length][];
+    for (int i = 0; i < _values.length; i++) {
+      Object value = _values[i];
+      if (value == null) {
+        continue;
+      }
+
+      if (value instanceof Integer) {
+        sizeInBytes += Integer.BYTES;
+      } else if (value instanceof Long) {
+        sizeInBytes += Long.BYTES;
+      } else if (value instanceof String) {
+        cache[i] = ((String) value).getBytes(StandardCharsets.UTF_8);
+        sizeInBytes += cache[i].length + Integer.BYTES;
+      } else if (value instanceof ByteArray) {
+        cache[i] = ((ByteArray) value).getBytes();
+        sizeInBytes += ((ByteArray) value).length() + Integer.BYTES;

Review Comment:
   (nit)
   ```suggestion
           sizeInBytes += cache[i].length + Integer.BYTES;
   ```



##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/readers/PrimaryKey.java:
##########
@@ -37,7 +41,57 @@ public Object[] getValues() {
   }
 
   public byte[] asBytes() {
-    return SerializationUtils.serialize(_values);
+    int sizeInBytes = 0;
+    byte[][] cache = new byte[_values.length][];
+    for (int i = 0; i < _values.length; i++) {
+      Object value = _values[i];
+      if (value == null) {
+        continue;
+      }
+
+      if (value instanceof Integer) {
+        sizeInBytes += Integer.BYTES;
+      } else if (value instanceof Long) {
+        sizeInBytes += Long.BYTES;
+      } else if (value instanceof String) {
+        cache[i] = ((String) value).getBytes(StandardCharsets.UTF_8);
+        sizeInBytes += cache[i].length + Integer.BYTES;
+      } else if (value instanceof ByteArray) {
+        cache[i] = ((ByteArray) value).getBytes();
+        sizeInBytes += ((ByteArray) value).length() + Integer.BYTES;
+      } else if (value instanceof Float) {
+        sizeInBytes += Float.BYTES;
+      } else if (value instanceof Double) {
+        sizeInBytes += Double.BYTES;
+      } else if (value instanceof BigDecimal) {
+        cache[i] = BigDecimalUtils.serialize((BigDecimal) value);
+        sizeInBytes += cache[i].length + Integer.BYTES;
+      } else {
+        throw new IllegalStateException("Data type not supported for 
serializing Primary Key with value: " + value);

Review Comment:
   We can handle `null` here:
   ```suggestion
           throw new IllegalStateException(String.format("Unsupported value: %s 
of type: %s", value, value != null ? value.getClass() : null));
   ```



##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/readers/PrimaryKey.java:
##########
@@ -37,7 +41,57 @@ public Object[] getValues() {
   }
 
   public byte[] asBytes() {
-    return SerializationUtils.serialize(_values);
+    int sizeInBytes = 0;
+    byte[][] cache = new byte[_values.length][];

Review Comment:
   In majority of the use cases, primary key has only one value, and we can 
optimize that case by doing one pass



##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/readers/PrimaryKey.java:
##########
@@ -37,7 +41,57 @@ public Object[] getValues() {
   }
 
   public byte[] asBytes() {
-    return SerializationUtils.serialize(_values);
+    int sizeInBytes = 0;
+    byte[][] cache = new byte[_values.length][];
+    for (int i = 0; i < _values.length; i++) {
+      Object value = _values[i];
+      if (value == null) {

Review Comment:
   It should not be `null` or we cannot guarantee different primary key 
generate different bytes.



-- 
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: commits-unsubscr...@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org