wgtmac commented on code in PR #1291:
URL: https://github.com/apache/parquet-mr/pull/1291#discussion_r1519191037
##########
parquet-column/src/main/java/org/apache/parquet/column/values/bytestreamsplit/ByteStreamSplitValuesReader.java:
##########
@@ -76,18 +87,12 @@ public void initFromPage(int valuesCount,
ByteBufferInputStream stream)
throw new ParquetDecodingException(errorMessage);
}
- // Allocate buffer for all of the byte stream data.
+ // Eagerly read and decode the data. This allows returning stable
+ // Binary views into the internal decode buffer for FIXED_LEN_BYTE_ARRAY.
final int totalSizeInBytes = stream.available();
- byteStreamData = new byte[totalSizeInBytes];
-
- // Eagerly read the data for each stream.
- final int numRead = stream.read(byteStreamData, 0, totalSizeInBytes);
- if (numRead != totalSizeInBytes) {
- String errorMessage = String.format(
- "Failed to read requested number of bytes. Expected: %d. Read %d.",
totalSizeInBytes, numRead);
- throw new ParquetDecodingException(errorMessage);
Review Comment:
It seems that this check has been changed into assert somewhere else. I'd
suggest to throw if unexpected padding or short read are found.
##########
parquet-column/src/main/java/org/apache/parquet/column/values/bytestreamsplit/ByteStreamSplitValuesReader.java:
##########
@@ -76,18 +87,12 @@ public void initFromPage(int valuesCount,
ByteBufferInputStream stream)
throw new ParquetDecodingException(errorMessage);
}
- // Allocate buffer for all of the byte stream data.
+ // Eagerly read and decode the data. This allows returning stable
+ // Binary views into the internal decode buffer for FIXED_LEN_BYTE_ARRAY.
final int totalSizeInBytes = stream.available();
- byteStreamData = new byte[totalSizeInBytes];
-
- // Eagerly read the data for each stream.
- final int numRead = stream.read(byteStreamData, 0, totalSizeInBytes);
- if (numRead != totalSizeInBytes) {
- String errorMessage = String.format(
- "Failed to read requested number of bytes. Expected: %d. Read %d.",
totalSizeInBytes, numRead);
- throw new ParquetDecodingException(errorMessage);
- }
-
+ final ByteBuffer encodedData = stream.slice(totalSizeInBytes).slice(); //
possibly zero-copy
+ final byte[] decodedData = decodeData(encodedData, this.valuesCount);
Review Comment:
Do we need to hold the reference of `decodedData`?
##########
parquet-column/src/main/java/org/apache/parquet/column/values/bytestreamsplit/ByteStreamSplitValuesWriter.java:
##########
@@ -140,4 +141,61 @@ public String memUsageString(String prefix) {
return String.format("%s DoubleByteStreamSplitWriter %d bytes", prefix,
getAllocatedSize());
}
}
+
+ public static class IntegerByteStreamSplitValuesWriter extends
ByteStreamSplitValuesWriter {
+ public IntegerByteStreamSplitValuesWriter(int initialCapacity, int
pageSize, ByteBufferAllocator allocator) {
+ super(4, initialCapacity, pageSize, allocator);
+ }
+
+ @Override
+ public void writeInteger(int v) {
+ super.scatterBytes(BytesUtils.intToBytes(v));
+ }
+
+ @Override
+ public String memUsageString(String prefix) {
+ return String.format("%s IntegerByteStreamSplitWriter %d bytes", prefix,
getAllocatedSize());
+ }
+ }
+
+ public static class LongByteStreamSplitValuesWriter extends
ByteStreamSplitValuesWriter {
+ public LongByteStreamSplitValuesWriter(int initialCapacity, int pageSize,
ByteBufferAllocator allocator) {
+ super(8, initialCapacity, pageSize, allocator);
+ }
+
+ @Override
+ public void writeLong(long v) {
+ super.scatterBytes(BytesUtils.longToBytes(v));
+ }
+
+ @Override
+ public String memUsageString(String prefix) {
+ return String.format("%s LongByteStreamSplitWriter %d bytes", prefix,
getAllocatedSize());
+ }
+ }
+
+ public static class FixedLenByteArrayByteStreamSplitValuesWriter extends
ByteStreamSplitValuesWriter {
+ private final int length;
+
+ public FixedLenByteArrayByteStreamSplitValuesWriter(
+ int length, int initialCapacity, int pageSize, ByteBufferAllocator
allocator) {
+ super(length, initialCapacity, pageSize, allocator);
+ this.length = length;
+ }
+
+ @Override
+ public final void writeBytes(Binary v) {
+ if (v.length() != length) {
Review Comment:
Use `assert` instead? This check is on the critical path.
##########
parquet-column/src/main/java/org/apache/parquet/column/values/bytestreamsplit/ByteStreamSplitValuesReaderForInteger.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * 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.parquet.column.values.bytestreamsplit;
+
+public class ByteStreamSplitValuesReaderForInteger extends
ByteStreamSplitValuesReader {
+ private static final int WIDTH = 4;
Review Comment:
nit: avoid a static variable by calling `super(4)` below.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
