subject:"\[Impala\-ASF\-CR\] IMPALA\-3193\: Show table's comment on show tables"

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8851 )

Change subject: IMPALA-3193: Show table's comment on show tables
..


Patch Set 15:

(1 comment)

Applied the update.

http://gerrit.cloudera.org:8080/#/c/8851/15/fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
File fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java:

http://gerrit.cloudera.org:8080/#/c/8851/15/fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java@81
PS15, Line 81: getLoadedTable
> I think this function should be called loadAndAddTable. The current name im
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 15
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Tue, 13 Feb 2018 00:34:35 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

Hello Dimitris Tsirogiannis, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-3193: Show table's comment on show tables
..

IMPALA-3193: Show table's comment on show tables

Currently SHOW TABLES command does not display given comment
when creating a table. DESC  shows column's comment and
SHOW DATABASES also shows database's comment.

On this change, table's comment is displayed if the table is
loaded. If the table is not loaded, an empty comment is returned.

Testing:
Adds E2E test: TestShowTables.test_table_comment_visibility

Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/frontend.cc
M be/src/service/frontend.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/main/java/org/apache/impala/service/MetadataOp.java
M fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
M testdata/workloads/functional-query/queries/QueryTest/show.test
M tests/metadata/test_metadata_query_statements.py
M www/catalog.tmpl
20 files changed, 338 insertions(+), 218 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/8851/16
--
To view, visit http://gerrit.cloudera.org:8080/8851
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 16
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

2018-02-12 Thread Dimitris Tsirogiannis (Code Review)

Dimitris Tsirogiannis has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8851 )

Change subject: IMPALA-3193: Show table's comment on show tables
..


Patch Set 15:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8851/15/fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
File fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java:

http://gerrit.cloudera.org:8080/#/c/8851/15/fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java@81
PS15, Line 81: getLoadedTable
I think this function should be called loadAndAddTable. The current name 
implies that you return a table that is already loaded but this function is the 
one doing the load.


http://gerrit.cloudera.org:8080/#/c/8851/15/fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java@107
PS15, Line 107: /**
  :* Overrides ImpaladCatalog.getTables to load the table 
metadata if it is missing.
  :*/
  :   @Override
  :   public List getTables(String dbName, PatternMatcher 
matcher)
  :   throws DatabaseNotFoundException {
  : List tables = super.getTables(dbName, matcher);
  :
  : // The table was not yet loaded. Load it in to the catalog 
and try getTable()
  : // again.
  : for (int i = 0; i < tables.size(); ++i) {
  :   Table table = tables.get(i);
  :   // Table doesn't exist or is already loaded. Just return 
it.
  :   if (table != null && !table.isLoaded()) {
  : try {
  :   tables.set(i, getLoadedTable(dbName, 
table.getName()));
  : } catch (CatalogException e) {
  :   LOG.error(String.format("Error loading table: %s.%s", 
dbName, table.getName()),
  :   e);
  : }
  :   }
  : }
  : return tables;
  :   }
> During JUnit test, it should be called at "impaladCatalog_.get().getTables(
Good point. Thanks for the explanation.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 15
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Mon, 12 Feb 2018 23:26:09 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8851 )

Change subject: IMPALA-3193: Show table's comment on show tables
..


Patch Set 15:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8851/15/fe/src/main/java/org/apache/impala/service/MetadataOp.java
File fe/src/main/java/org/apache/impala/service/MetadataOp.java:

http://gerrit.cloudera.org:8080/#/c/8851/15/fe/src/main/java/org/apache/impala/service/MetadataOp.java@279
PS15, Line 279: for (Table table: fe.getTables(db.getName(), 
tablePatternMatcher, user)) {
  :   String tabName = table.getName();
> Are we certain that this code doesn't introduce the same infinite wait for
I confirmed the problem should be solved with my change. The cause of the 
problem is JUnit test requires tables loading explicitly if they are not 
loaded. This is the reason why I introduce a new code at 
ImpaladTestCatalog.java.


http://gerrit.cloudera.org:8080/#/c/8851/15/fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
File fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java:

http://gerrit.cloudera.org:8080/#/c/8851/15/fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java@107
PS15, Line 107: /**
  :* Overrides ImpaladCatalog.getTables to load the table 
metadata if it is missing.
  :*/
  :   @Override
  :   public List getTables(String dbName, PatternMatcher 
matcher)
  :   throws DatabaseNotFoundException {
  : List tables = super.getTables(dbName, matcher);
  :
  : // The table was not yet loaded. Load it in to the catalog 
and try getTable()
  : // again.
  : for (int i = 0; i < tables.size(); ++i) {
  :   Table table = tables.get(i);
  :   // Table doesn't exist or is already loaded. Just return 
it.
  :   if (table != null && !table.isLoaded()) {
  : try {
  :   tables.set(i, getLoadedTable(dbName, 
table.getName()));
  : } catch (CatalogException e) {
  :   LOG.error(String.format("Error loading table: %s.%s", 
dbName, table.getName()),
  :   e);
  : }
  :   }
  : }
  : return tables;
  :   }
> Where is the ImpaladTestCatalog::getTables() called?
During JUnit test, it should be called at 
"impaladCatalog_.get().getTables(dbName, matcher)". See 
https://gerrit.cloudera.org/#/c/8851/15/fe/src/main/java/org/apache/impala/service/Frontend.java
 at 625.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 15
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Mon, 12 Feb 2018 23:17:02 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

2018-02-12 Thread Dimitris Tsirogiannis (Code Review)

Dimitris Tsirogiannis has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8851 )

Change subject: IMPALA-3193: Show table's comment on show tables
..


Patch Set 15:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8851/15/fe/src/main/java/org/apache/impala/service/MetadataOp.java
File fe/src/main/java/org/apache/impala/service/MetadataOp.java:

http://gerrit.cloudera.org:8080/#/c/8851/15/fe/src/main/java/org/apache/impala/service/MetadataOp.java@279
PS15, Line 279: for (Table table: fe.getTables(db.getName(), 
tablePatternMatcher, user)) {
  :   String tabName = table.getName();
Are we certain that this code doesn't introduce the same infinite wait for 
metadata load problem you run into during testing?


http://gerrit.cloudera.org:8080/#/c/8851/15/fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
File fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java:

http://gerrit.cloudera.org:8080/#/c/8851/15/fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java@107
PS15, Line 107: /**
  :* Overrides ImpaladCatalog.getTables to load the table 
metadata if it is missing.
  :*/
  :   @Override
  :   public List getTables(String dbName, PatternMatcher 
matcher)
  :   throws DatabaseNotFoundException {
  : List tables = super.getTables(dbName, matcher);
  :
  : // The table was not yet loaded. Load it in to the catalog 
and try getTable()
  : // again.
  : for (int i = 0; i < tables.size(); ++i) {
  :   Table table = tables.get(i);
  :   // Table doesn't exist or is already loaded. Just return 
it.
  :   if (table != null && !table.isLoaded()) {
  : try {
  :   tables.set(i, getLoadedTable(dbName, 
table.getName()));
  : } catch (CatalogException e) {
  :   LOG.error(String.format("Error loading table: %s.%s", 
dbName, table.getName()),
  :   e);
  : }
  :   }
  : }
  : return tables;
  :   }
Where is the ImpaladTestCatalog::getTables() called?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 15
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Mon, 12 Feb 2018 18:42:43 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8851 )

Change subject: IMPALA-3193: Show table's comment on show tables
..


Patch Set 14:

(1 comment)

Applied the update.

I realized that table loading should be required if a table has not been loaded 
on JUnit test. In getTables, some of tables can be IncompleteTable. Thus, these 
tables should be loaded explicitly. Please see my change in ImpaladTestCatalog.

http://gerrit.cloudera.org:8080/#/c/8851/14/fe/src/main/java/org/apache/impala/service/MetadataOp.java
File fe/src/main/java/org/apache/impala/service/MetadataOp.java:

http://gerrit.cloudera.org:8080/#/c/8851/14/fe/src/main/java/org/apache/impala/service/MetadataOp.java@283
PS14, Line 283: else {
  : continue;
  :   }
> Are you sure this is correct? If table is loaded and is not an instance of
Sorry, I should have thought it deeply.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 14
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Mon, 12 Feb 2018 12:42:54 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

Hello Dimitris Tsirogiannis, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-3193: Show table's comment on show tables
..

IMPALA-3193: Show table's comment on show tables

Currently SHOW TABLES command does not display given comment
when creating a table. DESC  shows column's comment and
SHOW DATABASES also shows database's comment.

On this change, table's comment is displayed if the table is
loaded. If the table is not loaded, an empty comment is returned.

Testing:
Adds E2E test: TestShowTables.test_table_comment_visibility

Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/frontend.cc
M be/src/service/frontend.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/main/java/org/apache/impala/service/MetadataOp.java
M fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
M testdata/workloads/functional-query/queries/QueryTest/show.test
M tests/metadata/test_metadata_query_statements.py
M www/catalog.tmpl
20 files changed, 338 insertions(+), 218 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/8851/15
--
To view, visit http://gerrit.cloudera.org:8080/8851
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 15
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

2018-02-09 Thread Dimitris Tsirogiannis (Code Review)

Dimitris Tsirogiannis has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8851 )

Change subject: IMPALA-3193: Show table's comment on show tables
..


Patch Set 14:

(1 comment)

Thanks for looking into it.

http://gerrit.cloudera.org:8080/#/c/8851/14/fe/src/main/java/org/apache/impala/service/MetadataOp.java
File fe/src/main/java/org/apache/impala/service/MetadataOp.java:

http://gerrit.cloudera.org:8080/#/c/8851/14/fe/src/main/java/org/apache/impala/service/MetadataOp.java@283
PS14, Line 283: else {
  : continue;
  :   }
Are you sure this is correct? If table is loaded and is not an instance of 
IncompleteTable you'll call continue and ignore the table which I don't think 
is the right behavior.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 14
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Fri, 09 Feb 2018 18:04:18 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8851 )

Change subject: IMPALA-3193: Show table's comment on show tables
..


Patch Set 13:

Applied the update.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 13
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Fri, 09 Feb 2018 00:32:03 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

Hello Dimitris Tsirogiannis, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-3193: Show table's comment on show tables
..

IMPALA-3193: Show table's comment on show tables

Currently SHOW TABLES command does not display given comment
when creating a table. DESC  shows column's comment and
SHOW DATABASES also shows database's comment.

On this change, table's comment is displayed if the table is
loaded. If the table is not loaded, an empty comment is returned.

Testing:
Adds E2E test: TestShowTables.test_table_comment_visibility

Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/frontend.cc
M be/src/service/frontend.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/main/java/org/apache/impala/service/MetadataOp.java
M testdata/workloads/functional-query/queries/QueryTest/show.test
M tests/metadata/test_metadata_query_statements.py
M www/catalog.tmpl
19 files changed, 296 insertions(+), 206 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/8851/14
--
To view, visit http://gerrit.cloudera.org:8080/8851
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 14
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

2018-02-08 Thread Dimitris Tsirogiannis (Code Review)

Dimitris Tsirogiannis has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8851 )

Change subject: IMPALA-3193: Show table's comment on show tables
..


Patch Set 13:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8851/13/fe/src/main/java/org/apache/impala/service/MetadataOp.java
File fe/src/main/java/org/apache/impala/service/MetadataOp.java:

http://gerrit.cloudera.org:8080/#/c/8851/13/fe/src/main/java/org/apache/impala/service/MetadataOp.java@281
PS13, Line 281:   try {
  : table = catalog.getTable(db.getName(), tabName);
  :   } catch (TableLoadingException e) {
  : // Ignore exception (this table will be skipped).
  :   }
  :   if (table == null) continue;
> The removal of the code causes the timed out issue. The problem is infinite
Thanks for investigating this. I think that we may have some redundant calls 
(fe.getTables() L279 and getTable() L282). Feel free to revert some of these 
changes to avoid these redundancies.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 13
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Thu, 08 Feb 2018 18:19:21 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8851 )

Change subject: IMPALA-3193: Show table's comment on show tables
..


Patch Set 13:

(1 comment)

The timed out issue at Jenkins should be resolved on this patch set.

http://gerrit.cloudera.org:8080/#/c/8851/13/fe/src/main/java/org/apache/impala/service/MetadataOp.java
File fe/src/main/java/org/apache/impala/service/MetadataOp.java:

http://gerrit.cloudera.org:8080/#/c/8851/13/fe/src/main/java/org/apache/impala/service/MetadataOp.java@281
PS13, Line 281:   try {
  : table = catalog.getTable(db.getName(), tabName);
  :   } catch (TableLoadingException e) {
  : // Ignore exception (this table will be skipped).
  :   }
  :   if (table == null) continue;
The removal of the code causes the timed out issue. The problem is infinite 
retries for not loaded table at Frontend.requestTblLoadAndWait.

I thought the code should be duplicate. By the way, there are some conditions 
to check that a table is loaded, table type is come from IncompleteType and 
TableLoadingException happens. I prefer to revive the code instead of copy of 
essential code snippet due to avoidance of any side effect. Here is the 
relevant code for the condition: 
https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java#L250



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 13
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Thu, 08 Feb 2018 13:53:45 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

Hello Dimitris Tsirogiannis, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-3193: Show table's comment on show tables
..

IMPALA-3193: Show table's comment on show tables

Currently SHOW TABLES command does not display given comment
when creating a table. DESC  shows column's comment and
SHOW DATABASES also shows database's comment.

On this change, table's comment is displayed if the table is
loaded. If the table is not loaded, an empty comment is returned.

Testing:
Adds E2E test: TestShowTables.test_table_comment_visibility

Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/frontend.cc
M be/src/service/frontend.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/main/java/org/apache/impala/service/MetadataOp.java
M testdata/workloads/functional-query/queries/QueryTest/show.test
M tests/metadata/test_metadata_query_statements.py
M www/catalog.tmpl
19 files changed, 290 insertions(+), 200 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/8851/13
--
To view, visit http://gerrit.cloudera.org:8080/8851
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 13
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

2018-02-01 Thread Kim Jin Chul (Code Review)

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8851 )

Change subject: IMPALA-3193: Show table's comment on show tables
..


Patch Set 12: Code-Review-1

My change seems to be related with the timed out of the parallel test run in 
Jenkins. Let me look into this.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 12
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Thu, 01 Feb 2018 09:07:55 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

2018-01-31 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8851 )

Change subject: IMPALA-3193: Show table's comment on show tables
..


Patch Set 12:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1847/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 12
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Wed, 31 Jan 2018 22:48:35 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

2018-01-31 Thread Dimitris Tsirogiannis (Code Review)

Dimitris Tsirogiannis has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8851 )

Change subject: IMPALA-3193: Show table's comment on show tables
..


Patch Set 12: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 12
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Wed, 31 Jan 2018 22:48:29 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

2018-01-30 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8851 )

Change subject: IMPALA-3193: Show table's comment on show tables
..


Patch Set 11: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1827/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 11
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Wed, 31 Jan 2018 04:47:40 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

2018-01-30 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8851 )

Change subject: IMPALA-3193: Show table's comment on show tables
..


Patch Set 11:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1827/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 11
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Tue, 30 Jan 2018 18:47:37 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

2018-01-30 Thread Dimitris Tsirogiannis (Code Review)

Dimitris Tsirogiannis has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8851 )

Change subject: IMPALA-3193: Show table's comment on show tables
..


Patch Set 11: Code-Review+2

Thanks!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 11
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Tue, 30 Jan 2018 18:47:19 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

2018-01-29 Thread Kim Jin Chul (Code Review)

Hello Dimitris Tsirogiannis,

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

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

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

Change subject: IMPALA-3193: Show table's comment on show tables
..

IMPALA-3193: Show table's comment on show tables

Currently SHOW TABLES command does not display given comment
when creating a table. DESC  shows column's comment and
SHOW DATABASES also shows database's comment.

On this change, table's comment is displayed if the table is
loaded. If the table is not loaded, an empty comment is returned.

Testing:
Adds E2E test: TestShowTables.test_table_comment_visibility

Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/frontend.cc
M be/src/service/frontend.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/main/java/org/apache/impala/service/MetadataOp.java
M testdata/workloads/functional-query/queries/QueryTest/show.test
M tests/metadata/test_metadata_query_statements.py
M www/catalog.tmpl
19 files changed, 290 insertions(+), 208 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 10
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Kim Jin Chul

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

2018-01-29 Thread Kim Jin Chul (Code Review)

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8851 )

Change subject: IMPALA-3193: Show table's comment on show tables
..


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8851/9/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:

http://gerrit.cloudera.org:8080/#/c/8851/9/be/src/catalog/catalog-server.cc@368
PS9, Line 368: if (table_info.__isset.comment) {
 : Value table_comment(table_info.comment.c_str(), 
document->GetAllocator());
 : table_obj.AddMember("comment", table_comment, 
document->GetAllocator());
 :   }
> Did you verify that this shows up in the web UI of impalad and catalogd?  I
Done. The comment column is added to the catalog page like this: 
https://image.ibb.co/cSjog6/2018_01_30_2_35_55.png



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 9
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Tue, 30 Jan 2018 05:40:59 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

2018-01-29 Thread Dimitris Tsirogiannis (Code Review)

Dimitris Tsirogiannis has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8851 )

Change subject: IMPALA-3193: Show table's comment on show tables
..


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8851/9/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:

http://gerrit.cloudera.org:8080/#/c/8851/9/be/src/catalog/catalog-server.cc@368
PS9, Line 368: if (table_info.__isset.comment) {
 : Value table_comment(table_info.comment.c_str(), 
document->GetAllocator());
 : table_obj.AddMember("comment", table_comment, 
document->GetAllocator());
 :   }
Did you verify that this shows up in the web UI of impalad and catalogd?  I 
don't remember seeing any changes in www/catalog.tmpl.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 9
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Tue, 30 Jan 2018 01:57:19 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

2018-01-25 Thread Kim Jin Chul (Code Review)

Hello Dimitris Tsirogiannis,

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

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

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

Change subject: IMPALA-3193: Show table's comment on show tables
..

IMPALA-3193: Show table's comment on show tables

Currently SHOW TABLES command does not display given comment
when creating a table. DESC  shows column's comment and
SHOW DATABASES also shows database's comment.

On this change, table's comment is displayed if the table is
loaded. If the table is not loaded, an empty comment is returned.

Testing:
Adds E2E test: TestShowTables.test_table_comment_visibility

Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/frontend.cc
M be/src/service/frontend.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/main/java/org/apache/impala/service/MetadataOp.java
M testdata/workloads/functional-query/queries/QueryTest/show.test
M tests/metadata/test_metadata_query_statements.py
18 files changed, 288 insertions(+), 208 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 9
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Kim Jin Chul

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

2018-01-25 Thread Dimitris Tsirogiannis (Code Review)

Dimitris Tsirogiannis has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8851 )

Change subject: IMPALA-3193: Show table's comment on show tables
..


Patch Set 8:

(4 comments)

Thanks for the quick iteration. Minor comments left but I think it's close.

http://gerrit.cloudera.org:8080/#/c/8851/8/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:

http://gerrit.cloudera.org:8080/#/c/8851/8/be/src/service/client-request-state.cc@281
PS8, Line 281:   }
DCHECK(names.size() == comments.size());


http://gerrit.cloudera.org:8080/#/c/8851/8/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:

http://gerrit.cloudera.org:8080/#/c/8851/8/common/thrift/Frontend.thrift@95
PS8, Line 95: of table names and a list of optional table comment.
: // Table comment is set if the table is loaded, so it will be 
empty.
"of TTableInfo". I believe that is sufficient, since you already have a 
description above for TTableInfo.


http://gerrit.cloudera.org:8080/#/c/8851/8/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:

http://gerrit.cloudera.org:8080/#/c/8851/8/fe/src/main/java/org/apache/impala/service/JniCatalog.java@198
PS8, Line 198:   tableInfo.setTbl_name(name);
Why not adding the table comments and let the client decide whether they are 
needed or not. It will also make this function consistent with the 
JniFrontend::getTablesInfo() which always returns table comments.


http://gerrit.cloudera.org:8080/#/c/8851/8/tests/metadata/test_metadata_query_statements.py
File tests/metadata/test_metadata_query_statements.py:

http://gerrit.cloudera.org:8080/#/c/8851/8/tests/metadata/test_metadata_query_statements.py@194
PS8, Line 194: result = self.execute_query("show tables in {db} like 
'loaded'".format(db=unique_database))
 :assert "SHOULD BE SHOWN" in result.data[0]
 :result = self.execute_query("show tables in {db} like 
'not_loaded'".format(
 :db=unique_database))
 :assert "" in result.data[0]
 :result = self.execute_query("show tables in {db} like 
'no_comment'".format(
 :db=unique_database))
 :assert "" in result.data[0]
I'd rather you did a single show tables call in {db} and verify the results for 
the three tables. The point is to exercise the case where tables with and w/o 
comments are returned.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 8
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Thu, 25 Jan 2018 21:41:56 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

Kim Jin Chul has posted comments on this change. (
http://gerrit.cloudera.org:8080/8851 )

Change subject: IMPALA-3193: Show table's comment on show tables
..

Patch Set 7:

(12 comments)

Applied update.

http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/catalog/catalog.h
File be/src/catalog/catalog.h:

http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:

http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/service/client-request-state.cc@280
PS7, Line 280: if (!tbl_info.__isset.comment) DCHECK(tbl_info.comment.empty());
: comments.push_back(tbl_info.comment);
> This check is a bit confusing. Ideally, you want to ensure that names and c
Sorry, actually I didn't catch your intention for adding DCHECK in the previous
comment. I still don't understand your last sentence. Why should we consider
this(i.e. check)?
As far as I understand, comment should be either empty(table is not loaded case
or a given empty string as a comment) or not empty. Hence, we just put it to
comments. Anyway, we should guarantee that the number of names should be equal
to the number of comments.

Let me remove the DCHECK code line which seems to be meaningless now.

http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/service/frontend.h
File be/src/service/frontend.h:

http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/service/frontend.h@57
PS7, Line 57: /// Returns all matching table names and table comments.
: /// If pattern is NULL, match all tables otherwise match only
those tables that
: /// match the pattern string. Patterns are "p1|p2|p3" where |
denotes choice,
: /// and each pN may contain wildcards denoted by '*' which
match all strings.
: /// The TSessionState parameter is used to filter results of
metadata operations when
: /// authorization is enabled. If this is a user initiated
request, it should
: /// be set to the user's current session. If this is an Impala
internal request,
: /// the session should be set to NULL which will skip privilege
checks returning all
: /// results. Table comments can be empty if table is not loaded.
> See previous comment about this comment :).
Done

http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/service/frontend.cc
File be/src/service/frontend.cc:

http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/service/frontend.cc@145
PS7, Line 145: return JniUtil::CallJniMethod(fe_, get_tables_info_id_, params,
: tables_info_result);
> nit: check if it fits in a single line
Done

http://gerrit.cloudera.org:8080/#/c/8851/7/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:

http://gerrit.cloudera.org:8080/#/c/8851/7/common/thrift/Frontend.thrift@91
PS7, Line 91: Comment needs to indicate when and if this is set. For instance,
: // it's not clear if this is set if the table is loaded but
there is no comment.
> I believe this was my comment. The point was to explicitly specify the beha
Done. Sorry, it's my mistake. I was confusing table 'comment' and your
'comment'.

http://gerrit.cloudera.org:8080/#/c/8851/7/common/thrift/Frontend.thrift@96
PS7, Line 96: // getTablesInfo returns a list of table names and a list of
table comment.
> comments is not consistent with the struct definition, plz update
Done

http://gerrit.cloudera.org:8080/#/c/8851/7/common/thrift/Frontend.thrift@98
PS7, Line 98: tableinfos
> tables_info
Done

http://gerrit.cloudera.org:8080/#/c/8851/6/fe/src/main/java/org/apache/impala/catalog/Table.java
File fe/src/main/java/org/apache/impala/catalog/Table.java:

http://gerrit.cloudera.org:8080/#/c/8851/6/fe/src/main/java/org/apache/impala/catalog/Table.java@407
PS6, Line 407: blic
> My thinking is that we're not particularly consistent with using final in o
Thanks for sharing your idea. It's just my curiosity. I want to know the reason
why you thought. Actually I didn't find the use of final keyword elsewhere
except for class member variable. I would like to follow other authors'
preference.

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

Hello Dimitris Tsirogiannis,

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

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

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

Change subject: IMPALA-3193: Show table's comment on show tables
..

IMPALA-3193: Show table's comment on show tables

Currently SHOW TABLES command does not display given comment
when creating a table. DESC  shows column's comment and
SHOW DATABASES also shows database's comment.

On this change, table's comment is displayed if the table is
loaded. If the table is not loaded, an empty comment is returned.

Testing:
Adds E2E test: TestShowTables.testTableCommentVisibility

Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/frontend.cc
M be/src/service/frontend.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/main/java/org/apache/impala/service/MetadataOp.java
M testdata/workloads/functional-query/queries/QueryTest/show.test
M tests/metadata/test_metadata_query_statements.py
18 files changed, 304 insertions(+), 197 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 8
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Kim Jin Chul

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

2018-01-23 Thread Dimitris Tsirogiannis (Code Review)

Dimitris Tsirogiannis has posted comments on this change. (
http://gerrit.cloudera.org:8080/8851 )

Change subject: IMPALA-3193: Show table's comment on show tables
..

Patch Set 7:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/catalog/catalog.h
File be/src/catalog/catalog.h:

http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/catalog/catalog.h@79
PS7, Line 79: Returns all matching table names and table comments, per Hive's
: /// "SHOW TABLES ". Each table name of TTableInfo
returned is unqualified.
: /// Comment cannot be set when table is not loaded. If pattern
is NULL, match all
: /// tables otherwise match only those tables that match the
pattern string. Patterns
: /// are "p1|p2|p3" where | denotes choice, and each pN may
contain wildcards denoted
: /// by '*' which match all strings. Table comments can be empty
if table is not loaded.
Let's fix this comment a bit:
"Returns information of all matching tables in 'db' based on the input
'pattern'. If pattern is NULL, return information of all tables in 'db'
otherwise return information of only those tables whose names match the pattern
string. Patterns are "p1|p2|p3" where | denotes choice, and each pN may contain
wildcards denoted by '*' which match all strings. Table information includes
the fully qualified table name and, optionally, the table's comment, if set.
Table comment will be empty if a table is not loaded."

http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:

http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/service/frontend.h
File be/src/service/frontend.h:

http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/service/frontend.cc
File be/src/service/frontend.cc:

http://gerrit.cloudera.org:8080/#/c/8851/7/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:

http://gerrit.cloudera.org:8080/#/c/8851/7/common/thrift/Frontend.thrift@91
PS7, Line 91: Comment needs to indicate when and if this is set. For instance,
: // it's not clear if this is set if the table is loaded but
there is no comment.
I believe this was my comment. The point was to explicitly specify the behavior
re setting the comment field.

http://gerrit.cloudera.org:8080/#/c/8851/7/common/thrift/Frontend.thrift@96
PS7, Line 96: // getTablesInfo returns a list of table names and a list of
table comment.
comments is not consistent with the struct definition, plz update

http://gerrit.cloudera.org:8080/#/c/8851/7/common/thrift/Frontend.thrift@98
PS7, Line 98: tableinfos
tables_info

http://gerrit.cloudera.org:8080/#/c/8851/6/fe/src/main/java/org/apache/impala/catalog/Table.java
File fe/src/main/java/org/apache/impala/catalog/Table.java:

http://gerrit.cloudera.org:8080/#/c/8851/6/fe/src/main/java/org/apache/impala/catalog/Table.java@407
PS6, Line 407: blic
> Done. By the way, may I know why you would not prefer final keyword here? I
My thinking is that we're not particularly consistent with using final in our
java-based codebase and we typically don't use it unless we absolutely have to.
I see how it can help in some cases. So, you may keep it if you want.

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8851 )

Change subject: IMPALA-3193: Show table's comment on show tables
..


Patch Set 6:

(22 comments)

Applied the update.

http://gerrit.cloudera.org:8080/#/c/8851/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8851/6//COMMIT_MSG@13
PS6, Line 13: table's comment can be displayed if catalog table
: owns Hive metastore table(HMS) object
> There is no notion of ownership here. A loaded table is always associated w
Done


http://gerrit.cloudera.org:8080/#/c/8851/6//COMMIT_MSG@14
PS6, Line 14: There is not any HMS call
: due to performance concern, so table's comment can be empty even
: though table has a comment.
> Rewrite as: "If the table is not loaded, an empty comment is returned."
Done


http://gerrit.cloudera.org:8080/#/c/8851/6//COMMIT_MSG@18
PS6, Line 18: There is a change in thrift part: a list of table comments is 
added to
: TGetTablesResult struct as an optional element.
> Remove this sentence.
Done


http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:

http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/catalog/catalog-server.cc@381
PS6, Line 381: for (const TTableInfo& table_info: 
get_tables_info_result.tableinfos) {
 :   Value table_obj(kObjectType);
 :   Value fq_name(Substitute("$0.$1", db.db_name, 
table_info.tbl_name).c_str(),
 :   document->GetAllocator());
 :   table_obj.AddMember("fqtn", fq_name, 
document->GetAllocator());
 :   Value table_name(table_info.tbl_name.c_str(), 
document->GetAllocator());
 :   table_obj.AddMember("name", table_name, 
document->GetAllocator());
 :   table_array.PushBack(table_obj, document->GetAllocator());
 : }
> Why not returning the table comments as well?
Done


http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/catalog/catalog.h
File be/src/catalog/catalog.h:

http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/catalog/catalog.h@83
PS6, Line 83: Hive metastore was not loaded for a table
> "table is not loaded".
Done


http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/catalog/catalog.cc
File be/src/catalog/catalog.cc:

http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/catalog/catalog.cc@140
PS6, Line 140:   return JniUtil::CallJniMethod(catalog_, get_table_names_id_, 
params, tables_info_result);
> long line
Done


http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:

http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/service/client-request-state.cc@275
PS6, Line 275: for (const TTableInfo& tbl_info: tables_info_result.tableinfos) {
 : names.push_back(tbl_info.tbl_name);
 : comments.push_back(tbl_info.comment);
 :   }
 :   SetResultSet(names, comments);
> This code will result to DCHECK being hit if some table doesn't have the co
Done


http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/service/frontend.cc
File be/src/service/frontend.cc:

http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/service/frontend.cc@139
PS6, Line 139: Status Frontend::GetTableNames(const string& db, const string* 
pattern,
 : const TSessionState* session, TGetTablesInfoResult* 
tables_info_result) {
 :   TGetTablesInfoParams params;
 :   params.__set_db(db);
 :   if (pattern != NULL) params.__set_pattern(*pattern);
 :   if (session != NULL) params.__set_session(*session);
 :   return JniUtil::CallJniMethod(fe_, get_table_names_id_, 
params, tables_info_result);
 : }
 :
 : Status Frontend::GetTableNamesAndComments(const string& db, 
const string* pattern,
 : const TSessionState* session, TGetTablesInfoResult* 
tables_info_result) {
 :   TGetTablesInfoParams params;
 :   params.__set_db(db);
 :   if (pattern != NULL) params.__set_pattern(*pattern);
 :   if (session != NULL) params.__set_session(*session);
 :   return JniUtil::CallJniMethod(fe_, 
get_table_names_and_comments_id_, params,
 :   tables_info_result);
 : }
> Should be a single call Frontend::GetTablesInfo()
Done


http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/service/impala-http-handler.cc
File be/src/service/impala-http-handler.cc:

http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/service/impala-http-handler.cc@522
PS6, Line 522: for (const TTableInfo& tbl_info: 
get_tables_info_result.tableinfos) {
 :   Value table_obj(kObjectType);
 :   Value fq_name(Substitute("$0.$1", db.db_name, 
tbl_info.tbl_name).c_str(),

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables