gszadovszky commented on code in PR #1291: URL: https://github.com/apache/parquet-mr/pull/1291#discussion_r1516226543
########## parquet-column/src/main/java/org/apache/parquet/column/values/bytestreamsplit/ByteStreamSplitValuesReaderForInteger.java: ########## @@ -0,0 +1,37 @@ +/* + * 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; + +import static org.apache.parquet.bytes.BytesUtils.bytesToInt; + +public class ByteStreamSplitValuesReaderForInteger extends ByteStreamSplitValuesReader { + private static final int width = 4; Review Comment: nit: the preferred java format expects to have constants (`static final`) with upper case: `WIDTH`. ########## parquet-column/src/main/java/org/apache/parquet/column/ParquetProperties.java: ########## @@ -420,8 +429,27 @@ public Builder withDictionaryEncoding(String columnPath, boolean enableDictionar return this; } - public Builder withByteStreamSplitEncoding(boolean enableByteStreamSplit) { - this.enableByteStreamSplit = enableByteStreamSplit; + /** + * Enable or disable BYTE_STREAM_SPLIT encoding for FLOAT and DOUBLE columns. + * + * @param enable whether BYTE_STREAM_SPLIT encoding should be enabled + * @return this builder for method chaining. + */ + public Builder withByteStreamSplitEncoding(boolean enable) { + this.enableByteStreamSplit = enable; + this.enableExtendedByteStreamSplit &= enable; + return this; + } + + /** + * Enable or disable BYTE_STREAM_SPLIT encoding for FLOAT, DOUBLE, INT32, INT64 and FIXED_LEN_BYTE_ARRAY columns. + * + * @param enable whether BYTE_STREAM_SPLIT encoding should be enabled + * @return this builder for method chaining. + */ + public Builder withExtendedByteStreamSplitEncoding(boolean enable) { Review Comment: WDYT about handling it like we do for dictionary encoding. Dictionary can be enabled/disabled globally or per specific columns. It would help experimenting. ########## parquet-column/src/main/java/org/apache/parquet/column/values/bytestreamsplit/ByteStreamSplitValuesReaderForFLBA.java: ########## @@ -0,0 +1,36 @@ +/* + * 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; + +import org.apache.parquet.io.api.Binary; + +public class ByteStreamSplitValuesReaderForFLBA extends ByteStreamSplitValuesReader { + private final byte[] valueByteBuffer; + + public ByteStreamSplitValuesReaderForFLBA(int length) { + super(length); + valueByteBuffer = new byte[length]; + } + + @Override + public Binary readBytes() { + gatherElementDataFromStreams(valueByteBuffer); + return Binary.fromConstantByteArray(valueByteBuffer); Review Comment: This does not seem to be correct. Using `Binary.fromConstantByteArray` means there will be no copy of the `byte[]` inside so by reading the next value you will rewrite the content of the previously returned `Binary` objects as well. I would suggest using `Binary.fromReusedByteArray` instead. ########## parquet-column/src/main/java/org/apache/parquet/column/values/factory/DefaultV2ValuesWriterFactory.java: ########## @@ -102,19 +125,39 @@ private ValuesWriter getBinaryValuesWriter(ColumnDescriptor path) { } private ValuesWriter getInt32ValuesWriter(ColumnDescriptor path) { - ValuesWriter fallbackWriter = new DeltaBinaryPackingValuesWriterForInteger( - parquetProperties.getInitialSlabSize(), - parquetProperties.getPageSizeThreshold(), - parquetProperties.getAllocator()); + final ValuesWriter fallbackWriter; + // Ideally we should only enable BYTE_STREAM_SPLIT if compression is enabled for this column. + // However, this information is not available here. Review Comment: It is passed down from `ParquetWriter`/`ParquetRecordWriter` so defined per file. I agree it would be nice to have it here but currently I cannot imagine how could we do it. Feel free to leave this comment here as a TODO, however, since we currently have it explicitly configure by the client, I don't think we need it. ########## parquet-column/src/main/java/org/apache/parquet/column/values/bytestreamsplit/ByteStreamSplitValuesReaderForLong.java: ########## @@ -0,0 +1,37 @@ +/* + * 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; + +import static org.apache.parquet.bytes.BytesUtils.bytesToLong; + +public class ByteStreamSplitValuesReaderForLong extends ByteStreamSplitValuesReader { + private static final int width = 8; Review Comment: upper case... ########## parquet-column/src/main/java/org/apache/parquet/column/values/factory/DefaultV2ValuesWriterFactory.java: ########## @@ -84,10 +92,25 @@ private ValuesWriter getBooleanValuesWriter() { } private ValuesWriter getFixedLenByteArrayValuesWriter(ColumnDescriptor path) { - ValuesWriter fallbackWriter = new DeltaByteArrayWriter( - parquetProperties.getInitialSlabSize(), - parquetProperties.getPageSizeThreshold(), - parquetProperties.getAllocator()); + final ValuesWriter fallbackWriter; + // Heuristic: enable BYTE_STREAM_SPLIT for DECIMAL and FLOAT16 columns. + final boolean useByteStreamSplit = parquetProperties.isExtendedByteStreamSplitEnabled() Review Comment: Why the different heuristics for V1 and V2? Do we know if `BYTE_STREAM_SPLIT` would be better for `DECIMAL`/`FLOAT16` values than `DELTA_BYTE_ARRAY`? It would be nice to come up with some smart heuristics instead of having the client set it, but if we don't know which one is better in certain cases, I would vote on not having any heuristics. -- 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]
