Daniel Vanko 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 21: (13 comments) Thanks for the comments! I created the follow-up tickets as well. http://gerrit.cloudera.org:8080/#/c/24570/15//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24570/15//COMMIT_MSG@23 PS15, Line 23: g tables always annotate S > do the engines above treat string and char/varchar differently? No, they treat them the same. I created IMPALA-15414 for validating CHAR/VARCHAR. But not sure whether to make it optional or mandatory with an option, feel free to add your comments. 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: > nit: title can be longer than 72 chars Done http://gerrit.cloudera.org:8080/#/c/24570/17//COMMIT_MSG@41 PS17, Line 41: Validation adds little to write time. CTAS of tpch.lineitem (6M rows) > Please add some measurements, for example CTAS of tpch.lineitem to Parquet Done 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: if ((page_stats.__isset.min_value) && (page_stats.__isset.max_value)) { : Status s_min = TruncateDown(page_stats.min_value, PAGE_INDEX_MAX_STRING_LENGTH, : &min_val); : Status s_max = > We should have UTF8-aware versions of TruncateDown/Up as the Parquet spec s Filed IMPALA-15413 to track this. 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: #pragma once : > nit: should be #pragma once Done http://gerrit.cloudera.org:8080/#/c/24570/17/common/thrift/Query.thrift File common/thrift/Query.thrift: http://gerrit.cloudera.org:8080/#/c/24570/17/common/thrift/Query.thrift@273 PS17, Line 273: // Enabled by default since Impala 5.0 (IMPALA-12675). A non-UTF-8 value aborts the > nit: bit verbose Done 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 ou Added the hex of the offending byte and the offset to the error message. Created IMPALA-15411 for the is_valid_utf8() builtin. 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> > We should highlight that existing Iceberg tables with STRING and non-UTF-8 Done Filed IMPALA-15411 for is_valid_utf8(). http://gerrit.cloudera.org:8080/#/c/24570/17/tests/query_test/test_datasketches.py File tests/query_test/test_datasketches.py: http://gerrit.cloudera.org:8080/#/c/24570/17/tests/query_test/test_datasketches.py@34 PS17, Line 34: # Sketches are serialized binary data stored in STRING columns, so they are not valid > nit: IMPALA-9821 could be mentioned in the comments, as setting the query o Done http://gerrit.cloudera.org:8080/#/c/24570/17/tests/query_test/test_iceberg.py File tests/query_test/test_iceberg.py: http://gerrit.cloudera.org:8080/#/c/24570/17/tests/query_test/test_iceberg.py@100 PS17, Line 100: """IMPALA-12675: Iceberg always annotates STRING as UTF-8, so writing non-UTF-8 bytes > nit: bit verbose Done 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@933 PS17, Line 933: """Returns {column_name: SchemaElement} for the single Parquet file of > nit: bit verbose Done http://gerrit.cloudera.org:8080/#/c/24570/17/tests/query_test/test_scanners.py@966 PS17, Line 966: : # Create table and insert data that should not have UTF8 annotation for strings : options['parquet_annotate_strings_utf8'] = False > optional: it seems a bit wasteful to copy and parse the same time 4 times Done http://gerrit.cloudera.org:8080/#/c/24570/17/tests/query_test/test_scanners.py@1027 PS17, Line 1027: err = self.execute_query_expect_failure(self.client, > After the INSERT OVERWRITE we could check that a SELECT query returns the e Done -- 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: 21 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: Wed, 23 Sep 2026 16:14:31 +0000 Gerrit-HasComments: Yes
