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 3: (13 comments) I think the approach makes sense. Thanks for adding so many tests. http://gerrit.cloudera.org:8080/#/c/15395/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15395/3//COMMIT_MSG@58 PS3, Line 58: * SHOW CREATE TABLE (show-create-table-full-acid.test) Can you also add describe and describe formatted tests? I assume these show hide row__id. http://gerrit.cloudera.org:8080/#/c/15395/3/be/src/exec/hdfs-orc-scanner.cc File be/src/exec/hdfs-orc-scanner.cc: http://gerrit.cloudera.org:8080/#/c/15395/3/be/src/exec/hdfs-orc-scanner.cc@181 PS3, Line 181: VLOG_QUERY << filename(); I assume you're going to remove the log lines? http://gerrit.cloudera.org:8080/#/c/15395/3/be/src/exec/hdfs-orc-scanner.cc@185 PS3, Line 185: if (scan_node_->hdfs_table()->IsFullAcid() && !schema_resolver_->IsFullAcid()) { This is going to be addressed at some point, right? So we can scan original files too. http://gerrit.cloudera.org:8080/#/c/15395/3/be/src/exec/orc-metadata-utils.h File be/src/exec/orc-metadata-utils.h: http://gerrit.cloudera.org:8080/#/c/15395/3/be/src/exec/orc-metadata-utils.h@46 PS3, Line 46: bool IsFullAcid() const; Maybe IsFileFullAcid()? Since it's going to get confusing in future about whether this is referring to the file or the table. 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@94 PS3, Line 94: if (table_idx == num_part_cols + 5) { I'd find it clearer if it was a named constant instead of the magic number http://gerrit.cloudera.org:8080/#/c/15395/3/be/src/exec/orc-metadata-utils.cc@101 PS3, Line 101: DCHECK DCHECK_GE http://gerrit.cloudera.org:8080/#/c/15395/3/be/src/exec/orc-metadata-utils.cc@231 PS3, Line 231: bool OrcSchemaResolver::IsFullAcid() const { Can you add a brief comment on the approach? It seems to be basically "if it walks like a duck and quacks like a duck, it's a duck". http://gerrit.cloudera.org:8080/#/c/15395/3/be/src/runtime/descriptors.h File be/src/runtime/descriptors.h: http://gerrit.cloudera.org:8080/#/c/15395/3/be/src/runtime/descriptors.h@333 PS3, Line 333: IsFullAcid IsFullAcidTable? http://gerrit.cloudera.org:8080/#/c/15395/3/fe/src/main/java/org/apache/impala/analysis/AlterTableStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableStmt.java: http://gerrit.cloudera.org:8080/#/c/15395/3/fe/src/main/java/org/apache/impala/analysis/AlterTableStmt.java@94 PS3, Line 94: // analyzer.ensureTableNotTransactional(table_, "ALTER TABLE"); ? http://gerrit.cloudera.org:8080/#/c/15395/3/fe/src/main/java/org/apache/impala/analysis/Path.java File fe/src/main/java/org/apache/impala/analysis/Path.java: http://gerrit.cloudera.org:8080/#/c/15395/3/fe/src/main/java/org/apache/impala/analysis/Path.java@473 PS3, Line 473: convertToFullAcid convertToFullAcidFilePath? http://gerrit.cloudera.org:8080/#/c/15395/3/testdata/workloads/functional-query/queries/QueryTest/acid-negative.test File testdata/workloads/functional-query/queries/QueryTest/acid-negative.test: http://gerrit.cloudera.org:8080/#/c/15395/3/testdata/workloads/functional-query/queries/QueryTest/acid-negative.test@1 PS3, Line 1: #==== Why commented out? http://gerrit.cloudera.org:8080/#/c/15395/3/testdata/workloads/functional-query/queries/QueryTest/full-acid-rowid.test File testdata/workloads/functional-query/queries/QueryTest/full-acid-rowid.test: http://gerrit.cloudera.org:8080/#/c/15395/3/testdata/workloads/functional-query/queries/QueryTest/full-acid-rowid.test@16 PS3, Line 16: ---- QUERY Can you also add a LABELS section to make sure the labels for the ROW__ID components are sane. Any maybe also a test that selects individual elements of row__id. http://gerrit.cloudera.org:8080/#/c/15395/3/testdata/workloads/functional-query/queries/QueryTest/full-acid-rowid.test@17 PS3, Line 17: select row__id.*, * from functional_orc_def.alltypestiny; Do we expect the values to be deterministic across data loads? -- 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: 3 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Norbert Luksa <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 18 Mar 2020 18:08:16 +0000 Gerrit-HasComments: Yes
