Csaba Ringhofer 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 10: (13 comments) Looked deeper into the decoder, found no issues but some code risky areas/code duplications/optimization opportunities 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: InitDataDecoder I would prefer not to duplicate the code here (see my comment at line 469). The if at line 366 could simply extended with checking PARQUET_TYPE, initializing the decoder for BYTE_ARRAY and returning error for other types. http://gerrit.cloudera.org:8080/#/c/24579/10/be/src/exec/parquet/parquet-column-readers.cc@469 PS10, Line 469: if (bool_decoder_) return bool_decoder_->SkipValues(num_values); bool_decoder_ should not exist with StringValue, parquet::Type::BYTE_ARRAY my preference would be to not specialize (thus duplicate) SkipEncodedValuesInPage, but rule out impossible combinations using constant time values from the templates. The following should eliminate the if at compile time for other Parquet types, getting the same speed as the specialization: if (PARQUET_TYPE == parquet::Type::BYTE_ARRAY && page_encoding_ == Encoding::DELTA_LENGTH_BYTE_ARRAY) same could be done for the bool_decoder_ check - it is only possible with parquet::Type::BOOLEAN 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. http://gerrit.cloudera.org:8080/#/c/24579/10/be/src/exec/parquet/parquet-column-readers.cc@503 PS10, Line 503: } else if (page_encoding_ == Encoding::DELTA_LENGTH_BYTE_ARRAY) { could add condition for PARQUET_TYPE - this is a very perf critical function, skipping the if in other types than string may matter http://gerrit.cloudera.org:8080/#/c/24579/10/be/src/exec/parquet/parquet-column-readers.cc@772 PS10, Line 772: } else if (page_encoding_ == Encoding::DELTA_LENGTH_BYTE_ARRAY) { could filter on PARQUET_TYPE http://gerrit.cloudera.org:8080/#/c/24579/10/be/src/exec/parquet/parquet-column-readers.cc@1019 PS10, Line 1019: } else if (page_encoding_ == Encoding::DELTA_LENGTH_BYTE_ARRAY) { could filter on PARQUET_TYPE 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: /// Decode the next string value into *out. Returns 1 on success, 0 if the page is : /// exhausted, -1 on error. The StringValue points into the page buffer (zero-copy). : /// Only valid after a successful call to NewPage(). : int NextValue(StringValue* out) WARN_UNUSED_RESULT; : : /// Decode up to 'num_values' strings, writing them to 'out' with 'stride' bytes : /// between consecutive StringValues in the output buffer. : /// Returns the number decoded, 0 if exhausted, -1 on error. : int NextValues(int num_values, StringValue* out, int64_t stride) WARN_UNUSED_RESULT; IMO these functions could benefit from adding RESTRICT to them and their pointers. This is also true for ParquetDeltaDecoder, so it may make sense to do this in another patch that focuses on speed. http://gerrit.cloudera.org:8080/#/c/24579/10/be/src/exec/parquet/parquet-delta-length-byte-array-decoder.h@80 PS10, Line 80: has_small_ naming: has_only_small_ would be clearer, the current name suggest that it has at least one small string same for the property 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: const size_t num_values = len_decoder.GetTotalValueCount(); We could sanitize this by passing data page level num_values. https://github.com/apache/parquet-format/blob/c6a6967f53906174567eca7d66e9abede1424723/src/main/thrift/parquet.thrift#L747 - currently an extremely large num_values, e.g. due to data corruption, could crash Impala (also see my next comment) num_values includes NULLs, so it could be <=, not == http://gerrit.cloudera.org:8080/#/c/24579/10/be/src/exec/parquet/parquet-delta-length-byte-array-decoder.cc@49 PS10, Line 49: lengths_.resize(num_values); IMO it would be better to do this in a streaming fashion, e.g. always decoding 1024 lengths and refilling if all are used. 1MB data pages are becoming the default (Impala writes smaller ones), and with small strings this can include many lengths (in case of many empty strings it could becomes really huge). This may also allow saving some CPU cycles when skipping rows, but not much, as skipping delta ints still need to read and add all deltas. I am ok with doing this in another patch. http://gerrit.cloudera.org:8080/#/c/24579/10/be/src/exec/parquet/parquet-delta-length-byte-array-decoder.cc@107 PS10, Line 107: // This is safe to smallify since the string points to the page buffer's memory, IMO the reason why it is safe to smallify is that the StringValue is in the tuple which is only used by the decoder at this point. What you describe is the reason why non-smallifed strings work. http://gerrit.cloudera.org:8080/#/c/24579/10/be/src/exec/parquet/parquet-delta-length-byte-array-decoder.cc@111 PS10, Line 111: !sv->IsSmall() Smallify() already returns a bool 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: order by id you don't really need ORDER BY, the RESULTS is order independent by default 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: order by id same as line 4 -- 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: 10 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, 30 Jul 2026 06:46:45 +0000 Gerrit-HasComments: Yes
