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 12: (13 comments) @Csaba, thanks for review. Many great points. http://gerrit.cloudera.org:8080/#/c/24579/10/be/src/exec/parquet/parquet-column-readers.cc File be/src/exec/parquet/parquet-column-readers.cc: http://gerrit.cloudera.org:8080/#/c/24579/10/be/src/exec/parquet/parquet-column-readers.cc@399 PS10, Line 399: > I would prefer not to duplicate the code here (see my comment at line 469). Yes, absolutely http://gerrit.cloudera.org:8080/#/c/24579/10/be/src/exec/parquet/parquet-column-readers.cc@469 PS10, Line 469: } Yes, it's much better. > An even nicer way to do this could be adding a constexpr function that checks > if an encoding+parquet type pair is supported and filter unneeded ifs in > compile time. Yeah it'd be good to somehow extract all of the dispatching logic for all template functions and have it in one place. http://gerrit.cloudera.org:8080/#/c/24579/10/be/src/exec/parquet/parquet-column-readers.cc@503 PS10, Line 503: // Not materializing anything - skip decoding any levels and rely on the value > could add condition for PARQUET_TYPE - this is a very perf critical functio Ack http://gerrit.cloudera.org:8080/#/c/24579/10/be/src/exec/parquet/parquet-column-readers.cc@772 PS10, Line 772: return true; > could filter on PARQUET_TYPE Ack http://gerrit.cloudera.org:8080/#/c/24579/10/be/src/exec/parquet/parquet-column-readers.cc@1019 PS10, Line 1019: return false; > could filter on PARQUET_TYPE Ack http://gerrit.cloudera.org:8080/#/c/24579/10/be/src/exec/parquet/parquet-delta-length-byte-array-decoder.h File be/src/exec/parquet/parquet-delta-length-byte-array-decoder.h: http://gerrit.cloudera.org:8080/#/c/24579/10/be/src/exec/parquet/parquet-delta-length-byte-array-decoder.h@57 PS10, Line 57: /// Only valid after a successful call to NewPage(). : int GetTotalValueCount() const { : DCHECK(initialized_); : return total_value_count_; : } : : /// Only valid after a successful call to NewPage(). : int NextValue(StringValue* out) WARN_UNUSED_RESULT; : > IMO these functions could benefit from adding RESTRICT to them and their po Ok, filed IMPALA-15250. http://gerrit.cloudera.org:8080/#/c/24579/10/be/src/exec/parquet/parquet-delta-length-byte-array-decoder.h@80 PS10, Line 80: efills len > naming: has_only_small_ would be clearer, the current name suggest that it Ack http://gerrit.cloudera.org:8080/#/c/24579/10/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/10/be/src/exec/parquet/parquet-delta-length-byte-array-decoder.cc@48 PS10, Line 48: // values to advance to the end of the lengths section. Thi > We could sanitize this by passing data page level num_values. Yeah, good point. http://gerrit.cloudera.org:8080/#/c/24579/10/be/src/exec/parquet/parquet-delta-length-byte-array-decoder.cc@49 PS10, Line 49: // concatenated string data > IMO it would be better to do this in a streaming fashion, e.g. always decod Agree, we can do here too. http://gerrit.cloudera.org:8080/#/c/24579/10/be/src/exec/parquet/parquet-delta-length-byte-array-decoder.cc@107 PS10, Line 107: ParquetDeltaLengthByteArrayDecoder::NextValues( > IMO the reason why it is safe to smallify is that the StringValue is in the Corrected. This was from https://github.com/apache/Impala/blob/master/be/src/exec/parquet/parquet-common.h#L578 The wording seemed a bit off to me as well. http://gerrit.cloudera.org:8080/#/c/24579/10/be/src/exec/parquet/parquet-delta-length-byte-array-decoder.cc@111 PS10, Line 111: > Smallify() already returns a bool Ack http://gerrit.cloudera.org:8080/#/c/24579/10/testdata/workloads/functional-query/queries/QueryTest/parquet-delta-length-byte-array-encoding.test File testdata/workloads/functional-query/queries/QueryTest/parquet-delta-length-byte-array-encoding.test: http://gerrit.cloudera.org:8080/#/c/24579/10/testdata/workloads/functional-query/queries/QueryTest/parquet-delta-length-byte-array-encoding.test@4 PS10, Line 4: > you don't really need ORDER BY, the RESULTS is order independent by default Ack http://gerrit.cloudera.org:8080/#/c/24579/10/testdata/workloads/functional-query/queries/QueryTest/parquet-delta-length-byte-array-encoding.test@56 PS10, Line 56: > same as line 4 Ack -- 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: 12 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: Thu, 06 Aug 2026 22:03:12 +0000 Gerrit-HasComments: Yes
