Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15395 )
Change subject: IMPALA-9042: Milestone 1: properly scan files that has full ACID schema ...................................................................... Patch Set 6: (5 comments) http://gerrit.cloudera.org:8080/#/c/15395/6/be/src/exec/orc-metadata-utils.h File be/src/exec/orc-metadata-utils.h: http://gerrit.cloudera.org:8080/#/c/15395/6/be/src/exec/orc-metadata-utils.h@51 PS6, Line 51: bool is_table_full_acid_; These could be const I think, since they're only set in the constructor http://gerrit.cloudera.org:8080/#/c/15395/3/be/src/exec/orc-metadata-utils.cc File be/src/exec/orc-metadata-utils.cc: http://gerrit.cloudera.org:8080/#/c/15395/3/be/src/exec/orc-metadata-utils.cc@231 PS3, Line 231: "Type mismatch: table column $0 is map to column $1 in ORC file '$2'", > Looking at the ORC file metadata it turned out that the ACID version is als I think it would be good to validate that the schema is as expected, just so the rest of the code can reliably assume that the schema is sane. http://gerrit.cloudera.org:8080/#/c/15395/6/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/15395/6/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@323 PS6, Line 323: table.getMetaStoreTable().getParameters()); nit: indentation is a bit off http://gerrit.cloudera.org:8080/#/c/15395/6/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@329 PS6, Line 329: if (isFullAcid && i == table.getNumClusteringCols() && nit: multi-line if should use parentheses. Maybe this would be better to restructure as an else if anyway... http://gerrit.cloudera.org:8080/#/c/15395/5/testdata/workloads/functional-query/queries/QueryTest/describe-path.test File testdata/workloads/functional-query/queries/QueryTest/describe-path.test: http://gerrit.cloudera.org:8080/#/c/15395/5/testdata/workloads/functional-query/queries/QueryTest/describe-path.test@143 PS5, Line 143: 'row__id','struct<\n operation:int comment '''',\n originaltransaction:bigint comment '''',\n bucket:int comment '''',\n rowid:bigint comment '''',\n currenttransaction:bigint comment ''''\n>','' I'm not sure if making this visible is a good idea. What does hive do? -- To view, visit http://gerrit.cloudera.org:8080/15395 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic2e2afec00c9a5cf87f1d61b5fe52b0085844bcb Gerrit-Change-Number: 15395 Gerrit-PatchSet: 6 Gerrit-Owner: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Norbert Luksa <norbert.lu...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Thu, 26 Mar 2020 23:49:13 +0000 Gerrit-HasComments: Yes
