vinooganesh commented on code in PR #3397: URL: https://github.com/apache/parquet-java/pull/3397#discussion_r3944056151
########## parquet-column/src/main/java/org/apache/parquet/column/values/alp/AlpValuesReaderForDouble.java: ########## @@ -0,0 +1,155 @@ +/* + * 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.alp; + +import static org.apache.parquet.column.values.alp.AlpConstants.*; + +import java.nio.ByteBuffer; +import java.util.Arrays; +import org.apache.parquet.column.values.bitpacking.BytePackerForLong; +import org.apache.parquet.column.values.bitpacking.Packer; +import org.apache.parquet.io.ParquetDecodingException; + +/** + * ALP values reader for DOUBLE type with lazy per-vector decoding. + * + * <p>Reads ALP-encoded double values from the interleaved page layout. + * Each vector is decoded on first access using BytePackerForLong-based unpacking. + */ +public class AlpValuesReaderForDouble extends AlpValuesReader { + + private double[] decodedValues; + private long[] deltasBuffer; + private int[] excPositionsBuffer; + private final long[] unpackPadBuf = new long[8]; + private byte[] unpackByteBuf; + + public AlpValuesReaderForDouble() { + super(); + } + + @Override + protected void allocateDecodedBuffer(int capacity) { + this.decodedValues = new double[capacity]; + this.deltasBuffer = new long[capacity]; + this.excPositionsBuffer = new int[capacity]; + this.unpackByteBuf = new byte[Long.SIZE]; // max bit width for long = 64 bytes + } + + @Override + public double readDouble() { + if (currentIndex >= totalCount) { + throw new ParquetDecodingException("ALP double data was already exhausted."); + } + ensureVectorDecoded(); + int indexInVector = currentIndex % vectorSize; Review Comment: Sorry for the slow reply on this one. I agree with Russell's read: this runs once per vector rather than once per value, so at the default vector size of 1024 it is well off the hot path, and it has not shown up as significant in the benchmarks. The power of two property is there if we ever want it, but I would rather not add the indirection until a profile says it matters. For what it is worth, the per value paths are where the time actually goes, and I did find real duplicated work there recently: the exception check was encoding each value and then the caller encoded it again. That is fixed in 8e743c60a. ########## parquet-hadoop/src/test/java/org/apache/parquet/format/converter/TestParquetMetadataConverter.java: ########## @@ -471,6 +471,10 @@ public void testLogicalToConvertedTypeConversion() { public void testEnumEquivalence() { ParquetMetadataConverter parquetMetadataConverter = new ParquetMetadataConverter(); for (org.apache.parquet.column.Encoding encoding : org.apache.parquet.column.Encoding.values()) { + // Skip ALP encoding as it's not yet in the parquet-format specification Review Comment: Following up on this, since the situation has changed since I last replied. #3709 inlined parquet.thrift into the repo, so in 229e0d99b I was able to declare ALP = 10 in the Encoding enum directly and delete the perl script that used to patch the generated code. As a side effect the enum round trip now works, and I confirmed testEnumEquivalence passes with the skip removed. I have left the skip in place for the moment though, because the condition you were actually pointing at has not really been met. ALP is in our inlined copy of the thrift file, not in released parquet-format, so removing the guard would make this test depend on a local addition. The tradeoff is that with the guard gone, anything that drops the ALP entry (dev/update-parquet-thrift.sh overwrites the file from upstream) turns into a loud test failure instead of a silent skip, which is arguably what we want. Happy to go either way. Let me know if you would rather I take the guard out now, and otherwise it comes out when ALP is accepted upstream, as originally planned. -- 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]
