Arnab Karmakar has posted comments on this change. ( http://gerrit.cloudera.org:8080/25008 )
Change subject: IMPALA-15413: Truncate Parquet page index min/max on UTF-8 boundaries ...................................................................... Patch Set 2: (8 comments) Thanks for working on this! http://gerrit.cloudera.org:8080/#/c/25008/2/be/src/util/string-util-test.cc File be/src/util/string-util-test.cc: http://gerrit.cloudera.org:8080/#/c/25008/2/be/src/util/string-util-test.cc@64 PS2, Line 64: 😀 > Reviewers, what do you think about including emojis in the code? Since these tests are about byte offsets, escaped bytes with the character in a comment, as in utf8-util-test.cc would be clearer. http://gerrit.cloudera.org:8080/#/c/25008/2/be/src/util/string-util.h File be/src/util/string-util.h: http://gerrit.cloudera.org:8080/#/c/25008/2/be/src/util/string-util.h@37 PS2, Line 37: /// 'str' holds the maximum value of some string set. We want to truncate it : /// to only occupy 'max_length' bytes. We also want to guarantee that the truncated : /// value remains greater than all the strings in the original set, so we need : /// to increase it after truncation. E.g.: when 'max_length' == 3: AAAAAAA => AAB : /// Returns error if it cannot increase the string value, ie. all bytes are 0xFF. : /// UTF-8 values are truncated as in TruncateDown(), then increased by raising a single : /// byte in place, so the result stays valid UTF-8 and within 'max_length'. Returns an : /// error if every character is already U+10FFFF. nit: I think we should merge the new comment with the older one so they dont disagree. It still says "all bytes are 0xFF" while the new paragraph adds the U+10FFFF case. http://gerrit.cloudera.org:8080/#/c/25008/2/be/src/util/string-util.cc File be/src/util/string-util.cc: http://gerrit.cloudera.org:8080/#/c/25008/2/be/src/util/string-util.cc@85 PS2, Line 85: if (!IncrementKeepingUtf8(result)) { If the truncated max is valid UTF-8 but can't be increased for cases like U+007F, U+07FF and U+FFFF, TruncateUp now returns an error. It sets valid_column_index_ = false for the column droping the page index for the whole column. The old byte-wise path succeeded (F4 8F BF BF became F4 8F BF C0). Although this might be very rare in real data but I think we could fall through to the byte-wise path rather than returning an error. http://gerrit.cloudera.org:8080/#/c/25008/2/be/src/util/utf8-util.h File be/src/util/utf8-util.h: http://gerrit.cloudera.org:8080/#/c/25008/2/be/src/util/utf8-util.h@38 PS2, Line 38: size_t TrimPartialUtf8(const char* ptr, size_t len); We can add a test for this in utf8-util-test.cc. http://gerrit.cloudera.org:8080/#/c/25008/2/tests/query_test/test_parquet_page_index.py File tests/query_test/test_parquet_page_index.py: http://gerrit.cloudera.org:8080/#/c/25008/2/tests/query_test/test_parquet_page_index.py@397 PS2, Line 397: def test_utf8_string_values(self, vector, unique_database, tmpdir): Can we also test CHAR and VARCHAR? http://gerrit.cloudera.org:8080/#/c/25008/2/tests/query_test/test_parquet_page_index.py@403 PS2, Line 403: the max's 4-byte ones do I think the increment was already handled by the old code and we should test use a max value whose characters don't line up with 64 bytes. http://gerrit.cloudera.org:8080/#/c/25008/2/tests/query_test/test_parquet_page_index.py@407 PS2, Line 407: E697A5 This is not affecting either result. http://gerrit.cloudera.org:8080/#/c/25008/2/tests/query_test/test_parquet_page_index.py@414 PS2, Line 414: assert column.column_index is not None We should assert exact min/max bytes, like test_max_string_values. -- To view, visit http://gerrit.cloudera.org:8080/25008 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib50b12a62cfad863825b1994b95a339ef6e5b788 Gerrit-Change-Number: 25008 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Vanko <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Daniel Vanko <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 06 Oct 2026 09:44:53 +0000 Gerrit-HasComments: Yes
