You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2018/07/11 00:45:11 UTC
[Impala-ASF-CR] IMPALA-7257. Support Kudu tables in LocalCatalog
Hello Tianyi Wang, Vuk Ercegovac,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/10912
to review the following change.
Change subject: IMPALA-7257. Support Kudu tables in LocalCatalog
......................................................................
IMPALA-7257. Support Kudu tables in LocalCatalog
This adds support for querying Kudu tables when --use_local_catalog is
enabled.
With this change, most of the kudu e2e tests pass. Those that don't are
related to missing support for CREATE TABLE AS SELECT, which currently
has some downcasts to 'Db' causing issues. That will be addressed in a
separate patch along with fixing CTAS for FS-backed tables.
Change-Id: I5b6a317ee895e43e00ade953e814867b56b4e6dd
---
M fe/src/main/java/org/apache/impala/analysis/AlterTableAddDropRangePartitionStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableAddPartitionStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableAddReplaceColsStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableAlterColStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetFileFormatStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetLocationStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSortByStmt.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/KuduPartitionExpr.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java
M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
M fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java
M fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java
A fe/src/main/java/org/apache/impala/catalog/FeKuduTable.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalView.java
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
M fe/src/main/java/org/apache/impala/planner/TableSink.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/KuduUtil.java
M fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java
34 files changed, 570 insertions(+), 229 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/12/10912/1
--
To view, visit http://gerrit.cloudera.org:8080/10912
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5b6a317ee895e43e00ade953e814867b56b4e6dd
Gerrit-Change-Number: 10912
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-7257. Support Kudu tables in LocalCatalog
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10912 )
Change subject: IMPALA-7257. Support Kudu tables in LocalCatalog
......................................................................
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java
File fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java:
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java@369
PS3, Line 369: for (KuduPartitionParam partitionParam: table.getPartitionBy()) {
> I've been somewhat inconsistent with what goes into KuduTable or HdfsTable
moved to FeKuduTable.Utils
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@258
PS3, Line 258: public static List<KuduPartitionParam> loadPartitionByParams(
> Another option is to put them in a class FeKuduTable.Common. It would be cl
k, I'll move them to a new class FeKuduTable.Utils (seems the name "utils" is used more often in the code base)
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java:
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java@59
PS3, Line 59: public static LocalTable loadFromKudu(LocalDb db, Table msTable) {
> package-private? It's for the local package only.
Done
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java@139
PS3, Line 139: {
> long line
Done
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java@147
PS3, Line 147: // TODO(todd)
> oops, this was a bug. It was a TODO to setKey_columns, but then I did the T
yea, appears key_columns are only used by the backend for logging. fixed.
--
To view, visit http://gerrit.cloudera.org:8080/10912
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b6a317ee895e43e00ade953e814867b56b4e6dd
Gerrit-Change-Number: 10912
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Jul 2018 22:45:48 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7257. Support Kudu tables in LocalCatalog
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/10912 )
Change subject: IMPALA-7257. Support Kudu tables in LocalCatalog
......................................................................
Patch Set 3:
(1 comment)
LGTM. Will +2 after the small fixes are applied.
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java:
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java@140
PS3, Line 140: kuduTableName_ instead of name below
> The line of code there is accessing KuduTableDescriptor::table_name() (set
You are right. It's only used for logging in the backend.
--
To view, visit http://gerrit.cloudera.org:8080/10912
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b6a317ee895e43e00ade953e814867b56b4e6dd
Gerrit-Change-Number: 10912
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Jul 2018 22:55:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7257. Support Kudu tables in LocalCatalog
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Tianyi Wang, Vuk Ercegovac,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10912
to look at the new patch set (#4).
Change subject: IMPALA-7257. Support Kudu tables in LocalCatalog
......................................................................
IMPALA-7257. Support Kudu tables in LocalCatalog
This adds support for querying Kudu tables when --use_local_catalog is
enabled.
With this change, most of the kudu e2e tests pass. Those that don't are
related to missing support for CREATE TABLE AS SELECT, which currently
has some downcasts to 'Db' causing issues. That will be addressed in a
separate patch along with fixing CTAS for FS-backed tables.
Change-Id: I5b6a317ee895e43e00ade953e814867b56b4e6dd
---
M fe/src/main/java/org/apache/impala/analysis/AlterTableAddDropRangePartitionStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableAddPartitionStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableAddReplaceColsStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableAlterColStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetFileFormatStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetLocationStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSortByStmt.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/KuduPartitionExpr.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java
M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
M fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java
A fe/src/main/java/org/apache/impala/catalog/FeKuduTable.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalView.java
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
M fe/src/main/java/org/apache/impala/planner/TableSink.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/KuduUtil.java
M fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java
33 files changed, 693 insertions(+), 343 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/12/10912/4
--
To view, visit http://gerrit.cloudera.org:8080/10912
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5b6a317ee895e43e00ade953e814867b56b4e6dd
Gerrit-Change-Number: 10912
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-7257. Support Kudu tables in LocalCatalog
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/10912 )
Change subject: IMPALA-7257. Support Kudu tables in LocalCatalog
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@258
PS3, Line 258: public static List<KuduPartitionParam> loadPartitionByParams(
> I think eventually they could become default methods in the FeKuduTable int
Another option is to put them in a class FeKuduTable.Common. It would be closer to the Java 8 code structure.
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java:
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java@140
PS3, Line 140: kuduTableName_ instead of name below
> I think that was a bug in the old implementation. The name that shows up in
It's eventually used here: https://github.com/apache/impala/blob/master/be/src/exec/kudu-scan-node-base.cc#L103. Why shouldn't it be the table name in Kudu?
--
To view, visit http://gerrit.cloudera.org:8080/10912
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b6a317ee895e43e00ade953e814867b56b4e6dd
Gerrit-Change-Number: 10912
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Jul 2018 21:26:47 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7257. Support Kudu tables in LocalCatalog
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10912 )
Change subject: IMPALA-7257. Support Kudu tables in LocalCatalog
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/20/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/10912
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b6a317ee895e43e00ade953e814867b56b4e6dd
Gerrit-Change-Number: 10912
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Jul 2018 00:37:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7257. Support Kudu tables in LocalCatalog
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10912 )
Change subject: IMPALA-7257. Support Kudu tables in LocalCatalog
......................................................................
Patch Set 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2862/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/10912
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b6a317ee895e43e00ade953e814867b56b4e6dd
Gerrit-Change-Number: 10912
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jul 2018 05:19:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7257. Support Kudu tables in LocalCatalog
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/10912 )
Change subject: IMPALA-7257. Support Kudu tables in LocalCatalog
......................................................................
Patch Set 3:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java
File fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java:
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java@369
PS3, Line 369: for (KuduPartitionParam partitionParam: table.getPartitionBy()) {
Why putting them here instead of KuduTable?
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@258
PS3, Line 258: public static List<KuduPartitionParam> loadPartitionByParams(
What's the future plan for those static methods shared with local table?
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java:
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java@59
PS3, Line 59: public static LocalTable loadFromKudu(LocalDb db, Table msTable) {
package-private? It's for the local package only.
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java@139
PS3, Line 139: {
long line
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java@140
PS3, Line 140: kuduTableName_ instead of name below
Why can't we do the same here?
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java@147
PS3, Line 147: // TODO(todd)
please explain
--
To view, visit http://gerrit.cloudera.org:8080/10912
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b6a317ee895e43e00ade953e814867b56b4e6dd
Gerrit-Change-Number: 10912
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Jul 2018 01:20:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7257. Support Kudu tables in LocalCatalog
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10912 )
Change subject: IMPALA-7257. Support Kudu tables in LocalCatalog
......................................................................
Patch Set 6:
Build Started https://jenkins.impala.io/job/gerrit-code-review-checks/48/
Running initial code review checks. This is experimental - please report any issues to tarmstrong@cloudera.com or on this JIRA: IMPALA-7317
--
To view, visit http://gerrit.cloudera.org:8080/10912
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b6a317ee895e43e00ade953e814867b56b4e6dd
Gerrit-Change-Number: 10912
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jul 2018 02:06:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7257. Support Kudu tables in LocalCatalog
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Bharath Vissapragada, Tianyi Wang, Vuk Ercegovac, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10912
to look at the new patch set (#6).
Change subject: IMPALA-7257. Support Kudu tables in LocalCatalog
......................................................................
IMPALA-7257. Support Kudu tables in LocalCatalog
This adds support for querying Kudu tables when --use_local_catalog is
enabled.
With this change, most of the kudu e2e tests pass. Those that don't are
related to missing support for CREATE TABLE AS SELECT, which currently
has some downcasts to 'Db' causing issues. That will be addressed in a
separate patch along with fixing CTAS for FS-backed tables.
Change-Id: I5b6a317ee895e43e00ade953e814867b56b4e6dd
---
M fe/src/main/java/org/apache/impala/analysis/AlterTableAddDropRangePartitionStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableAddPartitionStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableAddReplaceColsStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableAlterColStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetFileFormatStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetLocationStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSortByStmt.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/KuduPartitionExpr.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java
M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
M fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java
A fe/src/main/java/org/apache/impala/catalog/FeKuduTable.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalView.java
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
M fe/src/main/java/org/apache/impala/planner/TableSink.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/KuduUtil.java
M fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java
33 files changed, 697 insertions(+), 343 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/12/10912/6
--
To view, visit http://gerrit.cloudera.org:8080/10912
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5b6a317ee895e43e00ade953e814867b56b4e6dd
Gerrit-Change-Number: 10912
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-7257. Support Kudu tables in LocalCatalog
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10912 )
Change subject: IMPALA-7257. Support Kudu tables in LocalCatalog
......................................................................
Patch Set 6: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/10912
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b6a317ee895e43e00ade953e814867b56b4e6dd
Gerrit-Change-Number: 10912
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jul 2018 08:14:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7257. Support Kudu tables in LocalCatalog
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10912 )
Change subject: IMPALA-7257. Support Kudu tables in LocalCatalog
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java
File fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java:
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java@369
PS3, Line 369: for (KuduPartitionParam partitionParam: table.getPartitionBy()) {
> Why putting them here instead of KuduTable?
I've been somewhat inconsistent with what goes into KuduTable or HdfsTable vs FeCatalogUtils. I'll move this there.
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@258
PS3, Line 258: public static List<KuduPartitionParam> loadPartitionByParams(
> What's the future plan for those static methods shared with local table?
I think eventually they could become default methods in the FeKuduTable interface. However right now I wanted to preserve the ability to backport this back to the 2.x branch so I've been avoiding Java8-only features and making static methods either here or in FeCatalogUtils. I was inconsistent in this patch, so I'll move that FeCatalogUtils static method back here.
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java:
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java@140
PS3, Line 140: kuduTableName_ instead of name below
> Why can't we do the same here?
I think that was a bug in the old implementation. The name that shows up in the descriptor should be the name of the table as seen by Impala, rather than the name as seen by Kudu, don't you think? I think the only issue it would cause is potentially wrong messages in the plan or summary output.
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java@147
PS3, Line 147: // TODO(todd)
> please explain
oops, this was a bug. It was a TODO to setKey_columns, but then I did the TODO and forgot to remove this stub.
Interesting that the code still works, though, despite not setting the key columns in the descriptor.
--
To view, visit http://gerrit.cloudera.org:8080/10912
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b6a317ee895e43e00ade953e814867b56b4e6dd
Gerrit-Change-Number: 10912
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Jul 2018 02:07:36 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7257. Support Kudu tables in LocalCatalog
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10912 )
Change subject: IMPALA-7257. Support Kudu tables in LocalCatalog
......................................................................
Patch Set 5: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/2856/
--
To view, visit http://gerrit.cloudera.org:8080/10912
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b6a317ee895e43e00ade953e814867b56b4e6dd
Gerrit-Change-Number: 10912
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Jul 2018 19:59:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7257. Support Kudu tables in LocalCatalog
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10912 )
Change subject: IMPALA-7257. Support Kudu tables in LocalCatalog
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2856/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/10912
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b6a317ee895e43e00ade953e814867b56b4e6dd
Gerrit-Change-Number: 10912
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Jul 2018 16:44:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7257. Support Kudu tables in LocalCatalog
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10912 )
Change subject: IMPALA-7257. Support Kudu tables in LocalCatalog
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/48/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/10912
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b6a317ee895e43e00ade953e814867b56b4e6dd
Gerrit-Change-Number: 10912
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jul 2018 02:27:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7257. Support Kudu tables in LocalCatalog
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10912 )
Change subject: IMPALA-7257. Support Kudu tables in LocalCatalog
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java:
http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java@140
PS3, Line 140: kuduTableName_ instead of name below
> It's eventually used here: https://github.com/apache/impala/blob/master/be/
The line of code there is accessing KuduTableDescriptor::table_name() (set down on line 149 of this file below). The table name passed in TTableDescriptor should be the _impala_ table name, though, which may not match the underlying kudu table name. (usually the underlying kudu table is "impala::dbname.tablename"). I don't know who accesses the TTableDescriptor.tablename
--
To view, visit http://gerrit.cloudera.org:8080/10912
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b6a317ee895e43e00ade953e814867b56b4e6dd
Gerrit-Change-Number: 10912
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Jul 2018 22:27:49 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7257. Support Kudu tables in LocalCatalog
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/10912 )
Change subject: IMPALA-7257. Support Kudu tables in LocalCatalog
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10912
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b6a317ee895e43e00ade953e814867b56b4e6dd
Gerrit-Change-Number: 10912
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jul 2018 18:59:41 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7257. Support Kudu tables in LocalCatalog
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10912 )
Change subject: IMPALA-7257. Support Kudu tables in LocalCatalog
......................................................................
IMPALA-7257. Support Kudu tables in LocalCatalog
This adds support for querying Kudu tables when --use_local_catalog is
enabled.
With this change, most of the kudu e2e tests pass. Those that don't are
related to missing support for CREATE TABLE AS SELECT, which currently
has some downcasts to 'Db' causing issues. That will be addressed in a
separate patch along with fixing CTAS for FS-backed tables.
Change-Id: I5b6a317ee895e43e00ade953e814867b56b4e6dd
Reviewed-on: http://gerrit.cloudera.org:8080/10912
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Tianyi Wang <tw...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/analysis/AlterTableAddDropRangePartitionStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableAddPartitionStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableAddReplaceColsStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableAlterColStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetFileFormatStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetLocationStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSortByStmt.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/KuduPartitionExpr.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java
M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
M fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java
A fe/src/main/java/org/apache/impala/catalog/FeKuduTable.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalView.java
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
M fe/src/main/java/org/apache/impala/planner/TableSink.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/KuduUtil.java
M fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java
33 files changed, 697 insertions(+), 343 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Tianyi Wang: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/10912
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I5b6a317ee895e43e00ade953e814867b56b4e6dd
Gerrit-Change-Number: 10912
Gerrit-PatchSet: 7
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-7257. Support Kudu tables in LocalCatalog
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10912 )
Change subject: IMPALA-7257. Support Kudu tables in LocalCatalog
......................................................................
Patch Set 4:
Build Started https://jenkins.impala.io/job/gerrit-code-review-checks/20/
Running initial code review checks. This is experimental - please report any issues to tarmstrong@cloudera.com or on this JIRA: IMPALA-7317
--
To view, visit http://gerrit.cloudera.org:8080/10912
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b6a317ee895e43e00ade953e814867b56b4e6dd
Gerrit-Change-Number: 10912
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Jul 2018 23:48:47 +0000
Gerrit-HasComments: No