vinooganesh commented on code in PR #3397:
URL: https://github.com/apache/parquet-java/pull/3397#discussion_r3566426181
##########
parquet-column/src/main/java/org/apache/parquet/column/values/factory/DefaultV1ValuesWriterFactory.java:
##########
@@ -147,7 +148,13 @@ private ValuesWriter getInt96ValuesWriter(ColumnDescriptor
path) {
private ValuesWriter getDoubleValuesWriter(ColumnDescriptor path) {
final ValuesWriter fallbackWriter;
- if (this.parquetProperties.isByteStreamSplitEnabled(path)) {
+ if (this.parquetProperties.isAlpEnabled(path)) {
Review Comment:
Good call on the tests. I added coverage in DefaultValuesWriterFactoryTest
for both the float and double factories producing the ALP writers under V1 and
V2, including the per-column case and ALP taking precedence over byte stream
split.
On the placement question, I went back and forth but I'd like to keep ALP
registered in both V1 and V2 for now. It's a fallback data-page encoding and
doesn't rely on anything specific to the V2 page format, so there's no real
reason a PARQUET_1_0 writer can't use it, and a couple of the interop tests do
exercise ALP with V1 pages. It stays off by default either way, so nobody picks
it up unless they explicitly call withAlpEncoding. If the new versioning work
lands and we decide ALP should be pinned to a particular writer version, I'm
happy to move it then.
##########
parquet-column/src/main/java/org/apache/parquet/column/values/alp/AlpEncoderDecoder.java:
##########
@@ -0,0 +1,359 @@
+/*
+ * 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.*;
+
+/**
+ * Core ALP (Adaptive Lossless floating-Point) encoding and decoding logic.
+ *
+ * <p>ALP works by converting floating-point values to integers using decimal
scaling,
+ * then applying Frame of Reference encoding and bit-packing.
+ * Values that cannot be losslessly converted are stored as exceptions.
+ *
+ * <p>Encoding formula: encoded = fastRound(value * POW10[e] *
POW10_NEGATIVE[f])
+ * <p>Decoding formula: value = encoded * POW10[f] * POW10_NEGATIVE[e]
+ *
+ * <p>The order of operations is critical for IEEE 754 correctness. Both
formulas must
+ * be evaluated as single expressions — storing the intermediate
multiplication result
+ * in a variable before the second multiply changes IEEE 754 rounding and
produces extra
+ * exceptions. Uses multiply-by-reciprocal (via POW10_NEGATIVE) for C++ wire
compatibility.
Review Comment:
Fair point, that wording is misleading. ALP isn't in the parquet-format spec
yet (the linked PR is still the proposal), so there's no official byte layout
to reference. In practice the Arrow C++ implementation is what everyone is
matching against right now, which is why I called it out, but you're right that
"C++ wire compatibility" makes it sound language-specific when it really isn't.
What we actually care about is that the encoded bytes match the reference
layout, so a file written by one implementation reads back identically in
another. I'll reword this to talk about matching the reference layout rather
than singling out C++. Once the format PR merges we can point at the spec
instead.
##########
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:
Yep, exactly. This skip is only here because ALP isn't in the parquet-format
Encoding enum yet, so the round-trip check has nothing to compare against. Once
the format change is released and we pick up the new enum value, the guard
comes out and ALP goes through the same check as every other encoding. I left
the note so we don't forget, and I'm tracking it against the format PR.
##########
parquet-column/src/main/java/org/apache/parquet/column/values/alp/AlpValuesReader.java:
##########
@@ -0,0 +1,178 @@
+/*
+ * 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.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
+import org.apache.parquet.bytes.ByteBufferInputStream;
+import org.apache.parquet.column.values.ValuesReader;
+import org.apache.parquet.io.ParquetDecodingException;
+
+/**
+ * Abstract base class for ALP values readers with lazy per-vector decoding.
+ *
+ * <p>Reads ALP-encoded values from the interleaved page layout:
+ * <pre>
+ * ┌─────────┬──────────────────────┬──────────────┬──────────────┬─────┐
+ * │ Header │ Offset Array │ Vector 0 │ Vector 1 │ ... │
+ * │ 7 bytes │ 4B × numVectors │ (interleaved)│ (interleaved)│ │
+ * └─────────┴──────────────────────┴──────────────┴──────────────┴─────┘
+ * </pre>
+ *
+ * <p>Each vector is decoded lazily on first access. Skipping values does not
+ * trigger decoding of intermediate vectors.
+ */
+abstract class AlpValuesReader extends ValuesReader {
+
+ protected int vectorSize;
+ protected int totalCount;
+ protected int numVectors;
+ protected int currentIndex;
+ protected int currentVectorIndex;
+
+ protected int[] vectorOffsets;
+ protected ByteBuffer vectorsData;
+ protected int offsetArraySize;
+
+ AlpValuesReader() {
+ this.currentIndex = 0;
+ this.totalCount = 0;
+ this.currentVectorIndex = -1;
+ }
+
+ @Override
+ public void initFromPage(int valuesCount, ByteBufferInputStream stream)
+ throws ParquetDecodingException, IOException {
+ ByteBuffer headerBuf =
stream.slice(ALP_HEADER_SIZE).order(ByteOrder.LITTLE_ENDIAN);
+ int compressionMode = headerBuf.get() & 0xFF;
+ int integerEncoding = headerBuf.get() & 0xFF;
+ int logVectorSize = headerBuf.get() & 0xFF;
+ int numElements = headerBuf.getInt();
+
+ if (compressionMode != ALP_COMPRESSION_MODE) {
+ throw new ParquetDecodingException("Unsupported ALP compression mode: "
+ compressionMode);
+ }
+ if (integerEncoding != ALP_INTEGER_ENCODING_FOR) {
+ throw new ParquetDecodingException("Unsupported ALP integer encoding: "
+ integerEncoding);
+ }
+ if (logVectorSize < MIN_LOG_VECTOR_SIZE || logVectorSize >
MAX_LOG_VECTOR_SIZE) {
+ throw new ParquetDecodingException("Invalid ALP log vector size: " +
logVectorSize + ", must be between "
+ + MIN_LOG_VECTOR_SIZE + " and " + MAX_LOG_VECTOR_SIZE);
+ }
+ if (numElements < 0) {
+ throw new ParquetDecodingException("Invalid ALP element count: " +
numElements);
+ }
+ // ALP's num_elements is the count of non-null values that went through
encoding;
+ // valuesCount is the page row count, which is larger when the column has
nulls.
+ // The two are equal only for required (non-null) columns.
+ if (numElements > valuesCount) {
+ throw new ParquetDecodingException(
+ "ALP header element count " + numElements + " exceeds page
valuesCount " + valuesCount);
+ }
+
+ this.vectorSize = 1 << logVectorSize;
+ this.totalCount = numElements;
+ this.numVectors = (numElements + vectorSize - 1) / vectorSize;
+ this.currentIndex = 0;
+ this.currentVectorIndex = -1;
+
+ this.offsetArraySize = numVectors * Integer.BYTES;
+ ByteBuffer offsetBuf =
stream.slice(offsetArraySize).order(ByteOrder.LITTLE_ENDIAN);
+ this.vectorOffsets = new int[numVectors];
+ for (int v = 0; v < numVectors; v++) {
+ vectorOffsets[v] = offsetBuf.getInt();
+ }
+
+ // Slice remaining bytes into a 0-based view so decodeVector can use
+ // absolute get methods (vectorsData.get(pos)) directly.
+ int remainingBytes = (int) stream.available();
+ ByteBuffer rawSlice = stream.slice(remainingBytes);
Review Comment:
Right, that's the all-null case. When every value on the page is null
nothing reaches the writer, so num_elements is 0, numVectors works out to 0,
offsetArraySize is 0, and we end up calling stream.slice(0) with empty offset
and vector buffers. Nothing gets decoded and the reader just reports 0 values,
which is what we want, so it already handles it correctly.
I didn't have an explicit all-null test at the reader level, so I added
testFloatAllNullPage and testDoubleAllNullPage to lock this in.
--
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]
