You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Anurag Mantripragada (Code Review)" <ge...@cloudera.org> on 2019/11/15 23:55:49 UTC
[Impala-ASF-CR] IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
Anurag Mantripragada has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14720
Change subject: IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
......................................................................
IMPALA-9104: Add support for retrieval of primary keys
and foriegn keys through impala-hs2-server.
The goal is to let JDBC clients get constraint information
from Impala tables. We implement two new metadata operations in
impala-hs2-server, GetPrimaryKeys and GetCrossReference. For these to work,
new TCLIService methods were added. In FE, these two operations are
implemented to get the information from tables in the catalog.
Much like GetColumns(), tables need to be loaded in order to be able to get
PK/FK information. We wait for the PK table/FK table to load.
In the implementation, PK/FK information is returned
ONLY if the user has access to ALL the columns involved in the PK/FK
relationship.
Testing:
- Added three test tables to our test datasets since most of our FE tests
relied on dummy tables or testdata. It was difficult to test PK/FK with
these methods. Also, we can build on this testdata in future when we make
optimizer improvements.
- Added unit tests in AuthorizationTest and JDBCtest.
- Added e2e test in test_hs2.py
Caveats:
- Authorization information is not available for IncompleteTables. In
case of foreign key tables, it is not feasible to load all the
parent tables involved to check for column privileges. Therefore, there
maybe missing PK/FK relationships for parent tables that are not loaded.
- Retrieval of constraints is not yet supported in LocalCatalog mode. See
IMPALA-9158.
Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
---
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.h
M common/thrift/Frontend.thrift
M common/thrift/hive-1-api/TCLIService.thrift
M fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/main/java/org/apache/impala/analysis/TableDef.java
M fe/src/main/java/org/apache/impala/catalog/FeTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/MetadataOp.java
M fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
M testdata/datasets/functional/functional_schema_template.sql
M tests/hs2/test_hs2.py
17 files changed, 762 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/14720/1
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 1
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
Posted by "Anurag Mantripragada (Code Review)" <ge...@cloudera.org>.
Anurag Mantripragada has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
......................................................................
IMPALA-9104: Add support for retrieval of primary keys
and foriegn keys through impala-hs2-server.
The goal is to let JDBC clients get constraint information
from Impala tables. We implement two new metadata operations in
impala-hs2-server, GetPrimaryKeys and GetCrossReference. For these to work,
new TCLIService methods were added. In FE, these two operations are
implemented to get the information from tables in the catalog.
Much like GetColumns(), tables need to be loaded in order to be able to get
PK/FK information. We wait for the PK table/FK table to load.
In the implementation, PK/FK information is returned
ONLY if the user has access to ALL the columns involved in the PK/FK
relationship.
Testing:
- Added three test tables to our test datasets since most of our FE tests
relied on dummy tables or testdata. It was difficult to test PK/FK with
these methods. Also, we can build on this testdata in future when we make
optimizer improvements.
- Added unit tests in AuthorizationTest and JDBCtest.
- Added e2e test in test_hs2.py
Caveats:
- Authorization information is not available for IncompleteTables. In
case of foreign key tables, it is not feasible to load all the
parent tables involved to check for column privileges. Therefore, there
maybe missing PK/FK relationships for parent tables that are not loaded.
- Retrieval of constraints is not yet supported in LocalCatalog mode. See
IMPALA-9158.
Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
---
M be/src/rpc/kerberos-test.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.h
M bin/rat_exclude_files.txt
M common/thrift/Frontend.thrift
M common/thrift/hive-1-api/TCLIService.thrift
M fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/main/java/org/apache/impala/analysis/TableDef.java
M fe/src/main/java/org/apache/impala/catalog/FeTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/MetadataOp.java
M fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
A testdata/data/child_table.txt
A testdata/data/parent_table.txt
A testdata/data/parent_table_2.txt
M testdata/datasets/functional/functional_schema_template.sql
M tests/hs2/test_hs2.py
22 files changed, 788 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/14720/5
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 5
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Anurag Mantripragada (Code Review)" <ge...@cloudera.org>.
Anurag Mantripragada has uploaded a new patch set (#9). ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
The goal is to let JDBC clients get constraint information
from Impala tables. We implement two new metadata operations in
impala-hs2-server, GetPrimaryKeys and GetCrossReference, whose thrift
definitions are taken from Hive. For these to work,
new TCLIService methods were added. In FE, these two operations are
implemented to get the information from tables in the catalog.
Much like GetColumns(), tables need to be loaded in order to be able to get
PK/FK information. We wait for the PK table/FK table to load.
In the implementation, PK/FK information is returned
ONLY if the user has access to ALL the columns involved in the PK/FK
relationship.
Testing:
- Added three test tables to our test datasets since most of our FE tests
relied on dummy tables or testdata. It was difficult to test PK/FK with
these methods. Also, we can build on this testdata in future when we make
optimizer improvements.
- Added unit tests in AuthorizationTest and JDBCtest.
- Added e2e test in test_hs2.py
Caveats:
- Ranger needs OWNER user information for authorization. Since this is HMS
metadata that we do not aggresively load, this information is not available
for IncompleteTables. Some foreign key tables (fact tables for example)
might have FK/PK relationships with several PK tables some of which might
not be loaded in catalog. Currently we have no way to check column
previleges without owner user information tables. We do not return keys
involving such columns. Therefore, when Ranger is used, there maybe missing
PK/FK relationships for parent tables that are not loaded. This can be
tracked in IMPALA-9172.
- Retrieval of constraints is not yet supported in LocalCatalog mode. See
IMPALA-9158.
Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
---
M be/src/rpc/kerberos-test.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.h
M bin/rat_exclude_files.txt
M common/thrift/Frontend.thrift
M common/thrift/hive-1-api/TCLIService.thrift
M fe/src/main/java/org/apache/impala/analysis/TableDef.java
M fe/src/main/java/org/apache/impala/catalog/FeTable.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/MetadataOp.java
M fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
M testdata/data/README
A testdata/data/child_table.txt
A testdata/data/parent_table.txt
A testdata/data/parent_table_2.txt
M testdata/datasets/functional/functional_schema_template.sql
M tests/hs2/test_hs2.py
22 files changed, 793 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/14720/9
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 9
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Anurag Mantripragada (Code Review)" <ge...@cloudera.org>.
Anurag Mantripragada has uploaded a new patch set (#12). ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
The goal is to let JDBC clients get constraint information
from Impala tables. We implement two new metadata operations in
impala-hs2-server, GetPrimaryKeys and GetCrossReference, which are
already implemented in Hive's HS2. The thrift
definitions are copied from Hive's TCLIService.thrift. In FE, these
two operations are implemented to get the information from tables
in the catalog.
Much like GetColumns(), tables need to be loaded in order to be able to get
PK/FK information. We wait for the PK table/FK table to load.
In the implementation, PK/FK information is returned
ONLY if the user has access to ALL the columns involved in the PK/FK
relationship.
Testing:
- Added three test tables to our test datasets since most of our FE tests
relied on dummy tables or testdata. It was difficult to test PK/FK with
these methods. Also, we can build on this testdata in future when we make
optimizer improvements.
- Added unit tests in AuthorizationTest and JDBCtest.
- Added e2e test in test_hs2.py
- This patch modifies AnalyzeDDLTests and ToSqlTests to rely on the newly
added dataset instead of dummy tables for pk/fk tests.
Caveats:
- Ranger needs OWNER user information for authorization. Since this is HMS
metadata that we do not aggresively load, this information is not available
for IncompleteTables. Some foreign key tables (fact tables for example)
might have FK/PK relationships with several PK tables some of which might
not be loaded in catalog. Currently we have no way to check column
previleges without owner user information tables. We do not return keys
involving such columns. Therefore, when Ranger is used, there maybe missing
PK/FK relationships for parent tables that are not loaded. This can be
tracked in IMPALA-9172.
- Retrieval of constraints is not yet supported in LocalCatalog mode. See
IMPALA-9158.
Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
---
M be/src/rpc/kerberos-test.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.h
M bin/rat_exclude_files.txt
M common/thrift/Frontend.thrift
M common/thrift/hive-1-api/TCLIService.thrift
M fe/src/main/java/org/apache/impala/analysis/TableDef.java
M fe/src/main/java/org/apache/impala/catalog/FeTable.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/MetadataOp.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
M testdata/data/README
A testdata/data/child_table.txt
A testdata/data/parent_table.txt
A testdata/data/parent_table_2.txt
M testdata/datasets/functional/functional_schema_template.sql
M tests/hs2/test_hs2.py
25 files changed, 835 insertions(+), 48 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/14720/12
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 12
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
......................................................................
Patch Set 3: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/5224/
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 3
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sat, 16 Nov 2019 12:58:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Anurag Mantripragada (Code Review)" <ge...@cloudera.org>.
Anurag Mantripragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
Patch Set 10:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/main/java/org/apache/impala/service/Frontend.java@888
PS8, Line 888: .any()
: .onColumn(table.getTableName().getDb(), table.getTableName().getTbl(),
: fk.getFkcolumn_name(), table.getOwnerUser()).build();
:
> May be I don't understand what you mean by sequence here. Can you give an e
The confusion is because of a "foreign key" for a table is different from a "SQLForiegnKey" structures stored in HMS. A table can have a foreign key referencing two columns in a parent table. This "foreign key" is represent as two SQLForeignKeys, one for each column. These two share the same FKName but will have key_sequence numbers like 1, 2, .. respectively for each column.
For authorization, if we find that the user doesn't have privileges on one column in this FK, we will omit all the SQLForeignKeys that have the same fkName.
This is because the entire sequence is together considered a "foreign key" for that table and we don't want to return just one in the sequence.
I have written a more concrete example in the comments in code. Please let me know if doesn't make sense. :)
http://gerrit.cloudera.org:8080/#/c/14720/8/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/14720/8/fe/src/main/java/org/apache/impala/service/MetadataOp.java@393
PS8, Line 393: }
: columns.addAll(fe.getColumns(table, columnPatternMa
> Actually, it looks like if the column matcher is NONE, we get the table if
Hmm, I think you are right, it is unnecessary to load pks/fks if matcher is NONE. Added a condition.
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 10
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 01:56:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
......................................................................
Patch Set 6: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/5237/
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 6
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 18 Nov 2019 23:44:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
......................................................................
Patch Set 1: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/5220/
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 1
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sat, 16 Nov 2019 09:17:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
......................................................................
Patch Set 5: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/5232/
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 5
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 18 Nov 2019 02:43:30 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
Posted by "Anurag Mantripragada (Code Review)" <ge...@cloudera.org>.
Anurag Mantripragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
......................................................................
Patch Set 4:
Rat check is failing because I added new testdata file. How should I go about adding new testdata?
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 4
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sat, 16 Nov 2019 22:27:12 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
Posted by "Anurag Mantripragada (Code Review)" <ge...@cloudera.org>.
Anurag Mantripragada has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
......................................................................
IMPALA-9104: Add support for retrieval of primary keys
and foriegn keys through impala-hs2-server.
The goal is to let JDBC clients get constraint information
from Impala tables. We implement two new metadata operations in
impala-hs2-server, GetPrimaryKeys and GetCrossReference. For these to work,
new TCLIService methods were added. In FE, these two operations are
implemented to get the information from tables in the catalog.
Much like GetColumns(), tables need to be loaded in order to be able to get
PK/FK information. We wait for the PK table/FK table to load.
In the implementation, PK/FK information is returned
ONLY if the user has access to ALL the columns involved in the PK/FK
relationship.
Testing:
- Added three test tables to our test datasets since most of our FE tests
relied on dummy tables or testdata. It was difficult to test PK/FK with
these methods. Also, we can build on this testdata in future when we make
optimizer improvements.
- Added unit tests in AuthorizationTest and JDBCtest.
- Added e2e test in test_hs2.py
Caveats:
- Authorization information is not available for IncompleteTables. In
case of foreign key tables, it is not feasible to load all the
parent tables involved to check for column privileges. Therefore, there
maybe missing PK/FK relationships for parent tables that are not loaded.
- Retrieval of constraints is not yet supported in LocalCatalog mode. See
IMPALA-9158.
Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
---
M be/src/rpc/kerberos-test.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.h
M common/thrift/Frontend.thrift
M common/thrift/hive-1-api/TCLIService.thrift
M fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/main/java/org/apache/impala/analysis/TableDef.java
M fe/src/main/java/org/apache/impala/catalog/FeTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/MetadataOp.java
M fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
A testdata/data/child_table.txt
A testdata/data/parent_table.txt
A testdata/data/parent_table_2.txt
M testdata/datasets/functional/functional_schema_template.sql
M tests/hs2/test_hs2.py
21 files changed, 785 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/14720/4
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 4
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5227/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 3
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sat, 16 Nov 2019 20:45:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
Patch Set 8:
(13 comments)
Sorry for the delay in the review. Left some suggestions and comments below. Overall looks good to me.
http://gerrit.cloudera.org:8080/#/c/14720/8/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:
http://gerrit.cloudera.org:8080/#/c/14720/8/common/thrift/Frontend.thrift@594
PS8, Line 594: 10: optional TCLIService.TGetPrimaryKeysReq get_primary_keys_req
hmm, adding fields in the middle generally is considered a bad practice since it breaks backwards compatibility. Is there a specific reason to not add these fields with ids 11 and 12?
http://gerrit.cloudera.org:8080/#/c/14720/8/common/thrift/hive-1-api/TCLIService.thrift
File common/thrift/hive-1-api/TCLIService.thrift:
http://gerrit.cloudera.org:8080/#/c/14720/8/common/thrift/hive-1-api/TCLIService.thrift@954
PS8, Line 954: catalogName
Just curious to know if this is the Hive's catalog name or the catalog service identifier?
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java
File fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java:
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java@226
PS8, Line 226: public static TResultSet execGetPrimaryKeys(
: Frontend frontend, TMetadataOpRequest request, User user) throws ImpalaException {
: TGetPrimaryKeysReq req = request.getGet_primary_keys_req();
: return MetadataOp.getPrimaryKeys(frontend, req.getCatalogName(),
: req.getSchemaName(), req.getTableName(), user);
: }
:
: public static TResultSet execGetCrossReference(
: Frontend frontend, TMetadataOpRequest request, User user) throws ImpalaException {
: TGetCrossReferenceReq req = request.getGet_cross_reference_req();
: return MetadataOp.getCrossReference(frontend, req.getParentCatalogName(),
: req.getParentSchemaName(), req.getParentTableName(),
: req.getForeignCatalogName(), req.getForeignSchemaName(),
: req.getForeignTableName(), user);
: }
Are these methods are exactly the same as in case of hive-3 shim? If yes, why do we need them in the shim classes?
http://gerrit.cloudera.org:8080/#/c/14720/8/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/14720/8/fe/src/main/java/org/apache/impala/catalog/Table.java@610
PS8, Line 610: @Override // FeTable
: public List<SQLPrimaryKey> getPrimaryKeys() { return primaryKeys_; }
:
: @Override // FeTable
: public List<SQLForeignKey> getForeignKeys() { return foreignKeys_; }
Generally a good practice is to make these methods return Immutable objects so that clients who have a reference to these lists do not make changes to them. If we don't expect anyone else to modify these collections, I would suggest changing this to return ImmutableList.copyOf(primaryKeys_) or Collections.unmodifiableList() as appropriate
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java:
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java@215
PS8, Line 215: return new ArrayList<>();
replace with Collection.emptyList()?
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java@221
PS8, Line 221: eturn new ArrayList<>();
replace with Collection.emptyList()?
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/main/java/org/apache/impala/service/Frontend.java@888
PS8, Line 888: if (!authzChecker_.get().hasAccess(user, fkPrivilegeRequest) ||
: !authzChecker_.get().hasAccess(user, pkPrivilegeRequest)) {
: omitList.add(fkName);
: }
Isn't this logic same as
List<SQLForeignKey> result = new ArrayList<>();
for (alltblForeigKeys) {
...
if (authzChecker_.get().hasAccess(user, fkPrivilegeRequest)
&& authzChecker_.get().hasAccess(user,
pkPrivilegeRequest)) {
result.add(fKey);
}
}
return result;
We would avoid additional iteration and also potentially some auth RPC calls.
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/main/java/org/apache/impala/service/Frontend.java@1649
PS8, Line 1649: case GET_PRIMARY_KEYS: return MetastoreShim.execGetPrimaryKeys(this, request, user);
: case GET_CROSS_REFERENCE: return MetastoreShim.execGetCrossReference(this,
: request, user);
If the methods are exactly the same in both the shims, we should directly use MetadataOp methods here and get rid of the shim wrapper methods.
http://gerrit.cloudera.org:8080/#/c/14720/8/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/14720/8/fe/src/main/java/org/apache/impala/service/MetadataOp.java@393
PS8, Line 393: primaryKeys.addAll(fe.getPrimaryKeys(table, user));
: foreignKeys.addAll(fe.getForeignKeys(table, user));
do we have to exclude the keys which do not match the columnPatternMatcher? Also, may be skip the keys when columnPatternMatcher is NONE
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/main/java/org/apache/impala/service/MetadataOp.java@658
PS8, Line 658: This queries the Impala catalog to get the foreign keys of a given table.
: * Similar to getColumns, matching foreign key columns requires loading the
: * table metadata, so if any missing tables are found, an RPC to the CatalogServer
: * will be executed to request loading these tables. The matching process will be
: * restarted once the required tables have been loaded in the local Impalad Catalog or
: * the wait timeout has been reached
Can you please add documentation on what this method is doing with respect to matching of the parent* arguments?
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/main/java/org/apache/impala/service/MetadataOp.java@692
PS8, Line 692: we filter out the results based on the request and
: // send only the relevant key information.
suggest you change this to we filter out the foreignKeys which are not matching with given parent and foreign schema information.
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/main/java/org/apache/impala/service/MetadataOp.java@697
PS8, Line 697: foreignSchemaName, foreignTableName
Do we need these as a filtering criteria? seems like we are already pulling the information above only which match these foreign schema and tablename. Right?
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/main/java/org/apache/impala/service/MetadataOp.java@729
PS8, Line 729: * Helper to return only the foreign keys for primary key tables requested in the FK
: * request.
Can you be more specific here. It was not very clear to me until I read the code.
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 8
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 20 Nov 2019 19:27:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
Patch Set 9:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5092/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 9
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 00:40:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
Patch Set 11: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 11
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 06:20:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5041/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 5
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sun, 17 Nov 2019 18:59:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Anurag Mantripragada (Code Review)" <ge...@cloudera.org>.
Anurag Mantripragada has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
The goal is to let JDBC clients get constraint information
from Impala tables. We implement two new metadata operations in
impala-hs2-server, GetPrimaryKeys and GetCrossReference, whose thrift
definitions are taken from Hive. For these to work,
new TCLIService methods were added. In FE, these two operations are
implemented to get the information from tables in the catalog.
Much like GetColumns(), tables need to be loaded in order to be able to get
PK/FK information. We wait for the PK table/FK table to load.
In the implementation, PK/FK information is returned
ONLY if the user has access to ALL the columns involved in the PK/FK
relationship.
Testing:
- Added three test tables to our test datasets since most of our FE tests
relied on dummy tables or testdata. It was difficult to test PK/FK with
these methods. Also, we can build on this testdata in future when we make
optimizer improvements.
- Added unit tests in AuthorizationTest and JDBCtest.
- Added e2e test in test_hs2.py
Caveats:
- Ranger needs OWNER user information for authorization. Since this is HMS
metadata that we do not aggresively load, this information is not available
for IncompleteTables. Some foreign key tables (fact tables for example)
might have FK/PK relationships with several PK tables some of which might
not be loaded in catalog. Currently we have no way to check column
previleges without owner user information tables. We do not return keys
involving such columns. Therefore, when Ranger is used, there maybe missing
PK/FK relationships for parent tables that are not loaded. We should fix
this seperately.
- Retrieval of constraints is not yet supported in LocalCatalog mode. See
IMPALA-9158.
Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
---
M be/src/rpc/kerberos-test.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.h
M bin/rat_exclude_files.txt
M common/thrift/Frontend.thrift
M common/thrift/hive-1-api/TCLIService.thrift
M fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/main/java/org/apache/impala/analysis/TableDef.java
M fe/src/main/java/org/apache/impala/catalog/FeFsTable.java
M fe/src/main/java/org/apache/impala/catalog/FeTable.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/MetadataOp.java
M fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
M testdata/data/README
A testdata/data/child_table.txt
A testdata/data/parent_table.txt
A testdata/data/parent_table_2.txt
M testdata/datasets/functional/functional_schema_template.sql
M tests/hs2/test_hs2.py
25 files changed, 820 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/14720/7
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 7
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
Patch Set 8:
(1 comment)
Patch looks good to me. Thanks for making the suggested changes.
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/main/java/org/apache/impala/service/Frontend.java@888
PS8, Line 888: if (!authzChecker_.get().hasAccess(user, fkPrivilegeRequest) ||
: !authzChecker_.get().hasAccess(user, pkPrivilegeRequest)) {
: omitList.add(fkName);
: }
> The confusion is because of a "foreign key" for a table is different from a
Thanks for the explaination. Makes much more sense now.
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 8
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 06:20:18 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Anurag Mantripragada (Code Review)" <ge...@cloudera.org>.
Anurag Mantripragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
Patch Set 8:
(13 comments)
The developer docs page says clang can be used only with the C++ part of the code. I was manually doing this and still miss a few code style errors. Clang documentation says it can be used for Java too. Maybe I should try and modify our clang tool to work with Java. Thank you for catching these errors.
http://gerrit.cloudera.org:8080/#/c/14720/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14720/7//COMMIT_MSG@38
PS7, Line 38: This can be
: tracked in IMPAL
> Is there a JIRA for it?
This was first mentioned in https://gerrit.cloudera.org/#/c/14106/. I could not find a JIRA, so I filed IMPALA-9172. Updated the commit message.
http://gerrit.cloudera.org:8080/#/c/14720/7/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:
http://gerrit.cloudera.org:8080/#/c/14720/7/common/thrift/Frontend.thrift@576
PS7, Line 576: GET_CROSS_REFERENC
> GET_CROSS_REFERENCE
Done
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/analysis/TableDef.java
File fe/src/main/java/org/apache/impala/analysis/TableDef.java:
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/analysis/TableDef.java@547
PS7, Line 547: i
> nit: extra whitespace
Done
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/catalog/FeTable.java
File fe/src/main/java/org/apache/impala/catalog/FeTable.java:
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/catalog/FeTable.java@99
PS7, Line 99:
> I don't think this is necessary, here and elsewhere
This is obsolete. I reverted the NotImplementedException change.
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java:
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java@56
PS7, Line 56: import org.apache.impala.thrift.CatalogObjectsConstants;
> I don't think you need this
Done
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java:
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java@216
PS7, Line 216:
:
> nit: you can get rid of the '+' by putting this all on the second line
Not needed anymore since I reverted the NotImplementedException change.
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/service/Frontend.java@907
PS7, Line 907: blic List<? extends FeDb> getDbs(PatternMatcher matcher, User user)
: throws InternalException {
: List<? extends FeDb> dbs = getCatalog().getDbs(matcher);
: // If authorization is enabled
> My thinking in suggesting the NotImplementedException was that we would bub
I see your point. Since the IMPALA-9158 patch is in review, depending on which patch lands first, I will either create a cleanup patch or modify this patch to clean things. If it is okay with you, to avoid changing the tests more than once, I will revert to the earlier behavior of returning an empty PK/FK list in LocalCatlogMode.
http://gerrit.cloudera.org:8080/#/c/14720/5/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/14720/5/fe/src/main/java/org/apache/impala/service/MetadataOp.java@736
PS5, Line 736: eName == null) {
> Good question - I just know if it as our usual convention, eg. if you look
Thanks for the explanation. I reverted it to use the "diamond" style.
http://gerrit.cloudera.org:8080/#/c/14720/7/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/14720/7/fe/src/main/java/org/apache/impala/service/MetadataOp.java@650
PS7, Line 650: if (LOG.isTraceEnabled()) {
: LOG.trace("Returning {} primary k
> formatting
Done
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/service/MetadataOp.java@676
PS7, Line 676: tableM
> formatting
Done
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/service/MetadataOp.java@720
PS7, Line 720: }
: if (LOG.isTraceEnabled()) {
> formatting
Done
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java
File fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java:
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java@348
PS7, Line 348: privileges
> typo
Done
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java@373
PS7, Line 373: // On parent_table, only one of the pk columns has privileges for USER, hence the
> nit: unnecessary whitespace
Done
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 8
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 20 Nov 2019 00:50:35 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
Posted by "Anurag Mantripragada (Code Review)" <ge...@cloudera.org>.
Anurag Mantripragada has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
......................................................................
IMPALA-9104: Add support for retrieval of primary keys
and foriegn keys through impala-hs2-server.
The goal is to let JDBC clients get constraint information
from Impala tables. We implement two new metadata operations in
impala-hs2-server, GetPrimaryKeys and GetCrossReference. For these to work,
new TCLIService methods were added. In FE, these two operations are
implemented to get the information from tables in the catalog.
Much like GetColumns(), tables need to be loaded in order to be able to get
PK/FK information. We wait for the PK table/FK table to load.
In the implementation, PK/FK information is returned
ONLY if the user has access to ALL the columns involved in the PK/FK
relationship.
Testing:
- Added three test tables to our test datasets since most of our FE tests
relied on dummy tables or testdata. It was difficult to test PK/FK with
these methods. Also, we can build on this testdata in future when we make
optimizer improvements.
- Added unit tests in AuthorizationTest and JDBCtest.
- Added e2e test in test_hs2.py
Caveats:
- Authorization information is not available for IncompleteTables. In
case of foreign key tables, it is not feasible to load all the
parent tables involved to check for column privileges. Therefore, there
maybe missing PK/FK relationships for parent tables that are not loaded.
- Retrieval of constraints is not yet supported in LocalCatalog mode. See
IMPALA-9158.
Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
---
M be/src/rpc/kerberos-test.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.h
M common/thrift/Frontend.thrift
M common/thrift/hive-1-api/TCLIService.thrift
M fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/main/java/org/apache/impala/analysis/TableDef.java
M fe/src/main/java/org/apache/impala/catalog/FeTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/MetadataOp.java
M fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
M testdata/datasets/functional/functional_schema_template.sql
M tests/hs2/test_hs2.py
18 files changed, 767 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/14720/3
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 3
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Anurag Mantripragada (Code Review)" <ge...@cloudera.org>.
Anurag Mantripragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
Patch Set 7:
(20 comments)
Thanks for your comments.
http://gerrit.cloudera.org:8080/#/c/14720/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14720/5//COMMIT_MSG@7
PS5, Line 7: Support retrieval of PK/FK information through impala-hs2-server.
:
> nit: keep the title of the commit message to one line
Done
http://gerrit.cloudera.org:8080/#/c/14720/5//COMMIT_MSG@12
PS5, Line 12: en from Hive. For these to work,
> Might be worth explicitly mentioning that the thrift definitions for these
Done
http://gerrit.cloudera.org:8080/#/c/14720/5//COMMIT_MSG@31
PS5, Line 31: - Ranger needs OWNER user information for authorization. Since this is HMS
> Could you expand on this a little more, in particular why is not feasible,
Just learne that the issue is only when using Ranger. Updated the comment accordingly.
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/analysis/TableDef.java
File fe/src/main/java/org/apache/impala/analysis/TableDef.java:
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/analysis/TableDef.java@545
PS5, Line 545: if (parentTable instanceof HdfsTable) {
> unnecessary blank line
Done
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/analysis/TableDef.java@547
PS5, Line 547: // instead of HdfsTable.getPrimaryKeysSql() when we implement PK/FK feature
: // for LocalCatal
> Is this no longer required to deal with the Hive bug described in the comme
Done. I meant to modify the check. Updated the comment.
http://gerrit.cloudera.org:8080/#/c/14720/5/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/14720/5/fe/src/main/java/org/apache/impala/catalog/Table.java@114
PS5, Line 114: // List of primary keys associated with the table.
> brief comment
Done
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/catalog/Table.java@116
PS5, Line 116:
> brief comment
Done
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/catalog/Table.java@608
PS5, Line 608: public List<Column> getColumns() { return colsByPos_; }
> // FeTable
Done
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java:
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java@530
PS5, Line 530:
> Would it be better, here and elsewhere, to do something like throw a NotImp
That's a good idea. Done.
http://gerrit.cloudera.org:8080/#/c/14720/5/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/14720/5/fe/src/main/java/org/apache/impala/service/MetadataOp.java@254
PS5, Line 254: INT.to
> As far as I can tell, looks like the types you have here don't quite match
Thanks for the catch. The type mismatch was not intentional.
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/service/MetadataOp.java@266
PS5, Line 266: * Contains lists of databases, lists of table belonging to the dbs, list of columns
> This comment is stale now
Updated this comment to include PKs and FKs
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/service/MetadataOp.java@623
PS5, Line 623: tableM
> formatting
Done
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/service/MetadataOp.java@649
PS5, Line 649: }
: if (LOG.isTraceEnabled()) LOG.trace("
> Since it doesn't fit on one line, you should use {}, here and below
Done
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/service/MetadataOp.java@662
PS5, Line 662: * the wait timeout has been reached.
> nit: formatting
Done
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/service/MetadataOp.java@701
PS5, Line 701: row.colVals.add(EMPTY_COL_VAL); // PKTABLE_CAT
> Might be nice to note what each of the EMPTY_COL_VALs are for, eg. at the e
Done
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/service/MetadataOp.java@736
PS5, Line 736: Lists.newArrayList
> Lists.newArrayList
Done. Just curious: How is this different from new ArrayList<>()?
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/test/java/org/apache/impala/service/JdbcTest.java
File fe/src/test/java/org/apache/impala/service/JdbcTest.java:
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/test/java/org/apache/impala/service/JdbcTest.java@452
PS5, Line 452: assertEqua
> assertEquals?
Done
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/test/java/org/apache/impala/service/JdbcTest.java@454
PS5, Line 454: // TODO (IMPALA-9158): Remove this check.
> Maybe add a TODO: IMPALA-9158
Done
http://gerrit.cloudera.org:8080/#/c/14720/5/testdata/data/child_table.txt
File testdata/data/child_table.txt:
http://gerrit.cloudera.org:8080/#/c/14720/5/testdata/data/child_table.txt@1
PS5, Line 1: 101,1,1951,1001
> You can add a description of this and the other tables to testdata/data/REA
Done
http://gerrit.cloudera.org:8080/#/c/14720/5/tests/hs2/test_hs2.py
File tests/hs2/test_hs2.py:
http://gerrit.cloudera.org:8080/#/c/14720/5/tests/hs2/test_hs2.py@500
PS5, Line 500: pks = ["id", "yea
> Why? Are there any tests that exercise the path where the table isn't alrea
Tests pass without these. Removed them.
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 7
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 19 Nov 2019 01:43:34 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
Patch Set 10:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5095/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 10
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 02:27:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
Posted by "Anurag Mantripragada (Code Review)" <ge...@cloudera.org>.
Anurag Mantripragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
......................................................................
Patch Set 1:
Looking for suggestions on overall approach and authorization piece. Thank you!
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 1
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Nov 2019 23:56:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
Patch Set 7:
(13 comments)
Btw, not sure if you're familiar with clang-format, some instructions here: https://cwiki.apache.org/confluence/display/IMPALA/Contributing+to+Impala, but it can help a lot to reduce the number of formatting mistakes and make the review process easier
http://gerrit.cloudera.org:8080/#/c/14720/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14720/7//COMMIT_MSG@38
PS7, Line 38: We should fix
: this seperately.
Is there a JIRA for it?
http://gerrit.cloudera.org:8080/#/c/14720/7/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:
http://gerrit.cloudera.org:8080/#/c/14720/7/common/thrift/Frontend.thrift@576
PS7, Line 576: GET_CROSS_REFERECE
GET_CROSS_REFERENCE
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/analysis/TableDef.java
File fe/src/main/java/org/apache/impala/analysis/TableDef.java:
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/analysis/TableDef.java@547
PS7, Line 547:
nit: extra whitespace
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/catalog/FeTable.java
File fe/src/main/java/org/apache/impala/catalog/FeTable.java:
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/catalog/FeTable.java@99
PS7, Line 99: throws NotImplementedException;
I don't think this is necessary, here and elsewhere
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java:
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java@56
PS7, Line 56: import org.apache.impala.common.NotImplementedException;
I don't think you need this
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java:
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java@216
PS7, Line 216: "getPrimaryKeys() is not yet implemented in "
: + "LocalCatalog Mode"
nit: you can get rid of the '+' by putting this all on the second line
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/service/Frontend.java@907
PS7, Line 907: } catch (NotImplementedException e) {
: // getForeignKeys() is not supported in LocalCatalog mode. Until IMPALA-9158 is
: // fixed, return an empty list.
: return Lists.newArrayList();
My thinking in suggesting the NotImplementedException was that we would bubble it up to users so that they know that its not something that's expected to work yet.
I'm not certain that would actually work, eg. because of the way this is getting called through JNI, and I suppose it doesn't really matter since I see that you've already got a patch out for IMPALA-9158. Feel free to leave this the way it is, just as long as you clean it up in that patch.
http://gerrit.cloudera.org:8080/#/c/14720/5/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/14720/5/fe/src/main/java/org/apache/impala/service/MetadataOp.java@736
PS5, Line 736: Lists.newArrayList
> Done. Just curious: How is this different from new ArrayList<>()?
Good question - I just know if it as our usual convention, eg. if you look at the rest of this file.
Apparently though Lists.newArrayList() is supposed to be deprecated as of Java 7, see: https://guava.dev/releases/19.0/api/docs/com/google/common/collect/Lists.html#newArrayList() and the way you had it previously is now considered the preferable way
Of course, it can be a bit of a judgement call whether to go with the "preferred" style or to match the existing style in the file you're modifying, but some quick grepping suggests that we already use the "diamond" style more overall across the FE than Lists.newArrayList, so you can feel free to put this back the way that it was.
http://gerrit.cloudera.org:8080/#/c/14720/7/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/14720/7/fe/src/main/java/org/apache/impala/service/MetadataOp.java@650
PS7, Line 650: if (LOG.isTraceEnabled()) LOG.trace("Returning {} primary keys for table {}.",
: result.rows.size(), tableName);
formatting
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/service/MetadataOp.java@676
PS7, Line 676: , user
formatting
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/service/MetadataOp.java@720
PS7, Line 720: if (LOG.isTraceEnabled()) LOG.trace("Returning {} foreign keys for table {}.",
: result.rows.size(), foreignTableName);
formatting
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java
File fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java:
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java@348
PS7, Line 348: previleges
typo
http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java@373
PS7, Line 373:
nit: unnecessary whitespace
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 7
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 19 Nov 2019 19:46:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
Patch Set 12: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 12
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 22:25:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
......................................................................
Patch Set 2:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/5032/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 2
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sat, 16 Nov 2019 00:45:29 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Anurag Mantripragada (Code Review)" <ge...@cloudera.org>.
Anurag Mantripragada has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
The goal is to let JDBC clients get constraint information
from Impala tables. We implement two new metadata operations in
impala-hs2-server, GetPrimaryKeys and GetCrossReference, whose thrift
definitions are taken from Hive. For these to work,
new TCLIService methods were added. In FE, these two operations are
implemented to get the information from tables in the catalog.
Much like GetColumns(), tables need to be loaded in order to be able to get
PK/FK information. We wait for the PK table/FK table to load.
In the implementation, PK/FK information is returned
ONLY if the user has access to ALL the columns involved in the PK/FK
relationship.
Testing:
- Added three test tables to our test datasets since most of our FE tests
relied on dummy tables or testdata. It was difficult to test PK/FK with
these methods. Also, we can build on this testdata in future when we make
optimizer improvements.
- Added unit tests in AuthorizationTest and JDBCtest.
- Added e2e test in test_hs2.py
Caveats:
- Ranger needs OWNER user information for authorization. Since this is HMS
metadata that we do not aggresively load, this information is not available
for IncompleteTables. Some foreign key tables (fact tables for example)
might have FK/PK relationships with several PK tables some of which might
not be loaded in catalog. Currently we have no way to check column
previleges without owner user information tables. We do not return keys
involving such columns. Therefore, when Ranger is used, there maybe missing
PK/FK relationships for parent tables that are not loaded. This can be
tracked in IMPALA-9172.
- Retrieval of constraints is not yet supported in LocalCatalog mode. See
IMPALA-9158.
Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
---
M be/src/rpc/kerberos-test.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.h
M bin/rat_exclude_files.txt
M common/thrift/Frontend.thrift
M common/thrift/hive-1-api/TCLIService.thrift
M fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/main/java/org/apache/impala/analysis/TableDef.java
M fe/src/main/java/org/apache/impala/catalog/FeTable.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/MetadataOp.java
M fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
M testdata/data/README
A testdata/data/child_table.txt
A testdata/data/parent_table.txt
A testdata/data/parent_table_2.txt
M testdata/datasets/functional/functional_schema_template.sql
M tests/hs2/test_hs2.py
24 files changed, 803 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/14720/8
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 8
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
Posted by "Anurag Mantripragada (Code Review)" <ge...@cloudera.org>.
Anurag Mantripragada has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
......................................................................
IMPALA-9104: Add support for retrieval of primary keys
and foriegn keys through impala-hs2-server.
The goal is to let JDBC clients get constraint information
from Impala tables. We implement two new metadata operations in
impala-hs2-server, GetPrimaryKeys and GetCrossReference. For these to work,
new TCLIService methods were added. In FE, these two operations are
implemented to get the information from tables in the catalog.
Much like GetColumns(), tables need to be loaded in order to be able to get
PK/FK information. We wait for the PK table/FK table to load.
In the implementation, PK/FK information is returned
ONLY if the user has access to ALL the columns involved in the PK/FK
relationship.
Testing:
- Added three test tables to our test datasets since most of our FE tests
relied on dummy tables or testdata. It was difficult to test PK/FK with
these methods. Also, we can build on this testdata in future when we make
optimizer improvements.
- Added unit tests in AuthorizationTest and JDBCtest.
- Added e2e test in test_hs2.py
Caveats:
- Authorization information is not available for IncompleteTables. In
case of foreign key tables, it is not feasible to load all the
parent tables involved to check for column privileges. Therefore, there
maybe missing PK/FK relationships for parent tables that are not loaded.
- Retrieval of constraints is not yet supported in LocalCatalog mode. See
IMPALA-9158.
Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
---
M be/src/rpc/kerberos-test.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.h
M bin/rat_exclude_files.txt
M common/thrift/Frontend.thrift
M common/thrift/hive-1-api/TCLIService.thrift
M fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/main/java/org/apache/impala/analysis/TableDef.java
M fe/src/main/java/org/apache/impala/catalog/FeTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/MetadataOp.java
M fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
A testdata/data/child_table.txt
A testdata/data/parent_table.txt
A testdata/data/parent_table_2.txt
M testdata/datasets/functional/functional_schema_template.sql
M tests/hs2/test_hs2.py
22 files changed, 789 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/14720/6
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 6
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5224/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 3
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sat, 16 Nov 2019 02:58:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Anurag Mantripragada (Code Review)" <ge...@cloudera.org>.
Anurag Mantripragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
Patch Set 12:
The test failures in GVD were because we started returning Immutable lists for PKs/FKs. In IMPALA-2112 we were using dummy tables for tests which used to addPrimaryKeys in Frontend fixture. After we started returning Immutable list, this code broke. However, since we added new dataset in this patch, I modified the PK/FK tests to use the newly added dataset and removed the line where pk's had to be added in FrontendFixture.
The changes are in FrontEndFixture, AnalyzeDDLTest and ToSqlTest files.
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 12
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 17:41:22 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
......................................................................
Patch Set 5:
(20 comments)
http://gerrit.cloudera.org:8080/#/c/14720/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14720/5//COMMIT_MSG@7
PS5, Line 7: Add support for retrieval of primary keys
: and foriegn keys through impala-hs2-server.
nit: keep the title of the commit message to one line
http://gerrit.cloudera.org:8080/#/c/14720/5//COMMIT_MSG@12
PS5, Line 12: GetPrimaryKeys and GetCrossReference
Might be worth explicitly mentioning that the thrift definitions for these were taken directly from Hive, and ways that our behavior differs from Hive's (if any)
http://gerrit.cloudera.org:8080/#/c/14720/5//COMMIT_MSG@31
PS5, Line 31: - Authorization information is not available for IncompleteTables. In
Could you expand on this a little more, in particular why is not feasible, is there any plan to ever fix this, is this consistent with Hive's behavior, etc.
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/analysis/TableDef.java
File fe/src/main/java/org/apache/impala/analysis/TableDef.java:
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/analysis/TableDef.java@545
PS5, Line 545:
unnecessary blank line
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/analysis/TableDef.java@547
PS5, Line 547: // TODO: Remove this check when we implement PK/FK feature for LocalCatalog
: // (IMPALA-9158).
Is this no longer required to deal with the Hive bug described in the comment above?
http://gerrit.cloudera.org:8080/#/c/14720/5/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/14720/5/fe/src/main/java/org/apache/impala/catalog/Table.java@114
PS5, Line 114: protected final List<SQLPrimaryKey> primaryKeys_ = new ArrayList<>();
brief comment
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/catalog/Table.java@116
PS5, Line 116: protected final List<SQLForeignKey> foreignKeys_ = new ArrayList<>();
brief comment
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/catalog/Table.java@608
PS5, Line 608: @Override
// FeTable
Might as well match the rest of this file
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java:
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java@530
PS5, Line 530: return new ArrayList<>();
Would it be better, here and elsewhere, to do something like throw a NotImplementedException or similar to make sure we're not silently returning incorrect results?
http://gerrit.cloudera.org:8080/#/c/14720/5/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/14720/5/fe/src/main/java/org/apache/impala/service/MetadataOp.java@254
PS5, Line 254: STRING
As far as I can tell, looks like the types you have here don't quite match what is used in Hive, in particular I think Hive uses INT for UPDATE_RULE, DELETE_RULE, and DEFERRABILITY. see: https://github.com/apache/hive/blob/master/service/src/java/org/apache/hive/service/cli/operation/GetCrossReferenceOperation.java#L91
Was it intentional to not match Hive on this?
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/service/MetadataOp.java@266
PS5, Line 266: * Contains lists of databases, lists of table belonging to the dbs, list of columns
This comment is stale now
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/service/MetadataOp.java@623
PS5, Line 623: , user
formatting
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/service/MetadataOp.java@649
PS5, Line 649: if (LOG.isTraceEnabled()) LOG.trace("Returning " + result.rows.size() + " primary "
: + "keys for table " + tableName);
Since it doesn't fit on one line, you should use {}, here and below
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/service/MetadataOp.java@662
PS5, Line 662:
nit: formatting
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/service/MetadataOp.java@701
PS5, Line 701: row.colVals.add(EMPTY_COL_VAL);
Might be nice to note what each of the EMPTY_COL_VALs are for, eg. at the end of this line add // PKTABLE_CAT
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/service/MetadataOp.java@736
PS5, Line 736: new ArrayList<>();
Lists.newArrayList
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/test/java/org/apache/impala/service/JdbcTest.java
File fe/src/test/java/org/apache/impala/service/JdbcTest.java:
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/test/java/org/apache/impala/service/JdbcTest.java@452
PS5, Line 452: assertTrue
assertEquals?
http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/test/java/org/apache/impala/service/JdbcTest.java@454
PS5, Line 454: if (!TestUtils.isCatalogV2Enabled("localhost", 25020)) {
Maybe add a TODO: IMPALA-9158
http://gerrit.cloudera.org:8080/#/c/14720/5/testdata/data/child_table.txt
File testdata/data/child_table.txt:
http://gerrit.cloudera.org:8080/#/c/14720/5/testdata/data/child_table.txt@1
PS5, Line 1: 101,1,1951,1001
You can add a description of this and the other tables to testdata/data/README
http://gerrit.cloudera.org:8080/#/c/14720/5/tests/hs2/test_hs2.py
File tests/hs2/test_hs2.py:
http://gerrit.cloudera.org:8080/#/c/14720/5/tests/hs2/test_hs2.py@500
PS5, Line 500: # Load the table.
Why? Are there any tests that exercise the path where the table isn't already loaded when we do GetPrimaryKeys()?
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 5
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 18 Nov 2019 19:58:38 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
Patch Set 12:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5267/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 12
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 17:45:29 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
Patch Set 11: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/5264/
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 11
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 11:01:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
Patch Set 8:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5077/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 8
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 20 Nov 2019 01:26:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
The goal is to let JDBC clients get constraint information
from Impala tables. We implement two new metadata operations in
impala-hs2-server, GetPrimaryKeys and GetCrossReference, which are
already implemented in Hive's HS2. The thrift
definitions are copied from Hive's TCLIService.thrift. In FE, these
two operations are implemented to get the information from tables
in the catalog.
Much like GetColumns(), tables need to be loaded in order to be able to get
PK/FK information. We wait for the PK table/FK table to load.
In the implementation, PK/FK information is returned
ONLY if the user has access to ALL the columns involved in the PK/FK
relationship.
Testing:
- Added three test tables to our test datasets since most of our FE tests
relied on dummy tables or testdata. It was difficult to test PK/FK with
these methods. Also, we can build on this testdata in future when we make
optimizer improvements.
- Added unit tests in AuthorizationTest and JDBCtest.
- Added e2e test in test_hs2.py
- This patch modifies AnalyzeDDLTests and ToSqlTests to rely on the newly
added dataset instead of dummy tables for pk/fk tests.
Caveats:
- Ranger needs OWNER user information for authorization. Since this is HMS
metadata that we do not aggresively load, this information is not available
for IncompleteTables. Some foreign key tables (fact tables for example)
might have FK/PK relationships with several PK tables some of which might
not be loaded in catalog. Currently we have no way to check column
previleges without owner user information tables. We do not return keys
involving such columns. Therefore, when Ranger is used, there maybe missing
PK/FK relationships for parent tables that are not loaded. This can be
tracked in IMPALA-9172.
- Retrieval of constraints is not yet supported in LocalCatalog mode. See
IMPALA-9158.
Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Reviewed-on: http://gerrit.cloudera.org:8080/14720
Reviewed-by: Vihang Karajgaonkar <vi...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/rpc/kerberos-test.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.h
M bin/rat_exclude_files.txt
M common/thrift/Frontend.thrift
M common/thrift/hive-1-api/TCLIService.thrift
M fe/src/main/java/org/apache/impala/analysis/TableDef.java
M fe/src/main/java/org/apache/impala/catalog/FeTable.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/MetadataOp.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
M testdata/data/README
A testdata/data/child_table.txt
A testdata/data/parent_table.txt
A testdata/data/parent_table_2.txt
M testdata/datasets/functional/functional_schema_template.sql
M tests/hs2/test_hs2.py
25 files changed, 835 insertions(+), 48 deletions(-)
Approvals:
Vihang Karajgaonkar: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 13
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14720/10//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14720/10//COMMIT_MSG@13
PS10, Line 13: new TCLIService methods were added. In FE, these two operations are
Apologies for the drive-by comment, but I think it'd be worth mentioning that these operations are already implemented by Hive's HS2 and that the definitions were copied from Hive's TCLIService.thrift.
I initially wasn't sure whether this was an impala-specific extension or not.
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 10
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 02:13:09 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
Patch Set 9: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 9
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 01:12:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Anurag Mantripragada (Code Review)" <ge...@cloudera.org>.
Anurag Mantripragada has uploaded a new patch set (#11). ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
The goal is to let JDBC clients get constraint information
from Impala tables. We implement two new metadata operations in
impala-hs2-server, GetPrimaryKeys and GetCrossReference, which are
already implemented in Hive's HS2. The thrift
definitions are copied from Hive's TCLIService.thrift. In FE, these
two operations are implemented to get the information from tables
in the catalog.
Much like GetColumns(), tables need to be loaded in order to be able to get
PK/FK information. We wait for the PK table/FK table to load.
In the implementation, PK/FK information is returned
ONLY if the user has access to ALL the columns involved in the PK/FK
relationship.
Testing:
- Added three test tables to our test datasets since most of our FE tests
relied on dummy tables or testdata. It was difficult to test PK/FK with
these methods. Also, we can build on this testdata in future when we make
optimizer improvements.
- Added unit tests in AuthorizationTest and JDBCtest.
- Added e2e test in test_hs2.py
Caveats:
- Ranger needs OWNER user information for authorization. Since this is HMS
metadata that we do not aggresively load, this information is not available
for IncompleteTables. Some foreign key tables (fact tables for example)
might have FK/PK relationships with several PK tables some of which might
not be loaded in catalog. Currently we have no way to check column
previleges without owner user information tables. We do not return keys
involving such columns. Therefore, when Ranger is used, there maybe missing
PK/FK relationships for parent tables that are not loaded. This can be
tracked in IMPALA-9172.
- Retrieval of constraints is not yet supported in LocalCatalog mode. See
IMPALA-9158.
Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
---
M be/src/rpc/kerberos-test.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.h
M bin/rat_exclude_files.txt
M common/thrift/Frontend.thrift
M common/thrift/hive-1-api/TCLIService.thrift
M fe/src/main/java/org/apache/impala/analysis/TableDef.java
M fe/src/main/java/org/apache/impala/catalog/FeTable.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/MetadataOp.java
M fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
M testdata/data/README
A testdata/data/child_table.txt
A testdata/data/parent_table.txt
A testdata/data/parent_table_2.txt
M testdata/datasets/functional/functional_schema_template.sql
M tests/hs2/test_hs2.py
22 files changed, 809 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/14720/11
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 11
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
Patch Set 9:
(2 comments)
The patch looks good to me. Both the comments below are non-blocking and can be done in a follow-up later if required.
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/main/java/org/apache/impala/service/Frontend.java@888
PS8, Line 888: if (!authzChecker_.get().hasAccess(user, fkPrivilegeRequest) ||
: !authzChecker_.get().hasAccess(user, pkPrivilegeRequest)) {
: omitList.add(fkName);
: }
> There are these cases:
May be I don't understand what you mean by sequence here. Can you give an example and add it to the method doc so that its clear.
http://gerrit.cloudera.org:8080/#/c/14720/8/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/14720/8/fe/src/main/java/org/apache/impala/service/MetadataOp.java@393
PS8, Line 393: }
: columns.addAll(fe.getColumns(table, columnPatternMa
> We always match all columns for getPrimaryKeys() request. See line 629. It
Actually, it looks like if the column matcher is NONE, we get the table if its loaded. Hence, the columns.addAll() call at the 392 can be executed with the matcher is NONE. In such a case, we might end up returning primaryKeys when the column matcher is NONE. I thnk that should be okay though. What do you think?
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 9
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 01:10:59 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
Patch Set 8: Code-Review+1
> (13 comments)
>
> The developer docs page says clang can be used only with the C++
> part of the code. I was manually doing this and still miss a few
> code style errors. Clang documentation says it can be used for Java
> too. Maybe I should try and modify our clang tool to work with
> Java. Thank you for catching these errors.
Oh, I don't think there's any reason why you can't run clang-format as is on the Java parts of Impala (I do it all the time). To be clear, you should think of the clang-format output as a guideline, as there will be some places that it differs from our preferred style, so always feel free to ignore it if its suggestions differ from the style of the surrounding code.
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 8
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 20 Nov 2019 18:22:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Anurag Mantripragada (Code Review)" <ge...@cloudera.org>.
Anurag Mantripragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
Patch Set 9:
(13 comments)
Thanks for the comments Vihang.
http://gerrit.cloudera.org:8080/#/c/14720/8/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:
http://gerrit.cloudera.org:8080/#/c/14720/8/common/thrift/Frontend.thrift@594
PS8, Line 594:
> hmm, adding fields in the middle generally is considered a bad practice sin
Okay, will add new fields instead of reusing old ids.
http://gerrit.cloudera.org:8080/#/c/14720/8/common/thrift/hive-1-api/TCLIService.thrift
File common/thrift/hive-1-api/TCLIService.thrift:
http://gerrit.cloudera.org:8080/#/c/14720/8/common/thrift/hive-1-api/TCLIService.thrift@954
PS8, Line 954: catalogName
> Just curious to know if this is the Hive's catalog name or the catalog serv
This is the catalog name. https://github.com/apache/hive/blob/df8e185aeb555f522345d5703cd5375aad2ae4b4/service/src/java/org/apache/hive/service/cli/operation/GetPrimaryKeysOperation.java#L79
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java
File fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java:
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java@226
PS8, Line 226: TGetSchemasReq req = request.getGet_schemas_req();
: return MetadataOp.getSchemas(
: frontend, req.getCatalogName(), req.getSchemaName(), user);
: }
:
: /**
: * Supported HMS-2 types
: */
: public static final EnumSet<TableType> IMPALA_SUPPORTED_TABLE_TYPES = EnumSet
: .of(TableType.EXTERNAL_TABLE, TableType.MANAGED_TABLE, TableType.VIRTUAL_VIEW);
:
: /**
: * mapping between the HMS-2 type the Impala types
: */
: p
> Are these methods are exactly the same as in case of hive-3 shim? If yes, w
Removed the shim methods. Used MetadataOp directly.
http://gerrit.cloudera.org:8080/#/c/14720/8/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/14720/8/fe/src/main/java/org/apache/impala/catalog/Table.java@610
PS8, Line 610:
: @Override // FeTable
: public List<SQLPrimaryKey> getPrimaryKeys() {
: // Prevent clients from modifying the primary keys list.
: return ImmutableList.copyOf(primaryKeys_);
> Generally a good practice is to make these methods return Immutable objects
Done
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java:
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java@215
PS8, Line 215: // TODO: return primary k
> replace with Collection.emptyList()?
Done.
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java@221
PS8, Line 221: / TODO: return foreign k
> replace with Collection.emptyList()?
Done
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/main/java/org/apache/impala/service/Frontend.java@888
PS8, Line 888: if (!authzChecker_.get().hasAccess(user, fkPrivilegeRequest) ||
: !authzChecker_.get().hasAccess(user, pkPrivilegeRequest)) {
: omitList.add(fkName);
: }
> Isn't this logic same as
There are these cases:
```
if(auth enabled) {
if (any of the keys in sequence doesn't have access) {
don't return the entire sequence but continue checking other fks.
} else {
return all fks;
} else {
return all fks as is.
}
```
A sequence of keys has a common fk name. We are storing all fk names in which at least one FK has no permissions and skip the entire sequence in the end. Does this make sense?
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/main/java/org/apache/impala/service/Frontend.java@1649
PS8, Line 1649: case GET_PRIMARY_KEYS: return MetadataOp.getPrimaryKeys(this, request,
: user);
: case GET_CROSS_REFE
> If the methods are exactly the same in both the shims, we should directly u
Done
http://gerrit.cloudera.org:8080/#/c/14720/8/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/14720/8/fe/src/main/java/org/apache/impala/service/MetadataOp.java@393
PS8, Line 393: }
: columns.addAll(fe.getColumns(table, columnPatternMa
> do we have to exclude the keys which do not match the columnPatternMatcher?
We always match all columns for getPrimaryKeys() request. See line 629. It did not make sense to request just certain key columns especially when there could be composite primary keys. Also, when columnPatternMatcher is NONE, we do not load table metadata hence the primary keys list will be empty, this will essentially be a no-op.
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/main/java/org/apache/impala/service/MetadataOp.java@658
PS8, Line 658:
: return result;
: }
:
: /**
: * Executes the GetCrossReference Hi
> Can you please add documentation on what this method is doing with respect
Done
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/main/java/org/apache/impala/service/MetadataOp.java@692
PS8, Line 692: mn metadata.
: StmtMetadataLoader mdLoader = new StmtMetada
> suggest you change this to we filter out the foreignKeys which are not matc
Done
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/main/java/org/apache/impala/service/MetadataOp.java@697
PS8, Line 697: nMatcher.MATCHER_MATCH_NONE, user);
> Do we need these as a filtering criteria? seems like we are already pulling
That's correct, we already filter the tables using fkTableName. Removed foreignSchemaName and ForeignTableName as filtering criteria.
http://gerrit.cloudera.org:8080/#/c/14720/8/fe/src/main/java/org/apache/impala/service/MetadataOp.java@729
PS8, Line 729: }
: }
> Can you be more specific here. It was not very clear to me until I read the
Done
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 9
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 20 Nov 2019 23:56:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
Posted by "Anurag Mantripragada (Code Review)" <ge...@cloudera.org>.
Anurag Mantripragada has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
......................................................................
IMPALA-9104: Add support for retrieval of primary keys
and foriegn keys through impala-hs2-server.
The goal is to let JDBC clients get constraint information
from Impala tables. We implement two new metadata operations in
impala-hs2-server, GetPrimaryKeys and GetCrossReference. For these to work,
new TCLIService methods were added. In FE, these two operations are
implemented to get the information from tables in the catalog.
Much like GetColumns(), tables need to be loaded in order to be able to get
PK/FK information. We wait for the PK table/FK table to load.
In the implementation, PK/FK information is returned
ONLY if the user has access to ALL the columns involved in the PK/FK
relationship.
Testing:
- Added three test tables to our test datasets since most of our FE tests
relied on dummy tables or testdata. It was difficult to test PK/FK with
these methods. Also, we can build on this testdata in future when we make
optimizer improvements.
- Added unit tests in AuthorizationTest and JDBCtest.
- Added e2e test in test_hs2.py
Caveats:
- Authorization information is not available for IncompleteTables. In
case of foreign key tables, it is not feasible to load all the
parent tables involved to check for column privileges. Therefore, there
maybe missing PK/FK relationships for parent tables that are not loaded.
- Retrieval of constraints is not yet supported in LocalCatalog mode. See
IMPALA-9158.
Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
---
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.h
M common/thrift/Frontend.thrift
M common/thrift/hive-1-api/TCLIService.thrift
M fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/main/java/org/apache/impala/analysis/TableDef.java
M fe/src/main/java/org/apache/impala/catalog/FeTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/MetadataOp.java
M fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
M testdata/datasets/functional/functional_schema_template.sql
M tests/hs2/test_hs2.py
17 files changed, 763 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/14720/2
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 2
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5038/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 3
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sat, 16 Nov 2019 02:45:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
......................................................................
Patch Set 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5237/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 6
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 18 Nov 2019 19:00:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Anurag Mantripragada (Code Review)" <ge...@cloudera.org>.
Anurag Mantripragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14720/10//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14720/10//COMMIT_MSG@13
PS10, Line 13: definitions are copied from Hive's TCLIService.thrift. In FE, these
> Apologies for the drive-by comment, but I think it'd be worth mentioning th
Done.
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 11
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 02:56:27 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
Patch Set 12: Code-Review+2
Thanks for fixing the failing tests. Retriggered the GVD.
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 12
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 17:45:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
......................................................................
Patch Set 1:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/5031/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 1
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sat, 16 Nov 2019 00:38:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14720/1/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/14720/1/fe/src/main/java/org/apache/impala/service/MetadataOp.java@670
PS1, Line 670: PatternMatcher schemaMatcher = PatternMatcher.createJdbcPatternMatcher(foreignSchemaName);
line too long (94 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 1
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Nov 2019 23:56:25 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
......................................................................
Patch Set 4:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/5040/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 4
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sat, 16 Nov 2019 22:06:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
Patch Set 7:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5057/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 7
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 19 Nov 2019 02:27:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
Patch Set 11:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5264/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 11
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 06:21:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Anurag Mantripragada (Code Review)" <ge...@cloudera.org>.
Anurag Mantripragada has uploaded a new patch set (#10). ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
The goal is to let JDBC clients get constraint information
from Impala tables. We implement two new metadata operations in
impala-hs2-server, GetPrimaryKeys and GetCrossReference, whose thrift
definitions are taken from Hive. For these to work,
new TCLIService methods were added. In FE, these two operations are
implemented to get the information from tables in the catalog.
Much like GetColumns(), tables need to be loaded in order to be able to get
PK/FK information. We wait for the PK table/FK table to load.
In the implementation, PK/FK information is returned
ONLY if the user has access to ALL the columns involved in the PK/FK
relationship.
Testing:
- Added three test tables to our test datasets since most of our FE tests
relied on dummy tables or testdata. It was difficult to test PK/FK with
these methods. Also, we can build on this testdata in future when we make
optimizer improvements.
- Added unit tests in AuthorizationTest and JDBCtest.
- Added e2e test in test_hs2.py
Caveats:
- Ranger needs OWNER user information for authorization. Since this is HMS
metadata that we do not aggresively load, this information is not available
for IncompleteTables. Some foreign key tables (fact tables for example)
might have FK/PK relationships with several PK tables some of which might
not be loaded in catalog. Currently we have no way to check column
previleges without owner user information tables. We do not return keys
involving such columns. Therefore, when Ranger is used, there maybe missing
PK/FK relationships for parent tables that are not loaded. This can be
tracked in IMPALA-9172.
- Retrieval of constraints is not yet supported in LocalCatalog mode. See
IMPALA-9158.
Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
---
M be/src/rpc/kerberos-test.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.h
M bin/rat_exclude_files.txt
M common/thrift/Frontend.thrift
M common/thrift/hive-1-api/TCLIService.thrift
M fe/src/main/java/org/apache/impala/analysis/TableDef.java
M fe/src/main/java/org/apache/impala/catalog/FeTable.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/MetadataOp.java
M fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
M testdata/data/README
A testdata/data/child_table.txt
A testdata/data/parent_table.txt
A testdata/data/parent_table_2.txt
M testdata/datasets/functional/functional_schema_template.sql
M tests/hs2/test_hs2.py
22 files changed, 809 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/14720/10
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 10
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5232/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 5
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 18 Nov 2019 01:23:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
Patch Set 11:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5096/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 11
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 03:44:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Add support for retrieval of primary keys and foriegn keys through impala-hs2-server.
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5049/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 6
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 18 Nov 2019 19:44:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server.
......................................................................
Patch Set 12:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5099/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477
Gerrit-Change-Number: 14720
Gerrit-PatchSet: 12
Gerrit-Owner: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 18:22:50 +0000
Gerrit-HasComments: No