Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/20676 )
Change subject: IMPALA-12333: SHOW CREATE TABLE outputs some unnecessary table properties ...................................................................... Patch Set 2: (18 comments) Thanks for the reviews Noemi and Zoltan. http://gerrit.cloudera.org:8080/#/c/20676/1/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java File fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java: http://gerrit.cloudera.org:8080/#/c/20676/1/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@95 PS1, Line 95: FeTable.CATALOG_VERSION, : FeTable.LAST_MODIFIED_BY, > Worth mentioning in the commit message if these properties were not hidden Done http://gerrit.cloudera.org:8080/#/c/20676/1/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java File fe/src/main/java/org/apache/impala/catalog/IcebergTable.java: http://gerrit.cloudera.org:8080/#/c/20676/1/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java@124 PS1, Line 124: // Parquet compression codec and compression level table properties. : public static final String PARQUET_COMPRESSION_CODEC = : "write.parquet.compression-codec"; : public static final String PARQUET_COMPRESSION_LEVEL = : "write.parquet.compression-level"; > These are not specific to Iceberg, these are used by Impala's event process Moved them to FeTable. http://gerrit.cloudera.org:8080/#/c/20676/1/tests/metadata/test_show_create_table.py File tests/metadata/test_show_create_table.py: http://gerrit.cloudera.org:8080/#/c/20676/1/tests/metadata/test_show_create_table.py@30 PS1, Line 30: > flake8: E302 expected 2 blank lines, found 1 Done http://gerrit.cloudera.org:8080/#/c/20676/1/tests/metadata/test_show_create_table.py@31 PS1, Line 31: s > flake8: W605 invalid escape sequence '\(' Done http://gerrit.cloudera.org:8080/#/c/20676/1/tests/metadata/test_show_create_table.py@31 PS1, Line 31: e > flake8: W605 invalid escape sequence '\)' Done http://gerrit.cloudera.org:8080/#/c/20676/1/tests/metadata/test_show_create_table.py@33 PS1, Line 33: > flake8: E251 unexpected spaces around keyword / parameter equals Done http://gerrit.cloudera.org:8080/#/c/20676/1/tests/metadata/test_show_create_table.py@33 PS1, Line 33: > flake8: E251 unexpected spaces around keyword / parameter equals Done http://gerrit.cloudera.org:8080/#/c/20676/1/tests/metadata/test_show_create_table.py@33 PS1, Line 33: > flake8: E302 expected 2 blank lines, found 1 Done http://gerrit.cloudera.org:8080/#/c/20676/1/tests/metadata/test_show_create_table.py@42 PS1, Line 42: > flake8: W605 invalid escape sequence '\s' Done http://gerrit.cloudera.org:8080/#/c/20676/1/tests/metadata/test_show_create_table.py@42 PS1, Line 42: > flake8: W605 invalid escape sequence '\s' Done http://gerrit.cloudera.org:8080/#/c/20676/1/tests/metadata/test_show_create_table.py@310 PS1, Line 310: assert table_primary_keys_map['primary_keys'] == table.primary_key_names > flake8: E302 expected 2 blank lines, found 1 Done http://gerrit.cloudera.org:8080/#/c/20676/1/tests/metadata/test_show_create_table.py@324 PS1, Line 324: > flake8: E202 whitespace before ']' Done http://gerrit.cloudera.org:8080/#/c/20676/1/tests/metadata/test_show_create_table.py@324 PS1, Line 324: > flake8: E203 whitespace before ':' Done http://gerrit.cloudera.org:8080/#/c/20676/1/tests/metadata/test_show_create_table.py@329 PS1, Line 329: \ > flake8: E203 whitespace before ':' Done http://gerrit.cloudera.org:8080/#/c/20676/1/tests/metadata/test_show_create_table.py@335 PS1, Line 335: res[key.strip()] = value.strip() > flake8: E302 expected 2 blank lines, found 1 Done http://gerrit.cloudera.org:8080/#/c/20676/1/tests/metadata/test_show_create_table.py@351 PS1, Line 351: > flake8: W504 line break after binary operator Done http://gerrit.cloudera.org:8080/#/c/20676/1/tests/metadata/test_show_create_table.py@373 PS1, Line 373: > flake8: E502 the backslash is redundant between brackets Done http://gerrit.cloudera.org:8080/#/c/20676/1/tests/metadata/test_show_create_table.py@383 PS1, Line 383: describe_res = self.execu > Should LAST_MODIFIED_BY and LAST_MODIFIED_TIME be also included here? They are not added as table properties to Iceberg tables (at least not in my experiments). This test case focuses on Iceberg. -- To view, visit http://gerrit.cloudera.org:8080/20676 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id6f2cb9194f685a583b0d550532eb2454f119666 Gerrit-Change-Number: 20676 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Noemi Pap-Takacs <npaptak...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Mon, 13 Nov 2023 17:54:41 +0000 Gerrit-HasComments: Yes
