Mihaly Szjatinya has posted comments on this change. ( http://gerrit.cloudera.org:8080/24579 )
Change subject: IMPALA-15139: Support DELTA_LENGTH_BYTE_ARRAY Parquet encoding ...................................................................... Patch Set 18: (11 comments) http://gerrit.cloudera.org:8080/#/c/24579/16/be/src/benchmarks/parquet-delta-length-byte-array-benchmark.cc File be/src/benchmarks/parquet-delta-length-byte-array-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/24579/16/be/src/benchmarks/parquet-delta-length-byte-array-benchmark.cc@18 PS16, Line 18: // Benchmarks for ParquetDeltaLengthByteArrayDecoder, covering: > Thanks for adding the benchmarks! One thing I would add is checking the sam Added a Delta-vs-Plain scenario (scenario 5) on the same data: reads are somewhat slower than Plain (batch ~1.8x, 1-by-1 ~3.7x), and the gap is largest for skipping (~9.4x) indeed. The strings are mixed: long and smallified. http://gerrit.cloudera.org:8080/#/c/24579/16/be/src/benchmarks/parquet-delta-length-byte-array-benchmark.cc@31 PS16, Line 31: // all-long 146 iters/ms 1X (zero-copy: only pointer stored, no Smallify) : // mixed M-L 137 iters/ms 0.94X (short-first phase: S,L,S,L) > This looks a bit surprising - if smallifying has such a large effect, then Did some experiments on this. The deviation comes purely from the ORDER of the alternating long/short strings, not from anything about the mix itself. Both mixed cases have the identical 50/50 multiset and byte content; they differ only in which length comes first: S,L,S,L... (mixed M-L) vs L,S,L,S... (mixed L-M). That alone reproducibly shifts NextValues throughput by ~15% (0.94X vs 0.80X relative to all-long). To make sure it wasn't an artifact I ran the two phases as a same-run A/B, pinned to a single P-core (taskset -c 0), repeated several times; the ~15% gap is stable to +/-0.5%. Interestingly, at cold start the two phases are about equal; the gap only shows up once the core warms up to its full turbo frequency (inferred from throughput ramping up and plateauing over the first few runs). http://gerrit.cloudera.org:8080/#/c/24579/16/be/src/benchmarks/parquet-delta-length-byte-array-benchmark.cc@254 PS16, Line 254: int s = dec.SkipValu > This looks unintuitive - what will it check? My guess is that that the resu The out_buf is overwritten each time a Bench_* function fires, but it is irrelevant, since CheckCorrect always does its own fresh NewPage() + decode before reading out_buf. I.e. it is not checking the results of the last benchmark run — it only verifies that the decoder produces correct output for a given PageData. It is kind of a post-run integrity check, not strictly needed, since the unit tests already cover this. Note: the previous comment on this function said "prevents the compiler from eliminating decode work", which was incorrect — CheckCorrect only provides that guarantee for its own internal decode, not for the timed Bench_* loops. That may have been a source of confusion here. Renamed CheckCorrect to VerifyPage to make the intent less ambiguous. http://gerrit.cloudera.org:8080/#/c/24579/16/be/src/benchmarks/parquet-delta-length-byte-array-benchmark.cc@282 PS16, Line 282: return; > Why not check all? Done, added the missing two. http://gerrit.cloudera.org:8080/#/c/24579/16/be/src/exec/parquet/parquet-delta-length-byte-array-decoder.cc File be/src/exec/parquet/parquet-delta-length-byte-array-decoder.cc: http://gerrit.cloudera.org:8080/#/c/24579/16/be/src/exec/parquet/parquet-delta-length-byte-array-decoder.cc@54 PS16, Line 54: > nit: the header is not trusted more than the payload, this is just a sanity Ack http://gerrit.cloudera.org:8080/#/c/24579/16/be/src/exec/parquet/parquet-delta-length-byte-array-decoder.cc@64 PS16, Line 64: ing block > This could be done faster than doing SkipValues, which needs to count the a Agree, added a TODO. http://gerrit.cloudera.org:8080/#/c/24579/16/be/src/exec/parquet/parquet-delta-length-byte-array-decoder.cc@120 PS16, Line 120: DCHECK_GE(num_values, 0); > nit: could be UNLIKELY Ack http://gerrit.cloudera.org:8080/#/c/24579/16/be/src/exec/parquet/parquet-delta-length-byte-array-decoder.cc@125 PS16, Line 125: > nit: could be UNLIKELY Ack http://gerrit.cloudera.org:8080/#/c/24579/16/be/src/exec/parquet/parquet-delta-length-byte-array-decoder.cc@151 PS16, Line 151: DCHECK_GE(num_values, 0); > nit: could be UNLIKELY Ack http://gerrit.cloudera.org:8080/#/c/24579/16/be/src/exec/parquet/parquet-delta-length-byte-array-decoder.cc@156 PS16, Line 156: > nit: could be UNLIKELY Ack http://gerrit.cloudera.org:8080/#/c/24579/16/testdata/parquet_delta_length_byte_array_encoding/parquet_files_generator.py File testdata/parquet_delta_length_byte_array_encoding/parquet_files_generator.py: http://gerrit.cloudera.org:8080/#/c/24579/16/testdata/parquet_delta_length_byte_array_encoding/parquet_files_generator.py@117 PS16, Line 117: With default PyArrow page size all 2000 lengths land in one data page > It would be nice to also check the multiple page scenario. Done, and added a test. -- To view, visit http://gerrit.cloudera.org:8080/24579 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9e4395fa30cd7fb2c89ef5a2b16b043d4de60b52 Gerrit-Change-Number: 24579 Gerrit-PatchSet: 18 Gerrit-Owner: Mihaly Szjatinya <[email protected]> Gerrit-Reviewer: Balazs Hevele <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Mihaly Szjatinya <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Sun, 30 Aug 2026 15:37:14 +0000 Gerrit-HasComments: Yes
