Zoltan Borok-Nagy 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: (7 comments) Thanks for reviewing it. Most of the changes are related to tests. For new reviewers: I think it's worth to start with PS2 because it contains the essence of this CR. http://gerrit.cloudera.org:8080/#/c/15395/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15395/2//COMMIT_MSG@63 PS2, Line 63: TODO: > Also need tests on column masking since we also have some hacks in path res Updated test_ranger.py http://gerrit.cloudera.org:8080/#/c/15395/2/be/src/exec/hdfs-orc-scanner.cc File be/src/exec/hdfs-orc-scanner.cc: http://gerrit.cloudera.org:8080/#/c/15395/2/be/src/exec/hdfs-orc-scanner.cc@185 PS2, Line 185: if (scan_node_->hdfs_table()->IsFullAcid() && !schema_resolver_->IsFullAcid()) { > I think this is too strict. The test on file schema can be false positive. Done http://gerrit.cloudera.org:8080/#/c/15395/2/be/src/exec/orc-metadata-utils.cc File be/src/exec/orc-metadata-utils.cc: http://gerrit.cloudera.org:8080/#/c/15395/2/be/src/exec/orc-metadata-utils.cc@101 PS2, Line 101: DCHECK(table_idx >= num_part_cols); > I think this can be a DCHECK since partition columns should be skipped by h Done http://gerrit.cloudera.org:8080/#/c/15395/2/be/src/exec/orc-metadata-utils.cc@106 PS2, Line 106: > Could you add a DCHECK that the resulted index won't overflow? Done http://gerrit.cloudera.org:8080/#/c/15395/2/be/src/exec/orc-metadata-utils.cc@234 PS2, Line 234: root_->getSubtype(0)->getKind() != orc::TypeKind::INT || : root_->getSubtype(1)->getKind() ! > I think we should also check other fields and their types. Done http://gerrit.cloudera.org:8080/#/c/15395/2/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java File fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java: http://gerrit.cloudera.org:8080/#/c/15395/2/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@847 PS2, Line 847: HIVEFULLACIDWRITE > We don't have write support yet. Is this required somewhere? We need it to create tables. http://gerrit.cloudera.org:8080/#/c/15395/2/tests/query_test/test_scanners.py File tests/query_test/test_scanners.py: http://gerrit.cloudera.org:8080/#/c/15395/2/tests/query_test/test_scanners.py@1297 PS2, Line 1297: self.client.execute("create table %s.%s like tpch.lineitem stored as orc" % (db, tbl)) > Do we need this? Doesn't the table being translated to EXTERNAL table? Removed it. -- 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: Quanlong Huang <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 17 Mar 2020 17:18:48 +0000 Gerrit-HasComments: Yes
