-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce batched parquet column readers for flat types #14423
Conversation
Flat column reader sf1000 partitioned.pdf |
fc6a5db
to
b1458b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm % It should not count since I co-authored this
lib/trino-parquet/src/main/java/io/trino/parquet/reader/ColumnReaderFactory.java
Outdated
Show resolved
Hide resolved
plugin/trino-hive/src/test/java/io/trino/plugin/hive/parquet/TestParquetDecimalScaling.java
Outdated
Show resolved
Hide resolved
b1458b7
to
802cf5f
Compare
59135ac
to
868236e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are test failures.
Would be great to attach benchmark report
lib/trino-parquet/src/main/java/io/trino/parquet/reader/ColumnReader.java
Show resolved
Hide resolved
lib/trino-parquet/src/main/java/io/trino/parquet/reader/FilteredRowRanges.java
Show resolved
Hide resolved
lib/trino-parquet/src/main/java/io/trino/parquet/reader/FilteredRowRanges.java
Show resolved
Hide resolved
plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestParquetPageSkipping.java
Outdated
Show resolved
Hide resolved
...no-hive/src/test/java/io/trino/plugin/hive/TestParquetPageSkippingWithAcceleratedReader.java
Outdated
Show resolved
Hide resolved
Test failure looks unrelated but I'm re-running to confirm. |
868236e
to
07827fb
Compare
lib/trino-parquet/src/main/java/io/trino/parquet/reader/FilteredRowRanges.java
Show resolved
Hide resolved
lib/trino-parquet/src/main/java/io/trino/parquet/ParquetReaderUtils.java
Show resolved
Hide resolved
public static void unpack(byte[] values, int offset, long packedValue) | ||
{ | ||
values[offset] = (byte) (packedValue & 1); | ||
values[offset + 1] = (byte) ((packedValue >>> 1) & 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure this is better than a loop? What does the resulting assembly and perf look for this? How did we measure it? As in the comment above, the downside is more bytecode and fewer opportunities for inlining as a result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those unrollings have show significant improvements in jmh benchmarks and some visible (not huge obviously) gains in macrobenchmarks.
lib/trino-parquet/src/main/java/io/trino/parquet/reader/FilteredRowRanges.java
Outdated
Show resolved
Hide resolved
lib/trino-parquet/src/main/java/io/trino/parquet/reader/SimpleSliceInputStream.java
Outdated
Show resolved
Hide resolved
lib/trino-parquet/src/test/java/io/trino/parquet/reader/BenchmarkReadUleb128Int.java
Outdated
Show resolved
Hide resolved
throws Exception | ||
{ | ||
benchmark(BenchmarkReadUleb128Int.class) | ||
.withOptions(optionsBuilder -> optionsBuilder.jvmArgsAppend("-Xmx4g", "-Xms4g")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My guess is that this line has been mindlessly copied from one benchmark to another for centuries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is carried over from existing benchmark classes. I was assuming that using a fixed heap size somehow leads to more consistent results, but I don't know if that's actually the case.
16c3547
to
eabde80
Compare
9361f0c
to
b4bb82b
Compare
149d9e8
to
8cb5c1c
Compare
6cda3e5
to
1280c45
Compare
1280c45
to
ebff3e2
Compare
ebff3e2
to
f1a5f8a
Compare
f1a5f8a
to
22c4144
Compare
lib/trino-parquet/src/main/java/io/trino/parquet/reader/decoders/ValueDecoder.java
Outdated
Show resolved
Hide resolved
lib/trino-parquet/src/main/java/io/trino/parquet/reader/flat/DefinitionLevelDecoder.java
Outdated
Show resolved
Hide resolved
lib/trino-parquet/src/main/java/io/trino/parquet/reader/decoders/ApacheParquetValueDecoder.java
Outdated
Show resolved
Hide resolved
lib/trino-parquet/src/main/java/io/trino/parquet/reader/flat/FlatColumnReader.java
Outdated
Show resolved
Hide resolved
Introduce a standalone ColumnReaderFactory class along with ColumnReader interface Co-authored-by: Krzysztof Skrzypczynski <krzysztof.skrzypczynski@starburstdata.com>
9a393e8
to
e0d0fd2
Compare
NullsDecoder implements an optimized definition levels decoder for primitive columns where the definition level is either 0 or 1. ApacheParquetValueDecoders provides implementations of batched decoders which are wrappers over existing parquet-mr ValuesReader implementations. More optimized implementations of ValueDecoder will be added to ValueDecoders in subsequent changes. FilteredRowRangesIterator providers a way to iterate over rows selected by column index in a batch of positions at a time. FlatColumnReader makes use of the batched decoders and row-ranges iterator to implement an optimized ColumnReader with dedicated code paths for nullable and non-nullable types. ColumnReaderFactory is updated to use the optimized implementations for boolean, tinyint, short, int, long, float, double and short decimals. ColumnReaderFactory falls back to existing implementations where optimized implementations are not yet available or the flag parquet.optimized-reader.enabled is disabled. Co-authored-by: Raunaq Morarka <raunaqmorarka@gmail.com>
When a large enough number of rows are skipped due to `seek` operation, it is possible to skip decompressing and decoding parquet pages entirely.
Parquet schema may specify a column definition as OPTIONAL even though there are no nulls in the actual data. Row-group column statistics can be used to identify such cases and switch to faster non-nullable read paths in FlatColumnReader.
e0d0fd2
to
3b803f1
Compare
@raunaqmorarka I think we also need to add docs for this to the Hudi connector page. Not urgent, just let me know if you've got that covered or if you'd prefer me to follow up with that. |
Right, would be great if you could take care of that. |
Description
Non-technical explanation
Improve performance of reading Parquet files for boolean, tinyint, short, int, long, float, double and short decimal data types.
Release notes
( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text: