[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Internal Jenkins (Code Review)]

Internal Jenkins has posted comments on this change.

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..


Patch Set 11: Verified+1

-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Internal Jenkins (Code Review)]

Internal Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..


IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Reviewed-on: http://gerrit.cloudera.org:8080/5390
Reviewed-by: Lars Volker 
Reviewed-by: Matthew Jacobs 
Tested-by: Internal Jenkins
---
M common/thrift/Frontend.thrift
M fe/src/main/cup/sql-parser.cup
D fe/src/main/java/org/apache/impala/analysis/ShowPartitionsStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_partition_ddl.test
M tests/query_test/test_kudu.py
12 files changed, 238 insertions(+), 72 deletions(-)

Approvals:
  Lars Volker: Looks good to me, but someone else must approve
  Matthew Jacobs: Looks good to me, approved
  Internal Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..


Patch Set 11: Code-Review+1

Rebased, ran ParserTest, AnalyzeDDLTest, kudu_alter.test and 
kudu_partition_ddl.test locally.

-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..


Patch Set 10: Code-Review+2

Thanks!

-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Lars Volker (Code Review)]

Hello Internal Jenkins, Alex Behm, Tim Armstrong,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5390

to look at the new patch set (#10).

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..

IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
---
M common/thrift/Frontend.thrift
M fe/src/main/cup/sql-parser.cup
D fe/src/main/java/org/apache/impala/analysis/ShowPartitionsStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_partition_ddl.test
M tests/query_test/test_kudu.py
12 files changed, 238 insertions(+), 72 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/5390/10
-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5390/9/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test
File testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test:

PS9, Line 168: drop range partition 10 < values <= 20
do we test anywhere if we can specify the same range (exactly) as is returned? 
ie 11<=V<21 ?


-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..


Patch Set 9:

Fixed an issue in the request forwarding code. Will rebase again.

-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Lars Volker (Code Review)]

Hello Internal Jenkins, Alex Behm, Tim Armstrong,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5390

to look at the new patch set (#9).

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..

IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
---
M common/thrift/Frontend.thrift
M fe/src/main/cup/sql-parser.cup
D fe/src/main/java/org/apache/impala/analysis/ShowPartitionsStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_partition_ddl.test
M tests/query_test/test_kudu.py
12 files changed, 237 insertions(+), 71 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/5390/9
-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Internal Jenkins (Code Review)]

Internal Jenkins has posted comments on this change.

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..


Patch Set 8: Verified-1

Build failed: 
http://sandbox.jenkins.cloudera.com/job/impala-external-gerrit-verify-merge-ASF/624/

-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..


Patch Set 8: Code-Review+2

Carry for alex

-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Lars Volker (Code Review)]

Hello Alex Behm,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5390

to look at the new patch set (#8).

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..

IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
---
M common/thrift/Frontend.thrift
M fe/src/main/cup/sql-parser.cup
D fe/src/main/java/org/apache/impala/analysis/ShowPartitionsStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_partition_ddl.test
M tests/query_test/test_kudu.py
12 files changed, 242 insertions(+), 72 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/5390/8
-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..


Patch Set 7: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5390/7/fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java
File fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java:

Line 76:   (op_ == TShowStatsOp.PARTITIONS || op_ == 
TShowStatsOp.RANGE_PARTITIONS)) {
remove the RANGE_PARTITIONS part, seems more important to say that is only 
works for Kudu


-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Lars Volker (Code Review)]

Lars Volker has uploaded a new patch set (#7).

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..

IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
---
M common/thrift/Frontend.thrift
M fe/src/main/cup/sql-parser.cup
D fe/src/main/java/org/apache/impala/analysis/ShowPartitionsStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_partition_ddl.test
M tests/query_test/test_kudu.py
12 files changed, 243 insertions(+), 72 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/5390/7
-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..


Patch Set 6:

(2 comments)

Please see PS7

http://gerrit.cloudera.org:8080/#/c/5390/6/fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java
File fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java:

Line 71:   if (table_.getNumClusteringCols() == 0) {
> Show table/column stats should be ok for unpartitioned tables.
Done


Line 85: } else if (table_ instanceof View) {
> seems simpler in my mind if we move this to the top as its own check (witho
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..


Patch Set 5:

(3 comments)

Thanks for the review. Please see PS6.

http://gerrit.cloudera.org:8080/#/c/5390/5/fe/src/main/java/org/apache/impala/analysis/ShowPartitionsStmt.java
File fe/src/main/java/org/apache/impala/analysis/ShowPartitionsStmt.java:

Line 30: public class ShowPartitionsStmt extends ShowStatsStmt {
> What's the benefit of subclassing ShowStatStmt? Why not just merge the code
The idea was to keep this change as small as possible. I removed this file now 
and unified the behavior in ShowStatStmt, which does look better.


http://gerrit.cloudera.org:8080/#/c/5390/5/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

Line 422: String header = "RANGE (" + 
Joiner.on(',').join(getRangeDistributionColNames()) + ")";
> nit: blank lines seem superfluous from here onward
Done


http://gerrit.cloudera.org:8080/#/c/5390/5/tests/query_test/test_kudu.py
File tests/query_test/test_kudu.py:

Line 117: name = unique_database + "." + self.random_table_name()
> why random table name? unique_database already takes care of that
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Lars Volker (Code Review)]

Lars Volker has uploaded a new patch set (#6).

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..

IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
---
M common/thrift/Frontend.thrift
M fe/src/main/cup/sql-parser.cup
D fe/src/main/java/org/apache/impala/analysis/ShowPartitionsStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_partition_ddl.test
M tests/query_test/test_kudu.py
12 files changed, 242 insertions(+), 73 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/5390/6
-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Lars Volker (Code Review)]

Lars Volker has uploaded a new patch set (#5).

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..

IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
---
M common/thrift/Frontend.thrift
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/ShowPartitionsStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_partition_ddl.test
M tests/query_test/test_kudu.py
12 files changed, 241 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/5390/5
-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..


Patch Set 4:

(3 comments)

Please see PS5

http://gerrit.cloudera.org:8080/#/c/5390/4//COMMIT_MSG
Commit Message:

PS4, Line 9: This is a rough draft and should serve as a base for discussion on 
how
   : to properly implement this. I'm happy to add another command to the
   : catalog to handle show partitions separately from show stats. We 
might
   : as well merge this change now and then clean it up properly in a
   : subsequent change.
   : 
   : Also I'm happy to add more tests if needed, e.g. to the 
AnalyzerTests.
> can you update this now?
Done


http://gerrit.cloudera.org:8080/#/c/5390/4/testdata/workloads/functional-query/queries/QueryTest/kudu_partition_ddl.test
File 
testdata/workloads/functional-query/queries/QueryTest/kudu_partition_ddl.test:

PS4, Line 97: 'VALUES = (10, "martin")'
: 'VALUES = (20, "dimitris")'
: 'VALUES = (30, "matthew")'
> did you fetch the latest kudu client jar from mvn? VALUES should be VALUE. 
Cleaning ~/.m2 and rebuilding from scratch seemed to help.


http://gerrit.cloudera.org:8080/#/c/5390/4/tests/query_test/test_kudu.py
File tests/query_test/test_kudu.py:

PS4, Line 133: assert False
> ?
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5390/4//COMMIT_MSG
Commit Message:

PS4, Line 9: This is a rough draft and should serve as a base for discussion on 
how
   : to properly implement this. I'm happy to add another command to the
   : catalog to handle show partitions separately from show stats. We 
might
   : as well merge this change now and then clean it up properly in a
   : subsequent change.
   : 
   : Also I'm happy to add more tests if needed, e.g. to the 
AnalyzerTests.
can you update this now?


http://gerrit.cloudera.org:8080/#/c/5390/4/testdata/workloads/functional-query/queries/QueryTest/kudu_partition_ddl.test
File 
testdata/workloads/functional-query/queries/QueryTest/kudu_partition_ddl.test:

PS4, Line 97: 'VALUES = (10, "martin")'
: 'VALUES = (20, "dimitris")'
: 'VALUES = (30, "matthew")'
did you fetch the latest kudu client jar from mvn? VALUES should be VALUE. We 
may need to wait to get this in so we don't break tests when the mvn artifact 
changes


http://gerrit.cloudera.org:8080/#/c/5390/4/tests/query_test/test_kudu.py
File tests/query_test/test_kudu.py:

PS4, Line 133: assert False
?


-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..


Patch Set 2:

Added the missing test with unbounded range partitions in PS4.

-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Lars Volker (Code Review)]

Lars Volker has uploaded a new patch set (#4).

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..

IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

This is a rough draft and should serve as a base for discussion on how
to properly implement this. I'm happy to add another command to the
catalog to handle show partitions separately from show stats. We might
as well merge this change now and then clean it up properly in a
subsequent change.

Also I'm happy to add more tests if needed, e.g. to the AnalyzerTests.

Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
---
M common/thrift/Frontend.thrift
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/ShowPartitionsStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_partition_ddl.test
M tests/query_test/test_kudu.py
12 files changed, 246 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/5390/4
-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..


Patch Set 2:

(5 comments)

Thanks for the reviews, please see PS3.

http://gerrit.cloudera.org:8080/#/c/5390/2//COMMIT_MSG
Commit Message:

Line 1: Parent: 87fd39ad (IMPALA-4477: Bump Kudu version to latest master 
(60aa54e))
> can you add a test cases:
Done.
1) by adding a "show range partitions" query to kudu_alter.test. I change the 
analysis to throw an exception for non-range partitioned tables.
2) added test AnalyzeDDLTest#TestShowRangePartitions()

I'll work on 3) next while waiting for the next round of review comments.


http://gerrit.cloudera.org:8080/#/c/5390/2/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:

PS2, Line 187: PARTITION
> PARTITIONS
Done


http://gerrit.cloudera.org:8080/#/c/5390/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

Line 421: resultSchema.addToColumns(new TColumn("Partition Specifier", 
Type.STRING.toThrift()));
> Maybe Range Partition Specifier?
Done


Line 421: resultSchema.addToColumns(new TColumn("Partition Specifier", 
Type.STRING.toThrift()));
> In the Kudu web UI we are calling it 'RANGE (col1, col2, col3, ...) PARTITI
Thanks Dan. I changed it to RANGE (col1, col2, col3, ...).


http://gerrit.cloudera.org:8080/#/c/5390/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java
File fe/src/main/java/org/apache/impala/service/JniFrontend.java:

PS2, Line 364:   // TODO Should we just pass params here? Or will this make 
the interfaces depend too
 :   // much on thrift types?
> we'll have to branch somewhere... I'm not sure it's clearly  better to hand
We could also branch here so we don't need an additional method there, but pass 
params to simplify the signatures. However it's stylistic and I'm also good 
with keeping this change minimal.


-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Lars Volker (Code Review)]

Lars Volker has uploaded a new patch set (#3).

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..

IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

This is a rough draft and should serve as a base for discussion on how
to properly implement this. I'm happy to add another command to the
catalog to handle show partitions separately from show stats. We might
as well merge this change now and then clean it up properly in a
subsequent change.

Also I'm happy to add more tests if needed, e.g. to the AnalyzerTests.

Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
---
M common/thrift/Frontend.thrift
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/ShowPartitionsStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test
10 files changed, 204 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/5390/3
-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5390/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

Line 421: resultSchema.addToColumns(new TColumn("Partition Specifier", 
Type.STRING.toThrift()));
> Maybe Range Partition Specifier?
In the Kudu web UI we are calling it 'RANGE (col1, col2, col3, ...) 
PARTITIONS'.  You can see a screenshot here: https://imgur.com/a/VNlMA.


-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/5390/2//COMMIT_MSG
Commit Message:

Line 1: Parent: 87fd39ad (IMPALA-4477: Bump Kudu version to latest master 
(60aa54e))
can you add a test cases:
1) w/ hash partitioning?
2) analysis tests for negative cases, e.g.
SHOW RANGE PARTITIONS hdfs_table
3) consider adding a test in test_kudu.py where you create a table via the 
python kudu client which has a single range partition unbounded on both sides, 
which we cannot express in impala. then the test loads it as an external table 
and calls show range partitions. You can see how we create tables in 
kudu_test_suite.py:temp_kudu_table() .

I think it's OK to get the change in without #3 but we should at least test it 
manually and get the change in later.


http://gerrit.cloudera.org:8080/#/c/5390/2/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:

PS2, Line 187: PARTITION
PARTITIONS


http://gerrit.cloudera.org:8080/#/c/5390/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

Line 421: resultSchema.addToColumns(new TColumn("Partition Specifier", 
Type.STRING.toThrift()));
Maybe Range Partition Specifier?
I'll ask Dan if there's a better name for this thing


http://gerrit.cloudera.org:8080/#/c/5390/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java
File fe/src/main/java/org/apache/impala/service/JniFrontend.java:

PS2, Line 364:   // TODO Should we just pass params here? Or will this make 
the interfaces depend too
 :   // much on thrift types?
we'll have to branch somewhere... I'm not sure it's clearly  better to handle 
the cases in there. Feel free to wait for input from Dimitris/Alex, but if you 
don't end up changing this please remove the TODO.


-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..


Patch Set 1:

(4 comments)

Thank you for the reviews. Please see PS2.

http://gerrit.cloudera.org:8080/#/c/5390/1/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:

Line 182:   1: required bool is_show_col_stats
> replace the two flags by an enum and use the thrift enum in the stmt code?
Done


http://gerrit.cloudera.org:8080/#/c/5390/1/fe/src/main/java/org/apache/impala/analysis/ShowPartitionsStmt.java
File fe/src/main/java/org/apache/impala/analysis/ShowPartitionsStmt.java:

Line 64:   if (kuduTable.getPrimaryKeyColumnNames().isEmpty()) {
> Not possible. Kudu tables must have PKs.
Done


http://gerrit.cloudera.org:8080/#/c/5390/1/fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java
File fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java:

Line 31:   protected final boolean isShowColStats_;
> The subclasses and flags are getting kind of confusing.
I gave this a try with enums and like how it turned out. It also allows for 
easier extension in the future.


http://gerrit.cloudera.org:8080/#/c/5390/1/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

Line 432:   for (String partition: partitions) {
> Do you know if the partitions are sorted? Might be good to do that, otherwi
Yes, the comment in the kudu client API explains that they are sorted. I added 
a comment here.


-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Lars Volker (Code Review)]

Lars Volker has uploaded a new patch set (#2).

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..

IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

This is a rough draft and should serve as a base for discussion on how
to properly implement this. I'm happy to add another command to the
catalog to handle show partitions separately from show stats. We might
as well merge this change now and then clean it up properly in a
subsequent change.

Also I'm happy to add more tests if needed, e.g. to the AnalyzerTests.

Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
---
M common/thrift/Frontend.thrift
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/ShowPartitionsStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test
9 files changed, 163 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/5390/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Matthew Jacobs 

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/5390/1/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:

Line 182:   1: required bool is_show_col_stats
replace the two flags by an enum and use the thrift enum in the stmt code?


http://gerrit.cloudera.org:8080/#/c/5390/1/fe/src/main/java/org/apache/impala/analysis/ShowPartitionsStmt.java
File fe/src/main/java/org/apache/impala/analysis/ShowPartitionsStmt.java:

Line 64:   if (kuduTable.getPrimaryKeyColumnNames().isEmpty()) {
Not possible. Kudu tables must have PKs.


http://gerrit.cloudera.org:8080/#/c/5390/1/fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java
File fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java:

Line 31:   protected final boolean isShowColStats_;
The subclasses and flags are getting kind of confusing.
For example, setting both isShowColsStats_ and isShowRangePartitions_ is 
illegal.

One idea for cleaning it up might be to introduce an enum for the different 
uses and move all the code inside here (i.e. merge the logic of ShowPartitions 
inside here).

Another alternative might be to have one subclass for the different uses.

Any other ideas?


http://gerrit.cloudera.org:8080/#/c/5390/1/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

Line 432:   for (String partition: partitions) {
Do you know if the partitions are sorted? Might be good to do that, otherwise 
it might be hard to search for a specific partition.


-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..


Patch Set 1:

Thanks, Lars!

So firstly, this functionality I think is pretty important so worth trying to 
get in. At first I was thinking that plumbing this through the 'ShowStats' path 
was too wonky, and that we'd probably want to just add separate 
classes/functions for a parallel path. However after thinking more about it, I 
think it's actually not the craziest thing to consider this a kind of ShowStats 
command. Right now it doesn't have any stats, but I think in the future we'd 
probably want to show number of rows. There's no way to get that info today, 
but with some help from Kudu it could be possible. That'd be super useful.

So to summarize, I'd be OK with this approach. We can add some TODOs about 
adding stats later. Let's see what others think.

-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

[Lars Volker (Code Review)]

Lars Volker has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/5390

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
..

IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables

This is a rough draft and should serve as a base for discussion on how
to properly implement this. I'm happy to add another command to the
catalog to handle show partitions separately from show stats. We might
as well merge this change now and then clean it up properly in a
subsequent change.

Also I'm happy to add more tests if needed, e.g. to the AnalyzerTests.

Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
---
M common/thrift/Frontend.thrift
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/ShowPartitionsStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test
9 files changed, 145 insertions(+), 21 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/5390/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker