vinooganesh commented on code in PR #3397:
URL: https://github.com/apache/parquet-java/pull/3397#discussion_r3964216004
##########
parquet-column/src/main/java/org/apache/parquet/column/ParquetProperties.java:
##########
@@ -51,6 +53,8 @@ public class ParquetProperties {
public static final int DEFAULT_DICTIONARY_PAGE_SIZE = DEFAULT_PAGE_SIZE;
public static final boolean DEFAULT_IS_DICTIONARY_ENABLED = true;
public static final boolean DEFAULT_IS_BYTE_STREAM_SPLIT_ENABLED = false;
+ public static final boolean DEFAULT_IS_ALP_ENABLED = false;
+ public static final int DEFAULT_ALP_VECTOR_SIZE =
AlpConfig.DEFAULT_VECTOR_SIZE;
Review Comment:
Agreed, and thanks for flagging it — there was no reason to alias the
constant. Removed in 7377588ef, so the call site now reads
`AlpConfig.DEFAULT_VECTOR_SIZE` directly.
##########
parquet-column/src/main/java/org/apache/parquet/column/values/alp/AlpConfig.java:
##########
@@ -0,0 +1,89 @@
+/*
+ * 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 java.util.Objects;
+
+/**
+ * Immutable per-column ALP encoding configuration: whether ALP is enabled and
the vector size
+ * (number of values per encoded vector) to use. Bundled together so a column
carries a single
+ * cohesive ALP setting rather than several independent properties.
+ */
+public final class AlpConfig {
+
+ /** Default values per encoded vector. */
+ public static final int DEFAULT_VECTOR_SIZE =
AlpConstants.DEFAULT_VECTOR_SIZE;
+
+ /** ALP disabled, with the default vector size. */
+ public static final AlpConfig DISABLED = new AlpConfig(false,
DEFAULT_VECTOR_SIZE);
+
+ private final boolean enabled;
+ private final int vectorSize;
+
+ /**
+ * @param enabled whether ALP encoding is enabled
+ * @param vectorSize values per encoded vector; must be a power of 2 in the
supported range
+ * @throws IllegalArgumentException if {@code vectorSize} is not a supported
vector size
+ */
+ public AlpConfig(boolean enabled, int vectorSize) {
Review Comment:
That separation makes sense, thank you. Fixed in e3b846a2b, where
`AlpConfig` holds only the vector size and a column is ALP encoded when it has
a config at all.
##########
parquet-column/src/main/java/org/apache/parquet/column/ParquetProperties.java:
##########
@@ -468,6 +504,8 @@ private Builder() {
DEFAULT_IS_BYTE_STREAM_SPLIT_ENABLED
? ByteStreamSplitMode.FLOATING_POINT
: ByteStreamSplitMode.NONE);
+ alp = ColumnProperty.<AlpConfig>builder()
Review Comment:
Good catch, it was silently dropping BYTE_STREAM_SPLIT. Both that conflict
and ALP on a non float or double column are now rejected in
`newColumnWriteStore` in e660c6cec, rather than resolved by precedence.
##########
parquet-column/src/main/java/org/apache/parquet/column/ParquetProperties.java:
##########
@@ -585,6 +624,77 @@ public Builder withExtendedByteStreamSplitEncoding(boolean
enable) {
return this;
}
+ /**
+ * Set the full ALP configuration for FLOAT and DOUBLE columns.
+ *
+ * @param config the ALP configuration
+ * @return this builder for method chaining.
+ */
+ public Builder withAlp(AlpConfig config) {
Review Comment:
Your sketch is what the API looks like now, thanks for laying it out:
e3b846a2b collapses the six methods into `withAlp()`, `withAlp(config)`,
`withAlp(columnPath)`, `withAlp(columnPath, config)` and `withoutAlp()`, with
the vector size only reachable through `AlpConfig`. The non float or double
check is in e660c6cec, and the one tradeoff worth flagging is that per-column
disabling goes away with the boolean setters, so let me know if you would like
that kept.
--
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]
