Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/16599 )
Change subject: IMPALA-10288: Implement DESCRIBE HISTORY for Iceberg tables ...................................................................... Patch Set 2: (12 comments) Sorry, I think I have also posted a rebase along with my changes. http://gerrit.cloudera.org:8080/#/c/16599/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16599/1//COMMIT_MSG@7 PS1, Line 7: IMPALA-10288 > I think maybe we should create a new sub-task under IMPALA-10153 for this p Done http://gerrit.cloudera.org:8080/#/c/16599/1//COMMIT_MSG@17 PS1, Line 17: 2020-10-13 14:01:07.234000 > Does Spark 'select * from table.history' show the same timestamp values? I I do no conversion on the stored data. Whatever is stored (millis from epoch) is taken as it is when I create a timestamp from it. http://gerrit.cloudera.org:8080/#/c/16599/1//COMMIT_MSG@14 PS1, Line 14: +----------------------------+---------------------+---------------------+---------------------+ : | creation_time | snapshot_id | parent_id | is_current_ancestor | : +----------------------------+---------------------+---------------------+---------------------+ : | 2020-10-13 14:01:07.234000 | 4400379706200951771 | NULL | TRUE | : | 2020-10-13 14:01:19.307000 | 4221472712544505868 | 4400379706200951771 | TRUE | : +----------------------------+---------------------+---------------------+---------------------+ > Maybe we can add more info here, such as operation/manifest_list/summary. S That should be a different query in my opinion. Here what I try to reproduce is Spark's "select * from tablename.history" not "select * from tablename.snapshots". However, if you still feel that those information should be part of the output here, I'm open to include it. See here https://iceberg.apache.org/spark/#inspecting-tables I added this to the message. http://gerrit.cloudera.org:8080/#/c/16599/1//COMMIT_MSG@24 PS1, Line 24: cting > nit: basic Done http://gerrit.cloudera.org:8080/#/c/16599/1//COMMIT_MSG@24 PS1, Line 24: : > SHOW TABLE HISTORY? Done http://gerrit.cloudera.org:8080/#/c/16599/1/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/16599/1/be/src/service/client-request-state.cc@426 PS1, Line 426: case TCatalogOpType::SHOW_TABLE_HISTORY > Maybe we should add some comments here, since this syntax is only supported Done http://gerrit.cloudera.org:8080/#/c/16599/1/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/16599/1/common/thrift/Frontend.thrift@287 PS1, Line 287: // Result of the DESCRIBE HISTORY command. : struct TGetTableHistoryResult { : 1: required list<TGetTableHistoryResultItem> result : } : > Please also add some comments for these new thrift structs. Done http://gerrit.cloudera.org:8080/#/c/16599/1/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/16599/1/fe/src/main/cup/sql-parser.cup@295 PS1, Line 295: KW_HISTORY > Adding "HISTORY" as a keyword will fail table creation with field names of Update: I just forgot to add KW_HISTORY to another place of this file. http://gerrit.cloudera.org:8080/#/c/16599/1/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/16599/1/fe/src/main/java/org/apache/impala/service/Frontend.java@109 PS1, Line 109: import org.apache.impala.catalog.ImpaladCatalo > Unused import Done http://gerrit.cloudera.org:8080/#/c/16599/1/fe/src/main/java/org/apache/impala/service/Frontend.java@1066 PS1, Line 1066: pendingCheckTasks.add(checkAuthorizationPool_.submit( > Please also add some comments for this method. Done http://gerrit.cloudera.org:8080/#/c/16599/1/testdata/data/README File testdata/data/README: http://gerrit.cloudera.org:8080/#/c/16599/1/testdata/data/README@578 PS1, Line 578: full_acid_schema_but_no_acid_version.orc > Please extend this file with the newly added Iceberg table. Done http://gerrit.cloudera.org:8080/#/c/16599/1/testdata/datasets/functional/functional_schema_template.sql File testdata/datasets/functional/functional_schema_template.sql: http://gerrit.cloudera.org:8080/#/c/16599/1/testdata/datasets/functional/functional_schema_template.sql@2983 PS1, Line 2983: IES('iceberg.file_format'='orc', 'icebe > Maybe we should use '/test-warehouse/iceberg_test/iceberg_multi_snapshots' Done -- To view, visit http://gerrit.cloudera.org:8080/16599 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I56a4b92c27e8e4a79109696cbae62735a00750e5 Gerrit-Change-Number: 16599 Gerrit-PatchSet: 2 Gerrit-Owner: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: wangsheng <[email protected]> Gerrit-Comment-Date: Fri, 30 Oct 2020 13:34:44 +0000 Gerrit-HasComments: Yes
