vinooganesh commented on code in PR #3397:
URL: https://github.com/apache/parquet-java/pull/3397#discussion_r3942060684
##########
parquet-column/src/main/java/org/apache/parquet/column/ParquetProperties.java:
##########
@@ -585,6 +637,74 @@ public Builder withExtendedByteStreamSplitEncoding(boolean
enable) {
return this;
}
+ /**
+ * Enable or disable ALP encoding for FLOAT and DOUBLE columns.
+ *
+ * @param enable whether ALP encoding should be enabled
+ * @return this builder for method chaining.
+ */
+ public Builder withAlpEncoding(boolean enable) {
+ this.alpEnabled.withDefaultValue(enable);
+ return this;
+ }
+
+ /**
+ * Enable or disable ALP encoding for the specified column.
+ *
+ * @param columnPath the path of the column (dot-string)
+ * @param enable whether ALP encoding should be enabled
+ * @return this builder for method chaining.
+ */
+ public Builder withAlpEncoding(String columnPath, boolean enable) {
+ this.alpEnabled.withValue(columnPath, enable);
+ return this;
+ }
+
+ /**
+ * Set the ALP vector size (number of values per encoded vector) for FLOAT
and DOUBLE columns.
+ * Must be a power of 2 in the range supported by {@link AlpConstants}.
+ *
+ * @param vectorSize the vector size
+ * @return this builder for method chaining.
+ */
+ public Builder withAlpVectorSize(int vectorSize) {
Review Comment:
You're right that this was more complicated than it needed to be, thanks for
pushing on it. The builder was keeping the enabled flag and the vector size as
two separate ColumnProperty scaffolds, merging them into an AlpConfig in
buildAlp(), and then splitting them apart again in the copy constructor.
Nothing actually needed that round trip.
Fixed in 68c103b84. There is now a single ColumnProperty<AlpConfig>, with
withAlp(AlpConfig) and withAlp(columnPath, AlpConfig) as the primary API. I
kept withAlpEncoding and withAlpVectorSize as thin read-modify-write
conveniences on top, since they match the ergonomics of
withByteStreamSplitEncoding and it means no existing caller had to change,
including ParquetWriter.Builder. buildAlp and the copy constructor split are
both gone.
I also moved the vector size validation into the AlpConfig constructor,
which felt like the right home for it since an invalid size should be rejected
however the config was built. That had the nice side effect of removing the
last reference to AlpConstants from ParquetProperties, which made your
visibility comment straightforward to act on.
One thing I want to flag for you. To let the convenience setters modify an
existing config rather than replace it, I added getDefaultValue and getValue to
ColumnProperty.Builder. That class is package private so it is not public API,
but it is a shared file rather than something ALP specific, so please say if
you would rather I found another way around it.
##########
parquet-column/src/main/java/org/apache/parquet/column/values/alp/AlpConstants.java:
##########
@@ -0,0 +1,115 @@
+/*
+ * 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 org.apache.parquet.Preconditions;
+
+/**
+ * Constants for the ALP (Adaptive Lossless floating-Point) encoding.
+ *
+ * <p>ALP encoding converts floating-point values to integers using decimal
scaling,
+ * then applies Frame of Reference encoding and bit-packing.
+ * Values that cannot be losslessly converted are stored as exceptions.
+ *
+ * <p>Based on the paper: "ALP: Adaptive Lossless floating-Point Compression"
(SIGMOD 2024)
+ *
+ * @see <a href="https://dl.acm.org/doi/10.1145/3626717";>ALP Paper</a>
+ */
+public final class AlpConstants {
Review Comment:
Good question, and the answer is that it did not need to be public. I
checked every reference and ParquetProperties was the only thing outside the
package touching it, for DEFAULT_VECTOR_SIZE and validateVectorSize. Once the
AlpConfig change above removed both of those, nothing external was left.
The class and all of its members are package private now, in 6872c2abd. It
seemed worth doing properly before a release rather than after, since it keeps
the ALP internals out of the public API surface permanently.
##########
parquet-column/src/main/java/org/apache/parquet/column/values/alp/AlpConstants.java:
##########
@@ -0,0 +1,115 @@
+/*
+ * 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 org.apache.parquet.Preconditions;
+
+/**
+ * Constants for the ALP (Adaptive Lossless floating-Point) encoding.
+ *
+ * <p>ALP encoding converts floating-point values to integers using decimal
scaling,
+ * then applies Frame of Reference encoding and bit-packing.
+ * Values that cannot be losslessly converted are stored as exceptions.
+ *
+ * <p>Based on the paper: "ALP: Adaptive Lossless floating-Point Compression"
(SIGMOD 2024)
+ *
+ * @see <a href="https://dl.acm.org/doi/10.1145/3626717";>ALP Paper</a>
+ */
+public final class AlpConstants {
+
+ private AlpConstants() {
+ // Utility class
+ }
+
+ // Page header fields
+ public static final int ALP_COMPRESSION_MODE = 0;
+ public static final int ALP_INTEGER_ENCODING_FOR = 0;
+ public static final int ALP_HEADER_SIZE = 7;
+
+ public static final int DEFAULT_VECTOR_SIZE = 1024;
+ public static final int DEFAULT_VECTOR_SIZE_LOG = 10;
+
+ // BytePacker packs/unpacks 8 values at a time (pack8Values/unpack8Values).
+ static final int PACK_GROUP_SIZE = 8;
+
+ // Capped at 15 (vectorSize=32768) because num_exceptions is uint16,
+ // so vectorSize must not exceed 65535 to avoid overflow when all values are
exceptions.
+ static final int MAX_LOG_VECTOR_SIZE = 15;
+ static final int MIN_LOG_VECTOR_SIZE = 3;
+
+ static final int FLOAT_MAX_EXPONENT = 10;
+ static final int DOUBLE_MAX_EXPONENT = 18;
+
+ // Sampler constants matching C++ AlpConstants.
+ // Sample SAMPLER_SAMPLE_VECTORS_PER_ROWGROUP vectors evenly distributed
across a rowgroup
+ // of SAMPLER_ROWGROUP_SIZE values, then lock in top MAX_PRESET_COMBINATIONS
combos.
+ static final int SAMPLER_ROWGROUP_SIZE = 122_880;
Review Comment:
Agreed, thank you. I went through each constant and sorted them by who
actually reads them, in 6872c2abd.
The sampler constants (SAMPLER_ROWGROUP_SIZE,
SAMPLER_SAMPLE_VECTORS_PER_ROWGROUP, MAX_PRESET_COMBINATIONS) are only used by
the writer, so they moved to AlpValuesWriter. The rounding magic numbers and
the powers of ten tables are only used by the codec, so they moved to AlpCodec.
What is left in AlpConstants is genuinely shared and all of it describes the
wire format: the header and metadata sizes, the mode and encoding markers, the
vector size bounds, and the per type exponent limits. It reads much better as a
wire format definition than it did as a general bucket.
##########
parquet-column/src/main/java/org/apache/parquet/column/values/alp/AlpEncoderDecoder.java:
##########
@@ -0,0 +1,302 @@
+/*
+ * 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 org.apache.parquet.bytes.BytesUtils;
+
+/**
+ * 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. Likewise, scaling uses multiply-by-reciprocal (via
POW10_NEGATIVE) rather than
+ * division: this reproduces the exact IEEE 754 rounding of the ALP reference
algorithm, so the
+ * encoded integers — and therefore which values become exceptions and the
resulting bytes — are
+ * identical across implementations. It is about cross-implementation
determinism, not any one
+ * language.
+ *
+ * <p>Exception conditions:
+ * <ul>
+ * <li>NaN values</li>
+ * <li>Infinity values</li>
+ * <li>Negative zero (-0.0)</li>
+ * <li>Out of integer range</li>
+ * <li>Round-trip failure (decode(encode(v)) != v)</li>
+ * </ul>
+ */
+final class AlpEncoderDecoder {
Review Comment:
Happy to take this. Renamed to AlpCodec in 6872c2abd, along with its test. I
went with AlpCodec over AlpUtil since it really is a codec rather than a
collection of helpers.
##########
parquet-column/src/main/java/org/apache/parquet/column/values/alp/AlpEncoderDecoder.java:
##########
@@ -0,0 +1,302 @@
+/*
+ * 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.*;
Review Comment:
Understood, and sorry for the noise. All five files now use explicit static
imports, in 6872c2abd. I did this last on purpose, after the constants had been
moved to their new homes, so that the import lists reflect where things ended
up rather than needing a second pass.
##########
parquet-column/src/main/java/org/apache/parquet/column/values/alp/AlpValuesWriter.java:
##########
@@ -0,0 +1,647 @@
+/*
+ * 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.nio.ByteOrder;
+import java.util.ArrayList;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.parquet.bytes.ByteBufferAllocator;
+import org.apache.parquet.bytes.BytesInput;
+import org.apache.parquet.bytes.BytesUtils;
+import org.apache.parquet.bytes.CapacityByteArrayOutputStream;
+import org.apache.parquet.column.Encoding;
+import org.apache.parquet.column.values.ValuesWriter;
+import org.apache.parquet.column.values.bitpacking.BytePacker;
+import org.apache.parquet.column.values.bitpacking.BytePackerForLong;
+import org.apache.parquet.column.values.bitpacking.Packer;
+
+/**
+ * ALP (Adaptive Lossless floating-Point) values writer.
+ *
+ * <p>ALP encoding converts floating-point values to integers using decimal
scaling,
+ * then applies Frame of Reference encoding and bit-packing.
+ * Values that cannot be losslessly converted are stored as exceptions.
+ *
+ * <p>Writing is incremental: values are buffered in a fixed-size vector
buffer,
+ * and each full vector is encoded and flushed to the output stream
immediately.
+ * On {@link #getBytes()}, any remaining partial vector is flushed, and the
+ * final page bytes are assembled.
+ *
+ * <p>Interleaved Page Layout:
+ * <pre>
+ * ┌─────────┬──────────────────────┬──────────────┬──────────────┬─────┐
+ * │ Header │ Offset Array │ Vector 0 │ Vector 1 │ ... │
+ * │ 7 bytes │ 4B × numVectors │ (interleaved)│ (interleaved)│ │
Review Comment:
Thanks for spotting that. The cause is that the cell contains `4B ×
numVectors`, and the HTML entity is seven characters in the source but renders
as a single glyph, so the box could never line up in both your editor and the
rendered javadoc at the same time.
Fixed in 6872c2abd by using the literal character and padding the cell to
the right width, so every row is 73 characters in both views. The same diagram
had been copied into AlpValuesReader with the same problem, so I fixed it there
too.
##########
parquet-format-structures/pom.xml:
##########
@@ -66,6 +66,35 @@
</execution>
</executions>
</plugin>
+ <!--
Review Comment:
Your timing on this was good. #3709 merged a couple of days after you left
the comment, so parquet.thrift is now inlined in the repo and I was able to
take the approach you suggested.
In 3bc0835bf, ALP = 10 is declared directly in the Encoding enum, and the
perl script and the exec-maven-plugin execution that ran it are both gone. I
verified the generated Encoding.java still carries ALP(10) after
BYTE_STREAM_SPLIT(9) with no patch step involved.
There is one wrinkle I want your opinion on. dev/update-parquet-thrift.sh
overwrites parquet.thrift wholesale from upstream, so the next time anyone runs
it the ALP entry will disappear silently. It also rewrites the
parquet-format.version sidecar, so a note there would not survive either. For
now I have put a clearly marked notice in the enum itself saying it is a local
addition that needs re-applying until ALP is accepted into parquet-format, on
the grounds that at least the loss shows up in a diff. If you would prefer this
handled a different way, such as a guard in the update script, I am glad to
change it.
--
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]
