Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/24570 )
Change subject: IMPALA-12675: Set PARQUET_ANNOTATE_STRINGS_UTF8 to true by default and turn on validation ...................................................................... Patch Set 17: (7 comments) Thanks for working on this! http://gerrit.cloudera.org:8080/#/c/24570/17//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24570/17//COMMIT_MSG@8 PS17, Line 8: turn on validation nit: title can be longer than 72 chars http://gerrit.cloudera.org:8080/#/c/24570/17//COMMIT_MSG@41 PS17, Line 41: Please add some measurements, for example CTAS of tpch.lineitem to Parquet with the option on and off. http://gerrit.cloudera.org:8080/#/c/24570/17/be/src/exec/parquet/hdfs-parquet-table-writer.cc File be/src/exec/parquet/hdfs-parquet-table-writer.cc: http://gerrit.cloudera.org:8080/#/c/24570/17/be/src/exec/parquet/hdfs-parquet-table-writer.cc@284 PS17, Line 284: Status s_min = TruncateDown(page_stats.min_value, PAGE_INDEX_MAX_STRING_LENGTH, : &min_val); : Status s_max = TruncateUp(page_stats.max_value, PAGE_INDEX_MAX_STRING_LENGTH, : &max_val); We should have UTF8-aware versions of TruncateDown/Up as the Parquet spec says that the min/max values must remain valid values: https://github.com/apache/parquet-format/blob/219e3f12a62f9476e830c21e26d030d231f7c017/src/main/thrift/parquet.thrift#L300 We should check other engines' behavior as well. We could use the algorithms from https://github.com/apache/parquet-java/blob/master/parquet-column/src/main/java/org/apache/parquet/internal/column/columnindex/BinaryTruncator.java I'm fine with doing it separately. http://gerrit.cloudera.org:8080/#/c/24570/17/be/src/util/utf8-util.h File be/src/util/utf8-util.h: http://gerrit.cloudera.org:8080/#/c/24570/17/be/src/util/utf8-util.h@18 PS17, Line 18: #ifndef IMPALA_UTIL_UTF8_UTIL_H : #define IMPALA_UTIL_UTF8_UTIL_H nit: should be #pragma once http://gerrit.cloudera.org:8080/#/c/24570/17/common/thrift/generate_error_codes.py File common/thrift/generate_error_codes.py: http://gerrit.cloudera.org:8080/#/c/24570/17/common/thrift/generate_error_codes.py@517 PS17, Line 517: . We could provide some hint about the offending value, e.g. length or hex output. If we had an is_valid_utf8() builtin, we could also recommend it for customers to use that function to find the invalid value. http://gerrit.cloudera.org:8080/#/c/24570/17/docs/topics/impala_incompatible_changes.xml File docs/topics/impala_incompatible_changes.xml: http://gerrit.cloudera.org:8080/#/c/24570/17/docs/topics/impala_incompatible_changes.xml@80 PS17, Line 80: Iceberg tables always annotate <codeph>STRING</codeph> : columns as UTF-8 and cannot disable it, so a <codeph>BINARY</codeph> column is : required there.</p></li> We should highlight that existing Iceberg tables with STRING and non-UTF-8 characters might run into problems. E.g. OPTIMIZE, UPDATA, MERGE might stop working on them, as they'll raise errors for the non-UTF-8 data. Later we could think about how to resolve it. We could also expose an is_valid_utf8() builtin, so users could find the offending values. http://gerrit.cloudera.org:8080/#/c/24570/17/tests/query_test/test_scanners.py File tests/query_test/test_scanners.py: http://gerrit.cloudera.org:8080/#/c/24570/17/tests/query_test/test_scanners.py@1027 PS17, Line 1027: assert "not valid UTF-8" in str(err) After the INSERT OVERWRITE we could check that a SELECT query returns the earlier state, i.e. the IOW didn't have any effect. -- To view, visit http://gerrit.cloudera.org:8080/24570 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia95bcba978863ffb5d603fdf81bb2c75ba06f7da Gerrit-Change-Number: 24570 Gerrit-PatchSet: 17 Gerrit-Owner: Daniel Vanko <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Vanko <[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, 17 Sep 2026 16:25:38 +0000 Gerrit-HasComments: Yes
