[Impala-ASF-CR] IMPALA-11871: Skip permissions loading and check on HDFS if Ranger is enabled
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20221 ) Change subject: IMPALA-11871: Skip permissions loading and check on HDFS if Ranger is enabled .. Patch Set 11: I revised the commit message and added an end-to-end test for the LOAD DATA statement in the patch set 11. -- To view, visit http://gerrit.cloudera.org:8080/20221 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id33c400fbe0c918b6b65d713b09009512835a4c9 Gerrit-Change-Number: 20221 Gerrit-PatchSet: 11 Gerrit-Owner: Halim Kim Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Halim Kim Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Sat, 13 Apr 2024 00:59:39 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11871: Skip permissions loading and check on HDFS if Ranger is enabled
Fang-Yu Rao has uploaded a new patch set (#11) to the change originally created by Halim Kim. ( http://gerrit.cloudera.org:8080/20221 ) Change subject: IMPALA-11871: Skip permissions loading and check on HDFS if Ranger is enabled .. IMPALA-11871: Skip permissions loading and check on HDFS if Ranger is enabled Before this patch, Impala checked whether the Impala service user had the WRITE access to the target HDFS table/partition(s) during the analysis of the INSERT and LOAD DATA statements in the legacy catalog mode. The access levels of the corresponding HDFS table and partitions were computed by the catalog server solely based on the HDFS permissions and ACLs when the table and partitions were instantiated. After this patch, we skip loading HDFS permissions and assume the Impala service user has the READ_WRITE permission on all the HDFS paths associated with the target table during query analysis when Ranger is enabled. The assumption could be removed after Impala's implementation of FsPermissionChecker could additionally take Ranger's policies of HDFS into consideration when performing the check. Testing: - Added end-to-end tests to verify Impala's behavior with respect to the INSERT and LOAD DATA statements when Ranger is enabled in the legacy catalog mode. Change-Id: Id33c400fbe0c918b6b65d713b09009512835a4c9 --- M bin/rat_exclude_files.txt M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java A testdata/data/load_data_with_catalog_v1.txt M tests/authorization/test_ranger.py 4 files changed, 147 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/20221/11 -- To view, visit http://gerrit.cloudera.org:8080/20221 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id33c400fbe0c918b6b65d713b09009512835a4c9 Gerrit-Change-Number: 20221 Gerrit-PatchSet: 11 Gerrit-Owner: Halim Kim Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Halim Kim Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang
[Impala-ASF-CR] IMPALA-11871: Skip permissions check on HDFS files if Ranger is enabled
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20221 ) Change subject: IMPALA-11871: Skip permissions check on HDFS files if Ranger is enabled .. Patch Set 10: I slightly revised the patch set 9 and added an end-to-end test. Will try to see if we could add another test for the LOAD DATA statement. -- To view, visit http://gerrit.cloudera.org:8080/20221 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id33c400fbe0c918b6b65d713b09009512835a4c9 Gerrit-Change-Number: 20221 Gerrit-PatchSet: 10 Gerrit-Owner: Halim Kim Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Halim Kim Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Thu, 11 Apr 2024 01:16:23 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11871: Skip permissions check on HDFS files if Ranger is enabled
Fang-Yu Rao has uploaded a new patch set (#10) to the change originally created by Halim Kim. ( http://gerrit.cloudera.org:8080/20221 ) Change subject: IMPALA-11871: Skip permissions check on HDFS files if Ranger is enabled .. IMPALA-11871: Skip permissions check on HDFS files if Ranger is enabled This patch assumes the Impala service user has the READ_WRITE permission on all the HDFS paths associated with a table during query analysis when Ranger is enabled. The assumption could be removed after Impala's implementation of FsPermissionChecker could additionally take Ranger's policies of HDFS into consideration when performing the check. Testing: - Added an end-to-end test to verify Impala's behavior with respect to the INSERT statement when Ranger is enabled in the legacy catalog mode. Change-Id: Id33c400fbe0c918b6b65d713b09009512835a4c9 --- M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java M tests/authorization/test_ranger.py 2 files changed, 59 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/20221/10 -- To view, visit http://gerrit.cloudera.org:8080/20221 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id33c400fbe0c918b6b65d713b09009512835a4c9 Gerrit-Change-Number: 20221 Gerrit-PatchSet: 10 Gerrit-Owner: Halim Kim Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Halim Kim Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang
[Impala-ASF-CR] IMPALA-12921: Support locally built Ranger
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/21160 ) Change subject: IMPALA-12921: Support locally built Ranger .. Patch Set 7: Hi all, please let me know if you have any comment on this patch (patch set 7). This patch is similar to https://gerrit.cloudera.org/c/17094/ where we added the support for locally built Hive (IMPALA-9218). The patch set 6 passed the dryrun tests at https://jenkins.impala.io/job/gerrit-verify-dryrun/10516/. In patch set 7, I only removed an exclusion from fe/pom.xml and updated some files that would not affect the functionality or the build of Impala when CDP dependencies are used. Thank you very much for the help! -- To view, visit http://gerrit.cloudera.org:8080/21160 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I268d6d4d6e371da7497aac8d12f78178d57c6f27 Gerrit-Change-Number: 21160 Gerrit-PatchSet: 7 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: John Sherman Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Wed, 10 Apr 2024 20:28:09 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12921: Support locally built Ranger
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/21160
to look at the new patch set (#7).
Change subject: IMPALA-12921: Support locally built Ranger
..
IMPALA-12921: Support locally built Ranger
This patch does the following.
1. Add the support for locally built Ranger.
2. Resolve IMPALA-12985 by switching to the new constructor when
instantiating RangerAccessRequestImpl.
The instructions on how to build Ranger locally and point
Impala to the locally built Ranger server are provided as below.
Suppose the Ranger project is under the folder $RANGER_SRC_DIR. We could
execute the following to build Ranger. By default, the compressed
tarball is produced under $RANGER_SRC_DIR/target.
mvn clean compile -B -nsu -DskipCheck=true -Dcheckstyle.skip=true \
package install -DskipITs -DskipTests -Dmaven.javadoc.skip=true
After building Ranger, we need to build Impala's Java code so that
Impala's Java code could consume the locally produced Ranger classes. We
will need to export the following environment variables before building
Impala. This prevents bootstrap_toolchain.py from trying to download the
compressed Ranger tarball. Moreover, we need to apply
IMPALA-12921_pom_patching.diff on Impala if the locally built Ranger
pulls in an incompatible version of hive-storage-api.
1. export RANGER_VERSION_OVERRIDE=\
$(mvn -f $RANGER_SRC_DIR/pom.xml -q help:evaluate \
-Dexpression=project.version -DforceStdout)
2. export RANGER_HOME_OVERRIDE=$RANGER_SRC_DIR/target/\
ranger-${RANGER_VERSION_OVERRIDE}-admin
It then suffices to execute the following to point
Impala to the locally built Ranger server before starting Impala.
1. source $IMPALA_HOME/bin/impala-config.sh
2. tar zxv -f $RANGER_SRC_DIR/target/\
ranger-${IMPALA_RANGER_VERSION}-admin.tar.gz \
-C $RANGER_SRC_DIR/target/
3. $IMPALA_HOME/bin/create-test-configuration.sh
4. $IMPALA_HOME/bin/create-test-configuration.sh \
-create_ranger_policy_db
5. $IMPALA_HOME/testdata/bin/run-ranger.sh
(run-all.sh has to be executed instead if other underlying services
have not been started)
6. $IMPALA_HOME/testdata/bin/setup-ranger.sh
Testing:
- Manually verified that we could point Impala to a locally built
Apache Ranger on the master branch (with tip being RANGER-4745).
- Manually verified that with the Ranger patch in RANGER-4771.diff,
we could manage the policy repository via GRANT/REVOKE statements.
- Verified that this patch passed the core tests.
Change-Id: I268d6d4d6e371da7497aac8d12f78178d57c6f27
---
M README-build.md
M bin/bootstrap_toolchain.py
M bin/create-test-configuration.sh
M bin/impala-config.sh
M bin/rat_exclude_files.txt
M
fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java
M
fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java
M testdata/bin/setup-ranger.sh
A testdata/cluster/ranger/IMPALA-12921_pom_patching.diff
A testdata/cluster/ranger/RANGER-4771.diff
A testdata/cluster/ranger/README
R testdata/cluster/ranger/setup/all_database_policy_revised.json.template
M testdata/cluster/ranger/setup/impala_user_non_owner.json.template
M testdata/cluster/ranger/setup/impala_user_owner.json.template
14 files changed, 126 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/21160/7
--
To view, visit http://gerrit.cloudera.org:8080/21160
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I268d6d4d6e371da7497aac8d12f78178d57c6f27
Gerrit-Change-Number: 21160
Gerrit-PatchSet: 7
Gerrit-Owner: Fang-Yu Rao
Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-12921: Support locally built Ranger
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/21160
to look at the new patch set (#6).
Change subject: IMPALA-12921: Support locally built Ranger
..
IMPALA-12921: Support locally built Ranger
This patch does the following.
1. Add the support for locally built Ranger.
2. Resolve IMPALA-12985 by switching to the new constructor when
instantiating RangerAccessRequestImpl.
The instructions on how to build Ranger locally and point
Impala to the locally built Ranger server are provided as below.
Suppose the Ranger project is under the folder $RANGER_SRC_DIR. We could
execute the following to build Ranger. By default, the compressed
tarball is produced under $RANGER_SRC_DIR/target.
mvn clean compile -B -nsu -DskipCheck=true -Dcheckstyle.skip=true \
package install -DskipITs -DskipTests -Dmaven.javadoc.skip=true
After building Ranger, we need to build Impala's Java code so that
Impala's Java code could consume the locally produced Ranger classes. We
will need to export the following environment variables before building
Impala. This prevents bootstrap_toolchain.py from trying to download the
compressed Ranger tarball.
1. export RANGER_VERSION_OVERRIDE=\
$(mvn -f $RANGER_SRC_DIR/pom.xml -q help:evaluate \
-Dexpression=project.version -DforceStdout)
2. export RANGER_HOME_OVERRIDE=$RANGER_SRC_DIR/target/\
ranger-${RANGER_VERSION_OVERRIDE}-admin
It then suffices to execute the following to point
Impala to the locally built Ranger server before starting Impala.
1. source $IMPALA_HOME/bin/impala-config.sh
2. tar zxv -f $RANGER_SRC_DIR/target/\
ranger-${IMPALA_RANGER_VERSION}-admin.tar.gz \
-C $RANGER_SRC_DIR/target/
3. $IMPALA_HOME/bin/create-test-configuration.sh
4. $IMPALA_HOME/bin/create-test-configuration.sh \
-create_ranger_policy_db
5. $IMPALA_HOME/testdata/bin/run-ranger.sh
(run-all.sh has to be executed instead if other underlying services
have not been started)
6. $IMPALA_HOME/testdata/bin/setup-ranger.sh
Testing:
- Manually verified that we could point Impala to a locally built
Apache Ranger on the master branch (with tip being RANGER-4745).
- Manually verified that with the Ranger patch in RANGER-4771.diff,
we could manage the policy repository via GRANT/REVOKE statements.
- Verified that this patch passed the core tests.
Change-Id: I268d6d4d6e371da7497aac8d12f78178d57c6f27
---
M bin/bootstrap_toolchain.py
M bin/create-test-configuration.sh
M bin/impala-config.sh
M bin/rat_exclude_files.txt
M fe/pom.xml
M
fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java
M
fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java
M testdata/bin/setup-ranger.sh
A testdata/cluster/ranger/RANGER-4771.diff
A testdata/cluster/ranger/README
R testdata/cluster/ranger/setup/all_database_policy_revised.json.template
M testdata/cluster/ranger/setup/impala_user_non_owner.json.template
M testdata/cluster/ranger/setup/impala_user_owner.json.template
13 files changed, 96 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/21160/6
--
To view, visit http://gerrit.cloudera.org:8080/21160
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I268d6d4d6e371da7497aac8d12f78178d57c6f27
Gerrit-Change-Number: 21160
Gerrit-PatchSet: 6
Gerrit-Owner: Fang-Yu Rao
Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-12921: Support locally built Ranger
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/21160
to look at the new patch set (#5).
Change subject: IMPALA-12921: Support locally built Ranger
..
IMPALA-12921: Support locally built Ranger
This patch does the following.
1. Add the support for locally built Ranger.
2. Resolve IMPALA-12985 by switching to the new constructor when
instantiating RangerAccessRequestImpl.
The instructions on how to build Ranger locally and point
Impala to the locally built Ranger server are provided as below.
Suppose the Ranger project is under the folder $RANGER_SRC_DIR. We could
execute the following to build Ranger. By default, the compressed
tarball is produced under $RANGER_SRC_DIR/target.
mvn clean compile -B -nsu -DskipCheck=true -Dcheckstyle.skip=true \
package install -DskipITs -DskipTests -Dmaven.javadoc.skip=true
After building Ranger, we need to build Impala's Java code so that
Impala's Java code could consume the locally produced Ranger classes. We
will need to export the following environment variables before building
Impala. This prevents bootstrap_toolchain.py from trying to download the
compressed Ranger tarball.
1. export RANGER_VERSION_OVERRIDE=\
$(mvn -f $RANGER_SRC_DIR/pom.xml -q help:evaluate \
-Dexpression=project.version -DforceStdout)
2. export RANGER_HOME_OVERRIDE=$RANGER_SRC_DIR/target/\
ranger-${RANGER_VERSION_OVERRIDE}-admin
It then suffices to execute the following to point
Impala to the locally built Ranger server before starting Impala.
1. source $IMPALA_HOME/bin/impala-config.sh
2. tar zxv -f $RANGER_SRC_DIR/target/\
ranger-${IMPALA_RANGER_VERSION}-admin.tar.gz \
-C $RANGER_SRC_DIR/target/
3. $IMPALA_HOME/bin/create-test-configuration.sh
4. $IMPALA_HOME/bin/create-test-configuration.sh \
-create_ranger_policy_db
5. $IMPALA_HOME/testdata/bin/run-ranger.sh
(run-all.sh has to be executed instead if other underlying services
have not been started)
6. $IMPALA_HOME/testdata/bin/setup-ranger.sh
Testing:
- Manually verified that we could point Impala to a locally built
Apache Ranger on the master branch (with tip being RANGER-4745).
- Manually verified that with the Ranger patch in RANGER-4771.diff,
we could manage the policy repository via GRANT/REVOKE statements.
- Verified that this patch passed the core tests.
Change-Id: I268d6d4d6e371da7497aac8d12f78178d57c6f27
---
M bin/bootstrap_toolchain.py
M bin/create-test-configuration.sh
M bin/impala-config.sh
M fe/pom.xml
M
fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java
M
fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java
M testdata/bin/setup-ranger.sh
A testdata/cluster/ranger/RANGER-4771.diff
A testdata/cluster/ranger/README
R testdata/cluster/ranger/setup/all_database_policy_revised.json.template
M testdata/cluster/ranger/setup/impala_user_non_owner.json.template
M testdata/cluster/ranger/setup/impala_user_owner.json.template
12 files changed, 92 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/21160/5
--
To view, visit http://gerrit.cloudera.org:8080/21160
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I268d6d4d6e371da7497aac8d12f78178d57c6f27
Gerrit-Change-Number: 21160
Gerrit-PatchSet: 5
Gerrit-Owner: Fang-Yu Rao
Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-12921: [WIP] Support locally built Ranger
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/21160
to look at the new patch set (#4).
Change subject: IMPALA-12921: [WIP] Support locally built Ranger
..
IMPALA-12921: [WIP] Support locally built Ranger
This patch adds the support for locally built Ranger in Impala's
minicluster. The instructions on how to build Ranger locally and point
Impala to the locally built Ranger server are provided as below.
Suppose the Ranger project is under the folder $RANGER_SRC_DIR. We
execute the following to build Ranger. By default, the compressed tar
files are produced under $RANGER_SRC_DIR/target.
mvn clean compile -B -nsu -DskipCheck=true -Dcheckstyle.skip=true \
package install -DskipITs -DskipTests -Dmaven.javadoc.skip=true
We also need to build Impala's Java code so that Impala's Java code
could consume the locally produced Ranger classes. We will need to
export the following environment variables before building Impala.
1. export RANGER_VERSION_OVERRIDE=\
$(mvn -f $RANGER_SRC_DIR/pom.xml -q help:evaluate \
-Dexpression=project.version -DforceStdout)
2. export RANGER_HOME_OVERRIDE=$RANGER_SRC_DIR/target/\
ranger-${RANGER_VERSION_OVERRIDE}-admin
It then suffices to execute the following to point
Impala to the locally built Ranger server.
1. source $IMPALA_HOME/bin/impala-config.sh
2. tar zxv -f $RANGER_SRC_DIR/target/\
ranger-${IMPALA_RANGER_VERSION}-admin.tar.gz \
-C $RANGER_SRC_DIR/target/
3. $IMPALA_HOME/bin/create-test-configuration.sh
4. $IMPALA_HOME/bin/create-test-configuration.sh \
-create_ranger_policy_db
5. $IMPALA_HOME/testdata/bin/run-ranger-server.sh
6. $IMPALA_HOME/testdata/bin/setup-ranger.sh
To-do:
- Resolve IMPALA-12985.
- Wait for RANGER-4771 to be resolved, or include a Ranger patch in
this patch that resolves the issue.
Change-Id: I268d6d4d6e371da7497aac8d12f78178d57c6f27
---
M bin/bootstrap_toolchain.py
M bin/create-test-configuration.sh
M bin/impala-config.sh
M fe/pom.xml
M testdata/bin/setup-ranger.sh
R testdata/cluster/ranger/setup/all_database_policy_revised.json.template
M testdata/cluster/ranger/setup/impala_user_non_owner.json.template
M testdata/cluster/ranger/setup/impala_user_owner.json.template
8 files changed, 44 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/21160/4
--
To view, visit http://gerrit.cloudera.org:8080/21160
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I268d6d4d6e371da7497aac8d12f78178d57c6f27
Gerrit-Change-Number: 21160
Gerrit-PatchSet: 4
Gerrit-Owner: Fang-Yu Rao
Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-12921: [WIP] Support locally built Ranger
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/21160
to look at the new patch set (#3).
Change subject: IMPALA-12921: [WIP] Support locally built Ranger
..
IMPALA-12921: [WIP] Support locally built Ranger
This patch adds the support for locally built Ranger in Impala's
minicluster. The instructions on how to build Ranger locally and point
Impala to the locally built Ranger server are provided as below.
Suppose the Ranger project is under the folder $RANGER_SRC_DIR. We
execute the following to build Ranger. By default, the compressed tar
files are produced under $RANGER_SRC_DIR/target.
mvn clean compile -B -nsu -DskipCheck=true -Dcheckstyle.skip=true \
package install -DskipITs -DskipTests -Dmaven.javadoc.skip=true
We also need to build Impala's Java code so that Impala's Java code
could consume the locally produced Ranger classes.
It then suffices to execute the following to point
Impala to the locally built Ranger server.
1. export RANGER_VERSION_OVERRIDE=\
$(mvn -f $RANGER_SRC_DIR/pom.xml -q help:evaluate \
-Dexpression=project.version -DforceStdout)
2. export RANGER_HOME_OVERRIDE=$RANGER_SRC_DIR/target/\
ranger-${RANGER_VERSION_OVERRIDE}-admin
3. source $IMPALA_HOME/bin/impala-config.sh
4. tar zxv -f $RANGER_SRC_DIR/target/\
ranger-${IMPALA_RANGER_VERSION}-admin.tar.gz \
-C $RANGER_SRC_DIR/target/
5. $IMPALA_HOME/bin/create-test-configuration.sh
6. $IMPALA_HOME/bin/create-test-configuration.sh \
-create_ranger_policy_db
7. $IMPALA_HOME/testdata/bin/run-ranger-server.sh
8. $IMPALA_HOME/testdata/bin/setup-ranger.sh
To-do:
- Wait for RANGER-4770 to be resolved.
- Wait for RANGER-4771 to be resolved.
Change-Id: I268d6d4d6e371da7497aac8d12f78178d57c6f27
---
M bin/impala-config.sh
M testdata/bin/setup-ranger.sh
R testdata/cluster/ranger/setup/all_database_policy_revised.json.template
M testdata/cluster/ranger/setup/impala_user_non_owner.json.template
M testdata/cluster/ranger/setup/impala_user_owner.json.template
5 files changed, 25 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/21160/3
--
To view, visit http://gerrit.cloudera.org:8080/21160
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I268d6d4d6e371da7497aac8d12f78178d57c6f27
Gerrit-Change-Number: 21160
Gerrit-PatchSet: 3
Gerrit-Owner: Fang-Yu Rao
Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-12291 impala checks hdfs ranger policy
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20221 ) Change subject: IMPALA-12291 impala checks hdfs ranger policy .. Patch Set 9: Hi all, based on our discussion so far, there are 2 approaches discussed. Any any better alternative is also appreciated! - Set the access level of a loaded table to both READ and WRITE as long as Ranger is used as the authorization provider. - Introduce a startup flag to allow the administrator to decide whether to skip the file system permissions check during table loading. There are some implementation details to consider for each approach. - How to determine whether Ranger is enabled (corresponding to the 1st approach)? It seems checking the value of the key 'dfs.namenode.inode.attributes.provider.class' in core-site.xml via the instance of Configuration as done in the patch set 9 could not be verified easily via a new test due to HDFS Ranger plug-in not being configured correctly. To be more specific, if we try to add the following configuration via https://github.com/apache/impala/blob/master/testdata/cluster/node_templates/common/etc/hadoop/conf/core-site.xml.py, the name node of HDFS could not be started. More plumbing has to be done to set up the plug-in. On the other hand, using the instance of BackendConfig allows us to add an end-to-end test to briefly verify Impala's behavior after the patch more easily. dfs.namenode.inode.attributes.provider.class org.apache.ranger.authorization.hadoop.RangerHdfsAuthorizer - What should be the default behavior with respect to setting the access level of a loaded table after this patch (corresponding to the 2nd approach)? It looks like making Impala assume the READ and WRITE access by default may be better especially in the legacy catalog mode. This relieves the Impala administrator of having to manually tweak the HDFS access control entries of the target HDFS paths that are not writable to the Impala service every time when an end user encounters such a problem. I have also collected the related tests that need to be revised if we decide to adopt the 2nd approach and make Impala assume the READ and WRITE access by default. - https://github.com/apache/impala/blob/master/tests/metadata/test_hdfs_permissions.py#L56 (TestHdfsPermissions.test_insert_into_read_only_table()). - https://github.com/apache/impala/blob/master/tests/query_test/test_insert_behaviour.py#L563 (TestInsertBehaviour.test_multiple_group_acls). - https://github.com/apache/impala/blob/master/tests/query_test/test_insert_behaviour.py#L331 (TestInsertBehaviour.test_readonly_table_dir). - https://github.com/apache/impala/blob/master/tests/query_test/test_insert_behaviour.py#L362 (TestInsertBehaviour.test_insert_acl_permissions). - https://github.com/apache/impala/blob/master/tests/query_test/test_insert_behaviour.py#L439 (TestInsertBehaviour.test_load_permissions). - https://github.com/apache/impala/blob/master/tests/query_test/test_insert_behaviour.py#L252 (TestInsertBehaviour.test_mixed_partition_permissions). - https://github.com/apache/impala/blob/master/tests/query_test/test_insert_behaviour.py#L202 (TestInsertBehaviour.test_insert_file_permissions). - https://github.com/apache/impala/blob/master/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java#L4008 (AnalyzeStmtsTest.TestLoadData). - https://github.com/apache/impala/blob/master/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java#L4042-L4047 (AnalyzeStmtsTest.TestInsert). - https://github.com/apache/impala/blob/master/fe/src/test/java/org/apache/impala/catalog/CatalogObjectToFromThriftTest.java#L110-L119 (CatalogObjectToFromThriftTest.TestPartitionedTable). -- To view, visit http://gerrit.cloudera.org:8080/20221 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id33c400fbe0c918b6b65d713b09009512835a4c9 Gerrit-Change-Number: 20221 Gerrit-PatchSet: 9 Gerrit-Owner: Halim Kim Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Halim Kim Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Fri, 05 Apr 2024 18:30:54 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12291 impala checks hdfs ranger policy
Fang-Yu Rao has posted comments on this change. (
http://gerrit.cloudera.org:8080/20221 )
Change subject: IMPALA-12291 impala checks hdfs ranger policy
..
Patch Set 9:
(2 comments)
Thanks for the reply Halim!
Other than changing how the catalog server initializes the access level of an
HdfsTable during table loading, it may also make sense to disable
analyzeWriteAccess() at
https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java#L406-L408
depending on the value of the newly introduced startup flag.
I do not have a strong objection against adding a new startup flag in this
patch. But maybe Quanlong and Aman could chime in to see if there is a better
alternative since we have already got a lot of flags.
http://gerrit.cloudera.org:8080/#/c/20221/5/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
http://gerrit.cloudera.org:8080/#/c/20221/5/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@852
PS5, Line 852: location = location.getParent();
> Your test code will be very helpful. Thank you Fang-Yu.
Thanks Halim! Not a problem.
Adding a startup flag like 'hdfs_permission_check' or
'skip_fs_permissions_check_in_analysis' to give the Impala administrator a
choice is reasonable and it does not have to depend on whether Ranger is
enabled.
It would be good that we mention the purpose of not performing the file system
permissions check somewhere (maybe in the commit message).
This prevents users from encountering an AnalysisException during query
analysis when the target table or any partition inside is not writable
to the Impala service according to Impala's FsPermissionChecker() even
though the Impala service user is granted the READ and WRITE privileges
on the respective file system paths via the Ranger policy repository of the
corresponding storage.
If we add such a startup flag, e.g., 'skip_fs_permissions_check_in_analysis',
then we could still add end-to-end tests to
https://github.com/apache/impala/blob/master/tests/authorization/test_ranger.py
like the following to make sure file system permissions check is still
performed if the Impala administrator does not want to skip it.
@pytest.mark.execute_serially
@SkipIfFS.hdfs_acls
@CustomClusterTestSuite.with_args(
impalad_args="{0} {1}".format(CATALOGD_ARGS,
"--skip_fs_permissions_check_in_analysis=false"),
catalogd_args="{0} {1}".format(CATALOGD_ARGS,
"--skip_fs_permissions_check_in_analysis=false"))
def test_insert_with_catalog_v1_not_skip_fs_permissions_check(self,
unique_name):
self._test_insert_with_catalog_v1(unique_name, False)
def _test_insert_with_catalog_v1(self, unique_name,
skip_fs_permissions_check=True):
"""
Test that when Ranger is the authorization provider in the legacy catalog
mode,
Impala skips or performs the file system permissions checking in query
analysis
depending on the startup flag 'skip_fs_permissions_check_in_analysis'.
"""
user = getuser()
admin_client = self.create_impala_client()
unique_database = unique_name + "_db"
unique_table = unique_name + "_tbl"
table_path = "test-warehouse/{0}.db/{1}".format(unique_database,
unique_table)
try:
admin_client.execute("drop database if exists {0} cascade"
.format(unique_database), user=ADMIN)
admin_client.execute("create database {0}".format(unique_database),
user=ADMIN)
admin_client.execute("create table {0}.{1} (x int)"
.format(unique_database, unique_table), user=ADMIN)
admin_client.execute("grant insert on table {0}.{1} to user {2}"
.format(unique_database, unique_table, user))
# Change the owner user and group of the HDFS path corresponding to the
table
# so that according to Impala's FsPermissionChecker, the table could not
be
# writable to the user that loads the table. This user usually is the one
# representing the Impala service.
self.hdfs_client.chown(table_path, "another_user", "another_group")
# Invalidate the table metadata to force the catalog server to reload the
HDFS
# table.
admin_client.execute("invalidate metadata {0}.{1}"
.format(unique_database, unique_table), user=ADMIN)
# Verify that Impala skips or performs the permissions checking in
# HdfsTable#getAvailableAccessLevel() depending on the startup flag
# 'skip_fs_permissions_check_in_analysis'.
query = "insert into {0}.{1} values (1)".format(unique_database,
unique_table)
if skip_fs_permissions_check:
self._run_query_as_user(query, user, True)
else:
result = self._run_query_as_user(query, user, False)
err = "Unable to INSERT into target table"
[Impala-ASF-CR] IMPALA-12291 impala checks hdfs ranger policy
Fang-Yu Rao has posted comments on this change. (
http://gerrit.cloudera.org:8080/20221 )
Change subject: IMPALA-12291 impala checks hdfs ranger policy
..
Patch Set 9:
(2 comments)
Thanks for the help Halim! I left some comments in HdfsTable.java. Let us know
what you think about them.
http://gerrit.cloudera.org:8080/#/c/20221/5/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
http://gerrit.cloudera.org:8080/#/c/20221/5/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@852
PS5, Line 852: location = location.getParent();
> Thank you for your detailed review.
Thanks for the suggestion Quanlong!
I think at the moment skipping the permissions checking is not a bad idea.
Specifically, we could change the if-statement at
https://gerrit.cloudera.org/c/20221/5/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java#846
to something like the following as Halim suggested above.
Note that instead of introducing a new variable denoting whether HDFS' Ranger
plug-in is enabled, I just use the BackendConfig instance to verify whether the
authorization provider is Ranger. Thus when Impala's Ranger plug-in is not
enabled and the table is not stored on the listed file systems in the cloud,
permissions checking would still occur.
So if there is any permission-related issue during the execution by Impala's
backend, we will have a runtime error after this patch as Quanlong mentioned
but by skipping the permissions checking we won't have a regression in
performance especially when there is a large number of partitions within a
table.
if (assumeReadWriteAccess(fs) ||
BackendConfig.INSTANCE.getAuthorizationProvider().equalsIgnoreCase("ranger")) {
return TAccessLevel.READ_WRITE;
}
I would also suggest we add the following end-to-end test in
https://github.com/apache/impala/blob/master/tests/authorization/test_ranger.py
to verify Impala frontend's behavior after this patch. I verified that it would
work with my change suggested above.
@pytest.mark.execute_serially
@SkipIfFS.hdfs_acls
@CustomClusterTestSuite.with_args(
impalad_args=IMPALAD_ARGS, catalogd_args=CATALOGD_ARGS)
def test_insert_with_catalog_v1(self, unique_name):
"""
Test that when Ranger is the authorization provider in the legacy catalog
mode,
Impala does not throw an AnalysisException when an authorized user tries to
execute
an INSERT query against a table which is not writable according to Impala's
FsPermissionChecker.
"""
user = getuser()
admin_client = self.create_impala_client()
unique_database = unique_name + "_db"
unique_table = unique_name + "_tbl"
table_path = "test-warehouse/{0}.db/{1}".format(unique_database,
unique_table)
try:
admin_client.execute("drop database if exists {0} cascade"
.format(unique_database), user=ADMIN)
admin_client.execute("create database {0}".format(unique_database),
user=ADMIN)
admin_client.execute("create table {0}.{1} (x int)"
.format(unique_database, unique_table), user=ADMIN)
admin_client.execute("grant insert on table {0}.{1} to user {2}"
.format(unique_database, unique_table, user))
# Change the owner user and group of the HDFS path corresponding to the
table
# so that according to Impala's FsPermissionChecker, the table could not
be
# writable to the user that loads the table. This user usually is the one
# representing the Impala service.
self.hdfs_client.chown(table_path, "another_user", "another_group")
# Invalidate the table metadata to force the catalog server to reload the
HDFS
# table.
admin_client.execute("invalidate metadata {0}.{1}"
.format(unique_database, unique_table), user=ADMIN)
# Verify that the user granted the INSERT privilege on the table does not
encounter
# the AnalysisException that would have been thrown if we had not skipped
the
# permissions checking in HdfsTable#getAvailableAccessLevel().
self._run_query_as_user("insert into {0}.{1} values (1)"
.format(unique_database, unique_table), user, True)
finally:
admin_client.execute("revoke insert on table {0}.{1} from user {2}"
.format(unique_database, unique_table, user))
admin_client.execute("drop database if exists {0} cascade"
.format(unique_database), user=ADMIN)
http://gerrit.cloudera.org:8080/#/c/20221/5/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/20221/5/fe/src/main/java/org/apache/impala/service/Frontend.java@2109
PS5, Line 2109: LOG.warn("Analysis Exception query {}: {}",
: queryCtx.client_request.stmt, errorMsg);
Not very
[Impala-ASF-CR] IMPALA-12921: [WIP] Support locally built Ranger
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/21160
to look at the new patch set (#2).
Change subject: IMPALA-12921: [WIP] Support locally built Ranger
..
IMPALA-12921: [WIP] Support locally built Ranger
This patch adds the support for locally built Ranger in Impala's
minicluster. The instructions on how to build Ranger locally and point
Impala to the locally built Ranger server are provided as below.
Suppose the Ranger project is under the folder $RANGER_SRC_DIR. We
execute the following to build Ranger. By default, the compressed tar
files are produced under $RANGER_SRC_DIR/target.
mvn clean compile -B -nsu -DskipCheck=true -Dcheckstyle.skip=true \
package install -DskipITs -DskipTests -Dmaven.javadoc.skip=true
It then suffices to execute the following to point
Impala to the locally built Ranger server.
1. export RANGER_VERSION_OVERRIDE=\
$(mvn -f $RANGER_SRC_DIR/pom.xml -q help:evaluate \
-Dexpression=project.version -DforceStdout)
2. export RANGER_HOME_OVERRIDE=$RANGER_SRC_DIR/target/\
ranger-${RANGER_VERSION_OVERRIDE}-admin
3. source $IMPALA_HOME/bin/impala-config.sh
4. tar zxv $RANGER_SRC_DIR/target/\
ranger-${IMPALA_RANGER_VERSION}-admin.tar.gz \
-C $RANGER_SRC_DIR/target/
5. $IMPALA_HOME/bin/create-test-configuration.sh
6. $IMPALA_HOME/bin/create-test-configuration.sh \
-create_ranger_policy_db
7. $IMPALA_HOME/testdata/bin/run-ranger-server.sh
8. $IMPALA_HOME/testdata/bin/setup-ranger.sh
To-do:
- Currently we are not able to manage the privileges via the GRANT or
REVOKE statements in impala-shell. Verified that it is due to a
recent Ranger commit (RANGER-4445), where
ServiceREST#ensureAdminAccess() is additionally invoked in
grantAccess() and revokeAccess(). Will need to figure out how to
resolve this.
Change-Id: I268d6d4d6e371da7497aac8d12f78178d57c6f27
---
M bin/impala-config.sh
M testdata/bin/setup-ranger.sh
R testdata/cluster/ranger/setup/all_database_policy_revised.json.template
M testdata/cluster/ranger/setup/impala_user_non_owner.json.template
M testdata/cluster/ranger/setup/impala_user_owner.json.template
5 files changed, 25 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/21160/2
--
To view, visit http://gerrit.cloudera.org:8080/21160
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I268d6d4d6e371da7497aac8d12f78178d57c6f27
Gerrit-Change-Number: 21160
Gerrit-PatchSet: 2
Gerrit-Owner: Fang-Yu Rao
Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-12921: [WIP] Support locally built Ranger
Fang-Yu Rao has uploaded this change for review. (
http://gerrit.cloudera.org:8080/21160
Change subject: IMPALA-12921: [WIP] Support locally built Ranger
..
IMPALA-12921: [WIP] Support locally built Ranger
This patch adds the support for locally built Ranger in Impala's
minicluster. Suppose that we have already built Ranger locally under the
folder $RANGER_SRC_DIR, and that the produced tar files are under
$RANGER_SRC_DIR/target. It suffices to execute the following to start
the locally built Ranger server.
1. export RANGER_VERSION_OVERRIDE=\
$(mvn -f $RANGER_SRC_DIR/pom.xml -q help:evaluate \
-Dexpression=project.version -DforceStdout)
2. export RANGER_HOME_OVERRIDE=$RANGER_SRC_DIR/target/\
ranger-${RANGER_VERSION_OVERRIDE}-admin
3. source $IMPALA_HOME/bin/impala-config.sh
4. tar zxv $RANGER_SRC_DIR/target/\
ranger-${IMPALA_RANGER_VERSION}-admin.tar.gz
5. $IMPALA_HOME/bin/create-test-configuration.sh
6. $IMPALA_HOME/bin/create-test-configuration.sh \
-create_ranger_policy_db
7. $IMPALA_HOME/testdata/bin/run-ranger-server.sh
8. $IMPALA_HOME/testdata/bin/setup-ranger.sh
To-do:
- Currently the GRANT/REVOKE APIs do not work with Apache Impala.
We should figure out why 'userName' in
ServiceREST#ensureAdminAccess() is null.
Change-Id: I268d6d4d6e371da7497aac8d12f78178d57c6f27
---
M bin/impala-config.sh
M testdata/bin/setup-ranger.sh
R testdata/cluster/ranger/setup/all_database_policy_revised.json.template
M testdata/cluster/ranger/setup/impala_user_non_owner.json.template
M testdata/cluster/ranger/setup/impala_user_owner.json.template
5 files changed, 25 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/21160/1
--
To view, visit http://gerrit.cloudera.org:8080/21160
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I268d6d4d6e371da7497aac8d12f78178d57c6f27
Gerrit-Change-Number: 21160
Gerrit-PatchSet: 1
Gerrit-Owner: Fang-Yu Rao
[Impala-ASF-CR] IMPALA-12805: Avoid problems of HIVE-27114 without configuration changes
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/21028 ) Change subject: IMPALA-12805: Avoid problems of HIVE-27114 without configuration changes .. Patch Set 1: (1 comment) Thanks for the help Andrew! I only left a minor comment on the commit message. I do not have any other suggestion. http://gerrit.cloudera.org:8080/#/c/21028/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21028/1//COMMIT_MSG@17 PS1, Line 17: non-empty nit: Should this be "empty" instead? At https://gerrit.cloudera.org/c/20937/2/fe/src/test/resources/hive-site.xml.py#71, we set the new Hive property to an empty string. Don't know if I missed anything. -- To view, visit http://gerrit.cloudera.org:8080/21028 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3c13efd0d1619930461829fd8ee1fea66fa764f5 Gerrit-Change-Number: 21028 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Sat, 10 Feb 2024 21:28:01 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE
Fang-Yu Rao has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/20916 ) Change subject: IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE .. IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE After RANGER-1200, Ranger allows the owner user of a resource to grant/revoke a privilege to/from a grantee/revokee, which requires the client of the Ranger server to provide the ownership information in the requests for granting and revoking accesses. Before this patch, Impala did not provide its Ranger plug-in with the owner user of resource in the GRANT and REVOKE statements and thus the owner user of a resource was not able to grant/revoke a privilege to/from other principals. This patch passes to the Ranger server the owner user of resource in the GRANT and REVOKE statements when the resource is a database, a table, or a column. This way the owner user does not have to be explicitly granted additional privileges on the resource to execute the GRANT and REVOKE statements for the aforementioned resource types. For user-defined functions, we will deal with this resource type in IMPALA-12685 in that it depends on IMPALA-11743 where we will have to make Impala load from Hive MetaStore the owner user of a user-defined function. The patch also fixes a potential bug in getOwnerUser() of Db, LocalDb, Table, and LocalTable. Specifically, before this patch, when determining the owner user of a database or a table, Impala returned the owner name without verifying the corresponding principal type is indeed a user. This was problematic because the principal type could be a group or a role. In addition, we note that Ranger assumes implicitly that the provided owner is a user. This could be seen from the definition of GrantRevokeRequest. Before Ranger adds an additional field in GrantRevokeRequest to distinguish an owner user from an owner group, Impala will not be able to support allowing a user in an owner group to grant or revoke privileges on the resources owned by the owner group. Testing: - Added an end-to-end test to verify that the owner user of a resource is able to execute the GRANT/REVOKE statements without being granted additional privileges if the resource is a database, a table, or a column. Change-Id: Ibac5335c65a860963ef0ccd890a926af80585ef3 Reviewed-on: http://gerrit.cloudera.org:8080/20916 Tested-by: Impala Public Jenkins Reviewed-by: Fang-Yu Rao --- M common/thrift/JniCatalog.thrift M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java M fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java M fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/catalog/Table.java M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java M tests/authorization/test_ranger.py 10 files changed, 375 insertions(+), 23 deletions(-) Approvals: Impala Public Jenkins: Verified Fang-Yu Rao: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/20916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ibac5335c65a860963ef0ccd890a926af80585ef3 Gerrit-Change-Number: 20916 Gerrit-PatchSet: 7 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang
[Impala-ASF-CR] IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20916 ) Change subject: IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE .. Patch Set 6: I have addressed Csaba's comments on patch set 4. Will carry forward Csaba's +2 and merge the patch since the Jenkins run at https://jenkins.impala.io/job/gerrit-verify-dryrun/10254/ against this patch was successful. -- To view, visit http://gerrit.cloudera.org:8080/20916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibac5335c65a860963ef0ccd890a926af80585ef3 Gerrit-Change-Number: 20916 Gerrit-PatchSet: 6 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Thu, 08 Feb 2024 00:47:55 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20916 ) Change subject: IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/20916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibac5335c65a860963ef0ccd890a926af80585ef3 Gerrit-Change-Number: 20916 Gerrit-PatchSet: 6 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Thu, 08 Feb 2024 00:46:10 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20916 ) Change subject: IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE .. Patch Set 6: I fixed an indentation error in patch set 6. -- To view, visit http://gerrit.cloudera.org:8080/20916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibac5335c65a860963ef0ccd890a926af80585ef3 Gerrit-Change-Number: 20916 Gerrit-PatchSet: 6 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Wed, 07 Feb 2024 19:23:49 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE
Hello Quanlong Huang, Aman Sinha, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20916 to look at the new patch set (#6). Change subject: IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE .. IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE After RANGER-1200, Ranger allows the owner user of a resource to grant/revoke a privilege to/from a grantee/revokee, which requires the client of the Ranger server to provide the ownership information in the requests for granting and revoking accesses. Before this patch, Impala did not provide its Ranger plug-in with the owner user of resource in the GRANT and REVOKE statements and thus the owner user of a resource was not able to grant/revoke a privilege to/from other principals. This patch passes to the Ranger server the owner user of resource in the GRANT and REVOKE statements when the resource is a database, a table, or a column. This way the owner user does not have to be explicitly granted additional privileges on the resource to execute the GRANT and REVOKE statements for the aforementioned resource types. For user-defined functions, we will deal with this resource type in IMPALA-12685 in that it depends on IMPALA-11743 where we will have to make Impala load from Hive MetaStore the owner user of a user-defined function. The patch also fixes a potential bug in getOwnerUser() of Db, LocalDb, Table, and LocalTable. Specifically, before this patch, when determining the owner user of a database or a table, Impala returned the owner name without verifying the corresponding principal type is indeed a user. This was problematic because the principal type could be a group or a role. In addition, we note that Ranger assumes implicitly that the provided owner is a user. This could be seen from the definition of GrantRevokeRequest. Before Ranger adds an additional field in GrantRevokeRequest to distinguish an owner user from an owner group, Impala will not be able to support allowing a user in an owner group to grant or revoke privileges on the resources owned by the owner group. Testing: - Added an end-to-end test to verify that the owner user of a resource is able to execute the GRANT/REVOKE statements without being granted additional privileges if the resource is a database, a table, or a column. Change-Id: Ibac5335c65a860963ef0ccd890a926af80585ef3 --- M common/thrift/JniCatalog.thrift M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java M fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java M fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/catalog/Table.java M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java M tests/authorization/test_ranger.py 10 files changed, 375 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/20916/6 -- To view, visit http://gerrit.cloudera.org:8080/20916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ibac5335c65a860963ef0ccd890a926af80585ef3 Gerrit-Change-Number: 20916 Gerrit-PatchSet: 6 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang
[Impala-ASF-CR] IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE
Hello Quanlong Huang, Aman Sinha, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20916 to look at the new patch set (#5). Change subject: IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE .. IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE After RANGER-1200, Ranger allows the owner user of a resource to grant/revoke a privilege to/from a grantee/revokee, which requires the client of the Ranger server to provide the ownership information in the requests for granting and revoking accesses. Before this patch, Impala did not provide its Ranger plug-in with the owner user of resource in the GRANT and REVOKE statements and thus the owner user of a resource was not able to grant/revoke a privilege to/from other principals. This patch passes to the Ranger server the owner user of resource in the GRANT and REVOKE statements when the resource is a database, a table, or a column. This way the owner user does not have to be explicitly granted additional privileges on the resource to execute the GRANT and REVOKE statements for the aforementioned resource types. For user-defined functions, we will deal with this resource type in IMPALA-12685 in that it depends on IMPALA-11743 where we will have to make Impala load from Hive MetaStore the owner user of a user-defined function. The patch also fixes a potential bug in getOwnerUser() of Db, LocalDb, Table, and LocalTable. Specifically, before this patch, when determining the owner user of a database or a table, Impala returned the owner name without verifying the corresponding principal type is indeed a user. This was problematic because the principal type could be a group or a role. In addition, we note that Ranger assumes implicitly that the provided owner is a user. This could be seen from the definition of GrantRevokeRequest. Before Ranger adds an additional field in GrantRevokeRequest to distinguish an owner user from an owner group, Impala will not be able to support allowing a user in an owner group to grant or revoke privileges on the resources owned by the owner group. Testing: - Added an end-to-end test to verify that the owner user of a resource is able to execute the GRANT/REVOKE statements without being granted additional privileges if the resource is a database, a table, or a column. Change-Id: Ibac5335c65a860963ef0ccd890a926af80585ef3 --- M common/thrift/JniCatalog.thrift M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java M fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java M fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/catalog/Table.java M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java M tests/authorization/test_ranger.py 10 files changed, 375 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/20916/5 -- To view, visit http://gerrit.cloudera.org:8080/20916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ibac5335c65a860963ef0ccd890a926af80585ef3 Gerrit-Change-Number: 20916 Gerrit-PatchSet: 5 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang
[Impala-ASF-CR] IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20916 ) Change subject: IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE .. Patch Set 4: (1 comment) Hi all, I have slightly revised patch set 3 according to Csaba's comments. Let me know if there is any other suggestion. Thanks very much for the help! http://gerrit.cloudera.org:8080/#/c/20916/3/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/20916/3/tests/authorization/test_ranger.py@1289 PS3, Line 1289: if privilege == "create": > here and in the finally block: Thanks for the suggestion! I will revise this patch accordingly. -- To view, visit http://gerrit.cloudera.org:8080/20916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibac5335c65a860963ef0ccd890a926af80585ef3 Gerrit-Change-Number: 20916 Gerrit-PatchSet: 4 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Tue, 06 Feb 2024 21:28:46 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE
Hello Quanlong Huang, Aman Sinha, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20916 to look at the new patch set (#4). Change subject: IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE .. IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE After RANGER-1200, Ranger allows the owner user of a resource to grant/revoke a privilege to/from a grantee/revokee, which requires the client of the Ranger server to provide the ownership information in the requests for granting and revoking accesses. Before this patch, Impala did not provide its Ranger plug-in with the owner user of resource in the GRANT and REVOKE statements and thus the owner user of a resource was not able to grant/revoke a privilege to/from other principals. This patch passes to the Ranger server the owner user of resource in the GRANT and REVOKE statements when the resource is a database, a table, or a column. This way the owner user does not have to be explicitly granted additional privileges on the resource to execute the GRANT and REVOKE statements for the aforementioned resource types. For user-defined functions, we will deal with this resource type in IMPALA-12685 in that it depends on IMPALA-11743 where we will have to make Impala load from Hive MetaStore the owner user of a user-defined function. The patch also fixes a potential bug in getOwnerUser() of Db, LocalDb, Table, and LocalTable. Specifically, before this patch, when determining the owner user of a database or a table, Impala returned the owner name without verifying the corresponding principal type is indeed a user. This was problematic because the principal type could be a group or a role. In addition, we note that Ranger assumes implicitly that the provided owner is a user. This could be seen from the definition of GrantRevokeRequest. Before Ranger adds an additional field in GrantRevokeRequest to distinguish an owner user from an owner group, Impala will not be able to support allowing a user in an owner group to grant or revoke privileges on the resources owned by the owner group. Testing: - Added an end-to-end test to verify that the owner user of a resource is able to execute the GRANT/REVOKE statements without being granted additional privileges if the resource is a database, a table, or a column. Change-Id: Ibac5335c65a860963ef0ccd890a926af80585ef3 --- M common/thrift/JniCatalog.thrift M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java M fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java M fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/catalog/Table.java M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java M tests/authorization/test_ranger.py 10 files changed, 381 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/20916/4 -- To view, visit http://gerrit.cloudera.org:8080/20916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ibac5335c65a860963ef0ccd890a926af80585ef3 Gerrit-Change-Number: 20916 Gerrit-PatchSet: 4 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang
[Impala-ASF-CR] IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE
Fang-Yu Rao has posted comments on this change. (
http://gerrit.cloudera.org:8080/20916 )
Change subject: IMPALA-12578: Pass owner user of database and table to Ranger
in GRANT/REVOKE
..
Patch Set 3:
(2 comments)
Hi all, I have tried to address Csaba's comments on patch set 2. Hopefully
patch set 3 looks better. Let me know if there is still any additional
suggestion. Thanks!
http://gerrit.cloudera.org:8080/#/c/20916/2/tests/authorization/test_ranger.py
File tests/authorization/test_ranger.py:
http://gerrit.cloudera.org:8080/#/c/20916/2/tests/authorization/test_ranger.py@1185
PS2, Line 1185: s a column.
: self._test_grant_revoke_by_owner_on_column(privilege,
column_names,
> Optional: lot of these parameters are just constants or come getuser(). The
Thanks for the suggestion!
In the next patch I will make some of them global variables and some of them
local variables within the respective functions. Hopefully this will make it
easier to read and digest the code.
'resource_owner_role' will probably remain here since it is also needed in
_test_grant_revoke_by_owner() when we clean up the roles created at the
beginning.
http://gerrit.cloudera.org:8080/#/c/20916/2/tests/authorization/test_ranger.py@1206
PS2, Line 1206: show_grant_database_stmt = "show grant {0} {1} on database
{2}"
: set_database_owner_user_stmt = "alter database {0} set
owner user {1}"
: set_database_owner_group_stmt = "alter database {0} set
owner group {1}"
: set_database_owner_role_stmt = "alter database {0} set
owner role {1}"
: resource_owner_group = OWNER_USER
: admin_client = self.create_impala_client()
:
: try:
: self._run_query_as_user(grant_database_stmt
: .format(privilege, unique_database, grantee_type,
grantee), OWNER_USER,
: True)
: result = admin_client.execute(show_grant_database_stmt
: .format(grantee_type, grantee, unique_database),
user=ADMIN)
: TestRanger._check_privileges(result, [
: [grantee_type, grantee, unique_database, ""
> optional: Couldn't this cleanup code go into the individual functions, e.g.
Thanks!
I will move this piece of cleanup code into the individual functions in the
next patch.
--
To view, visit http://gerrit.cloudera.org:8080/20916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibac5335c65a860963ef0ccd890a926af80585ef3
Gerrit-Change-Number: 20916
Gerrit-PatchSet: 3
Gerrit-Owner: Fang-Yu Rao
Gerrit-Reviewer: Aman Sinha
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Fang-Yu Rao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Quanlong Huang
Gerrit-Comment-Date: Mon, 05 Feb 2024 23:56:36 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE
Hello Quanlong Huang, Aman Sinha, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20916 to look at the new patch set (#3). Change subject: IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE .. IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE After RANGER-1200, Ranger allows the owner user of a resource to grant/revoke a privilege to/from a grantee/revokee, which requires the client of the Ranger server to provide the ownership information in the requests for granting and revoking accesses. Before this patch, Impala did not provide its Ranger plug-in with the owner user of resource in the GRANT and REVOKE statements and thus the owner user of a resource was not able to grant/revoke a privilege to/from other principals. This patch passes to the Ranger server the owner user of resource in the GRANT and REVOKE statements when the resource is a database, a table, or a column. This way the owner user does not have to be explicitly granted additional privileges on the resource to execute the GRANT and REVOKE statements for the aforementioned resource types. For user-defined functions, we will deal with this resource type in IMPALA-12685 in that it depends on IMPALA-11743 where we will have to make Impala load from Hive MetaStore the owner user of a user-defined function. The patch also fixes a potential bug in getOwnerUser() of Db, LocalDb, Table, and LocalTable. Specifically, before this patch, when determining the owner user of a database or a table, Impala returned the owner name without verifying the corresponding principal type is indeed a user. This was problematic because the principal type could be a group or a role. In addition, we note that Ranger assumes implicitly that the provided owner is a user. This could be seen from the definition of GrantRevokeRequest. Before Ranger adds an additional field in GrantRevokeRequest to distinguish an owner user from an owner group, Impala will not be able to support allowing a user in an owner group to grant or revoke privileges on the resources owned by the owner group. Testing: - Added an end-to-end test to verify that the owner user of a resource is able to execute the GRANT/REVOKE statements without being granted additional privileges if the resource is a database, a table, or a column. Change-Id: Ibac5335c65a860963ef0ccd890a926af80585ef3 --- M common/thrift/JniCatalog.thrift M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java M fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java M fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/catalog/Table.java M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java M tests/authorization/test_ranger.py 10 files changed, 377 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/20916/3 -- To view, visit http://gerrit.cloudera.org:8080/20916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ibac5335c65a860963ef0ccd890a926af80585ef3 Gerrit-Change-Number: 20916 Gerrit-PatchSet: 3 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang
[Impala-ASF-CR] IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE
Fang-Yu Rao has posted comments on this change. (
http://gerrit.cloudera.org:8080/20916 )
Change subject: IMPALA-12578: Pass owner user of database and table to Ranger
in GRANT/REVOKE
..
Patch Set 2:
(6 comments)
Hi all, I have addressed Csaba's and Quanlong's comments on patch set 1. Please
let me know if there is any additional suggestion. Thank you very much for your
help!
http://gerrit.cloudera.org:8080/#/c/20916/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20916/1//COMMIT_MSG@33
PS1, Line 33: the principal type could be
: a group or a role.
> Will this be supported in future tasks? I.e. a user belongs to the owner gr
Thanks Quanlong!
Taking a look at the definition of the class GrantRevokeRequest at
https://github.com/apache/ranger/blob/master/agents-common/src/main/java/org/apache/ranger/plugin/util/GrantRevokeRequest.java,
I found that there is no field that denotes the type of the owner. The only
field related to the owner of a resource is 'ownerUser' and thus it seems
Ranger implicitly assumes that the provided owner corresponds to a user.
With the above being said, it looks like Impala would not be able to support
allowing a user belonging to an owner group to execute GRANT and REVOKE unless
Ranger adds an additional field in GrantRevokeRequest to distinguish an owner
user from an owner group.
For easy reference, we provide the class definition in the following.
public class GrantRevokeRequest implements Serializable {
private static final long serialVersionUID = 1L;
private String grantor;
private Set grantorGroups;
private Map resource;
private Set users;
private Set groups;
private Set roles;
private Set accessTypes;
private ListforwardedAddresses;
private String remoteIPAddress;
private Boolean delegateAdmin = Boolean.FALSE;
private Boolean enableAudit= Boolean.TRUE;
private Boolean replaceExistingPermissions = Boolean.FALSE;
private Boolean isRecursive= Boolean.FALSE;
private String clientIPAddress;
private String clientType;
private String requestData;
private String sessionId;
private String clusterName;
private String zoneName;
private String ownerUser;
...
}
http://gerrit.cloudera.org:8080/#/c/20916/1/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java
File fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java:
http://gerrit.cloudera.org:8080/#/c/20916/1/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java@236
PS1, Line 236: analyzeTargetDatabase(analyzer);
: break;
: case URI:
: Preconditions.checkNotNull(uri_);
: if (privilegeLevel_ != TPrivilegeLevel.ALL) {
: throw new AnalysisException("Only 'ALL' privilege may
be applied at " +
: "URI scope in privilege spec.");
: }
> nit: We can move codes of the precondition check and the try-catch clause i
Done
http://gerrit.cloudera.org:8080/#/c/20916/1/tests/authorization/test_ranger.py
File tests/authorization/test_ranger.py:
http://gerrit.cloudera.org:8080/#/c/20916/1/tests/authorization/test_ranger.py@1191
PS1, Line 1191: revoke_table_stmt, set_table_owner_group_stmt,
resource_owner_group,
> +1. This test has ~300 lines. It'd be better to refactor it.
Thanks Csaba and Quanlong! Yeah. Indeed. I will move the code for each case to
its respective sub-function in the next patch.
http://gerrit.cloudera.org:8080/#/c/20916/1/tests/authorization/test_ranger.py@1198
PS1, Line 1198: admin_client, revoke_column_stmt,
set_table_owner_group_stmt,
> nit: do we really need this refresh? I thought we only need it when we add/
Thanks Quanlong!
I am actually not sure whether or not after a GRANT statement we really need a
REFRESH AUTHORIZATION before a subsequent SHOW GRANT statement. I added the
REFRESH statements because I found that there are several places (but not every
place) in this test file that have the same pattern (GRANT/REVOKE, REFRESH,
SHOW GRANT). For instance, in _test_grant_revoke(), we have the following.
self.execute_query_expect_success(admin_client,
"grant select on database {0} to
{1} {2}"
.format(unique_database, kw, ident),
user=ADMIN)
self._refresh_authorization(admin_client,
[Impala-ASF-CR] IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE
Hello Quanlong Huang, Aman Sinha, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20916 to look at the new patch set (#2). Change subject: IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE .. IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE After RANGER-1200, Ranger allows the owner user of a resource to grant/revoke a privilege to/from a grantee/revokee, which requires the client of the Ranger server to provide the ownership information in the requests for granting and revoking accesses. Before this patch, Impala did not provide its Ranger plug-in with the owner user of resource in the GRANT and REVOKE statements and thus the owner user of a resource was not able to grant/revoke a privilege to/from other principals. This patch passes to the Ranger server the owner user of resource in the GRANT and REVOKE statements when the resource is a database, a table, or a column. This way the owner user does not have to be explicitly granted additional privileges on the resource to execute the GRANT and REVOKE statements for the aforementioned resource types. For user-defined functions, we will deal with this resource type in IMPALA-12685 in that it depends on IMPALA-11743 where we will have to make Impala load from Hive MetaStore the owner user of a user-defined function. The patch also fixes a potential bug in getOwnerUser() of Db, LocalDb, Table, and LocalTable. Specifically, before this patch, when determining the owner user of a database or a table, Impala returned the owner name without verifying the corresponding principal type is indeed a user. This was problematic because the principal type could be a group or a role. In addition, we note that Ranger assumes implicitly that the provided owner is a user. This could be seen from the definition of GrantRevokeRequest. Before Ranger adds an additional field in GrantRevokeRequest to distinguish an owner user from an owner group, Impala will not be able to support allowing a user in an owner group to grant or revoke privileges on the resources owned by the owner group. Testing: - Added an end-to-end test to verify that the owner user of a resource is able to execute the GRANT/REVOKE statements without being granted additional privileges if the resource is a database, a table, or a column. Change-Id: Ibac5335c65a860963ef0ccd890a926af80585ef3 --- M common/thrift/JniCatalog.thrift M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java M fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java M fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/catalog/Table.java M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java M tests/authorization/test_ranger.py 10 files changed, 371 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/20916/2 -- To view, visit http://gerrit.cloudera.org:8080/20916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ibac5335c65a860963ef0ccd890a926af80585ef3 Gerrit-Change-Number: 20916 Gerrit-PatchSet: 2 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang
[Impala-ASF-CR] IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20916 ) Change subject: IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE .. Patch Set 1: Hi all, please let me know if you have any comment on the patch. Thank you very much for the help! -- To view, visit http://gerrit.cloudera.org:8080/20916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibac5335c65a860963ef0ccd890a926af80585ef3 Gerrit-Change-Number: 20916 Gerrit-PatchSet: 1 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Thu, 18 Jan 2024 06:35:02 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE
Fang-Yu Rao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20916 Change subject: IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE .. IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE After RANGER-1200, Ranger allows the owner user of a resource to grant/revoke a privilege to/from a grantee/revokee, which requires the client of the Ranger server to provide the ownership information in the requests for granting and revoking accesses. Before this patch, Impala did not provide its Ranger plug-in with the owner user of resource in the GRANT and REVOKE statements and thus the owner user of a resource was not able to grant/revoke a privilege to/from other principals. This patch passes to the Ranger server the owner user of resource in the GRANT and REVOKE statements when the resource is a database, a table, or a column. This way the owner user does not have to be explicitly granted additional privileges on the resource to execute the GRANT and REVOKE statements for the aforementioned resource types. For user-defined functions, we will deal with this resource type in IMPALA-12685 in that it depends on IMPALA-11743 where we will have to make Impala load from Hive MetaStore the owner user of a user-defined function. The patch also fixes a potential bug in getOwnerUser() of Db, LocalDb, Table, and LocalTable. Specifically, before this patch, when determining the owner user of a database or a table, Impala returned the owner name without verifying the corresponding principal type is indeed a user. This was problematic because the principal type could be a group or a role. Testing: - Added an end-to-end test to verify that the owner user of a resource is able to execute the GRANT/REVOKE statements without being granted additional privileges if the resource is a database, a table, or a column. Change-Id: Ibac5335c65a860963ef0ccd890a926af80585ef3 --- M common/thrift/JniCatalog.thrift M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java M fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java M fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/catalog/Table.java M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java M tests/authorization/test_ranger.py 10 files changed, 342 insertions(+), 16 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/20916/1 -- To view, visit http://gerrit.cloudera.org:8080/20916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ibac5335c65a860963ef0ccd890a926af80585ef3 Gerrit-Change-Number: 20916 Gerrit-PatchSet: 1 Gerrit-Owner: Fang-Yu Rao
[Impala-ASF-CR] IMPALA-11553: Add event specific metrics in the table metrics
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20473 ) Change subject: IMPALA-11553: Add event specific metrics in the table metrics .. Patch Set 10: The patch failed the same test again: custom_cluster.test_events_custom_configs.TestEventProcessingCustomConfigs.test_self_events according to https://jenkins.impala.io/job/ubuntu-20.04-from-scratch/1290/testReport/junit/custom_cluster.test_events_custom_configs/TestEventProcessingCustomConfigs/test_self_events/. It looks like this patch has never passed the dry run test. Maybe this specific end-to-end test has to be fixed too? -- To view, visit http://gerrit.cloudera.org:8080/20473 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2428029361e610a0fcd8ed11be2ab771f03b00dd Gerrit-Change-Number: 20473 Gerrit-PatchSet: 10 Gerrit-Owner: Sai Hemanth Gantasala Gerrit-Reviewer: Anonymous Coward Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Sai Hemanth Gantasala Gerrit-Comment-Date: Fri, 22 Dec 2023 23:53:57 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11553: Add event specific metrics in the table metrics
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20473 ) Change subject: IMPALA-11553: Add event specific metrics in the table metrics .. Patch Set 10: Saw the following in the failed Jenkins run at https://jenkins.impala.io/job/ubuntu-20.04-from-scratch/1286/testReport/junit/custom_cluster.test_events_custom_configs/TestEventProcessingCustomConfigs/test_self_events/ (from https://jenkins.impala.io/job/gerrit-verify-dryrun/10092/console). Is this a known flaky test? custom_cluster/test_events_custom_configs.py:234: in test_self_events self.__run_self_events_test(unique_database, False) custom_cluster/test_events_custom_configs.py:751: in __run_self_events_test self.__exec_sql_and_check_selfevent_counter(stmt, use_impala) custom_cluster/test_events_custom_configs.py:996: in __exec_sql_and_check_selfevent_counter assert events_skipped == events_skipped_after E assert 69 == 70 -- To view, visit http://gerrit.cloudera.org:8080/20473 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2428029361e610a0fcd8ed11be2ab771f03b00dd Gerrit-Change-Number: 20473 Gerrit-PatchSet: 10 Gerrit-Owner: Sai Hemanth Gantasala Gerrit-Reviewer: Anonymous Coward Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Sai Hemanth Gantasala Gerrit-Comment-Date: Fri, 22 Dec 2023 18:31:36 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11501: Add flag to allow catalog-cache operations on masked tables
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20742 ) Change subject: IMPALA-11501: Add flag to allow catalog-cache operations on masked tables .. Patch Set 3: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/20742/2/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/20742/2/tests/authorization/test_ranger.py@1615 PS2, Line 1615: ida > Yeah, just wanted to use a short grant statement. I can change it to the mi Ack -- To view, visit http://gerrit.cloudera.org:8080/20742 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I45935654cbf05a55d740f1b04781022c271f7678 Gerrit-Change-Number: 20742 Gerrit-PatchSet: 3 Gerrit-Owner: Quanlong Huang Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Wed, 13 Dec 2023 23:28:06 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11501: Add flag to allow catalog-cache operations on masked tables
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20742 ) Change subject: IMPALA-11501: Add flag to allow catalog-cache operations on masked tables .. Patch Set 3: (1 comment) Thanks for addressing my previous comments Quanlong! I do not have any additional suggestion. It should be fine to bump the vote of code-review if Csaba does not have more comments. http://gerrit.cloudera.org:8080/#/c/20742/2/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java: http://gerrit.cloudera.org:8080/#/c/20742/2/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java@81 PS2, Line 81: Pair getTableIfPresent(String dbName, String tableName); > catalog_.getMetaProvider() returns a MetaProvider. Adding this only in Cata Ack -- To view, visit http://gerrit.cloudera.org:8080/20742 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I45935654cbf05a55d740f1b04781022c271f7678 Gerrit-Change-Number: 20742 Gerrit-PatchSet: 3 Gerrit-Owner: Quanlong Huang Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Mon, 11 Dec 2023 22:06:25 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11501: Add flag to allow catalog-cache operations on masked tables
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20742 ) Change subject: IMPALA-11501: Add flag to allow catalog-cache operations on masked tables .. Patch Set 2: Code-Review+1 (2 comments) Thanks for working on this Quanlong! I only have one additional minor question regarding whether we should add getTableIfPresent() to CatalogdMetaProvider.java only. http://gerrit.cloudera.org:8080/#/c/20742/1/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/20742/1/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java@222 PS1, Line 222: (request.getPrivilege() != Privilege.REFRESH : || !BackendConfig.INSTANCE.allowCatalogCacheOpFromMaskedUsers()) > This is just for my own understanding. I am trying to answer my questions above. It looks like db.getTable(authorizableTable.getTableName()) at https://gerrit.cloudera.org/c/20742/1/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java#231 will fully load the table metadata only in the local catalog mode. Therefore, I think this additional condition is to prevent fully loading the table metadata in the local catalog mode. http://gerrit.cloudera.org:8080/#/c/20742/2/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java: http://gerrit.cloudera.org:8080/#/c/20742/2/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java@81 PS2, Line 81: Pair getTableIfPresent(String dbName, String tableName); Should we add this method to CatalogdMetaProvider.java only? I am asking this because I saw at https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java#L76-L79 that DirectMetaProvider.java is a provider that calls out to the source systems without caching. -- To view, visit http://gerrit.cloudera.org:8080/20742 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I45935654cbf05a55d740f1b04781022c271f7678 Gerrit-Change-Number: 20742 Gerrit-PatchSet: 2 Gerrit-Owner: Quanlong Huang Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Mon, 11 Dec 2023 05:42:21 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12398: Fix Ranger role not exists when altering db/table/view owner to a role
Fang-Yu Rao has posted comments on this change. (
http://gerrit.cloudera.org:8080/20508 )
Change subject: IMPALA-12398: Fix Ranger role not exists when altering
db/table/view owner to a role
..
Patch Set 7:
(1 comment)
Thanks for working on the patch Ji!
I do not have any additional suggestion. We could bump the code-review to +2 if
Quanlong does not have any comment either.
http://gerrit.cloudera.org:8080/#/c/20508/7/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java
File
fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java:
http://gerrit.cloudera.org:8080/#/c/20508/7/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java@2548
PS7, Line 2548: ALTER VIEW SET OWNER ROLE
> Could we also add a test case for the ALTER DATABASE SET OWNER ROLE stateme
According to an offline discussion with Ji, a test case for the ALTER DATABASE
SET OWNER ROLE statement is already there at
https://github.com/apache/impala/blob/master/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java#L2613-L2619.
Thus there is no need to add another test case.
try {
parseAndAnalyze("alter database functional set owner role foo",
authzCtx_, frontend_);
} catch (AnalysisException e) {
exceptionThrown = true;
assertEquals("Role 'foo' does not exist.", e.getLocalizedMessage());
}
--
To view, visit http://gerrit.cloudera.org:8080/20508
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b029bdb90111dbd0eab5189360cc81090225cda
Gerrit-Change-Number: 20508
Gerrit-PatchSet: 7
Gerrit-Owner: ji chen
Gerrit-Reviewer: Fang-Yu Rao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Quanlong Huang
Gerrit-Reviewer: ji chen
Gerrit-Comment-Date: Mon, 11 Dec 2023 01:05:15 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11501: Add flag to allow catalog-cache operations on masked tables
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20742 ) Change subject: IMPALA-11501: Add flag to allow catalog-cache operations on masked tables .. Patch Set 2: (3 comments) > Patch Set 2: > > (11 comments) Thanks a lot for the detailed explanation Quanlong! I left some very minor comments. http://gerrit.cloudera.org:8080/#/c/20742/1/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/20742/1/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java@222 PS1, Line 222: (request.getPrivilege() != Privilege.REFRESH : || !BackendConfig.INSTANCE.allowCatalogCacheOpFromMaskedUsers()) This is just for my own understanding. Is it true that by adding this additional condition we could improve the performance of INVALIDATE METADATA when '--allow_catalog_cache_op_from_masked_users' is true (in both catalog modes), because the following call to db.getTable(authorizableTable.getTableName()) at https://gerrit.cloudera.org/c/20742/1/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java#231 is a heavy operation and will be avoided in this case? In addition, according to our discussion at https://gerrit.cloudera.org/c/20742/1/tests/authorization/test_ranger.py#1610, since Impala REFRESH statement in the local catalog mode currently still triggers the loading of table metadata in other place(s), this additional condition could only improve the performance of REFRESH in the legacy catalog mode? http://gerrit.cloudera.org:8080/#/c/20742/1/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/20742/1/tests/authorization/test_ranger.py@1608 PS1, Line 1608: self.execute_query_expect_success( : non_admin_client, : "refresh functional.alltypestiny partition(year=2009, month=1)", user=user) > Nice findings! Thanks a lot for the detailed explanation Quanlong! Things are much clearer after your explanation. :-) Now I understand that CatalogMetaProvider#updateCatalogCache() will be called to invalidate the table metadata after a DDL according to https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java#L1274-L1287. http://gerrit.cloudera.org:8080/#/c/20742/2/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/20742/2/tests/authorization/test_ranger.py@1615 PS2, Line 1615: all Would the REFRESH privilege be sufficient in this case as mentioned in the commit message? -- To view, visit http://gerrit.cloudera.org:8080/20742 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I45935654cbf05a55d740f1b04781022c271f7678 Gerrit-Change-Number: 20742 Gerrit-PatchSet: 2 Gerrit-Owner: Quanlong Huang Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Thu, 07 Dec 2023 01:31:06 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3268: Add support for SHOW VIEWS statement
Fang-Yu Rao has posted comments on this change. (
http://gerrit.cloudera.org:8080/20669 )
Change subject: IMPALA-3268: Add support for SHOW VIEWS statement
..
Patch Set 11:
(1 comment)
Hi all, I have slightly revised the patch set 10 according to the comment from
Sai. Thanks!
http://gerrit.cloudera.org:8080/#/c/20669/10/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
File fe/src/test/java/org/apache/impala/analysis/ParserTest.java:
http://gerrit.cloudera.org:8080/#/c/20669/10/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@1981
PS10, Line 1981: ParsesOk("SHOW VIEWS 'tablename|othername'");
> Can you also add some queries for show views like '' and show views in
Done
--
To view, visit http://gerrit.cloudera.org:8080/20669
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I321fc5350392a815949a4e7d2a64d60466689788
Gerrit-Change-Number: 20669
Gerrit-PatchSet: 11
Gerrit-Owner: Fang-Yu Rao
Gerrit-Reviewer: Aman Sinha
Gerrit-Reviewer: Fang-Yu Rao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Smith
Gerrit-Reviewer: Quanlong Huang
Gerrit-Reviewer: Sai Hemanth Gantasala
Gerrit-Comment-Date: Wed, 06 Dec 2023 00:52:34 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3268: Add support for SHOW VIEWS statement
Hello Quanlong Huang, Aman Sinha, Sai Hemanth Gantasala, Michael Smith, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20669 to look at the new patch set (#11). Change subject: IMPALA-3268: Add support for SHOW VIEWS statement .. IMPALA-3268: Add support for SHOW VIEWS statement This patch adds the support for the SHOW VIEWS statement and does not change the current behavior of the SHOW TABLES statement. It is recommended that we start the catalog server with '--pull_table_types_and_comments=true' when executing this command so that the table type of every Table is always available. Specifically, after this patch, a user will be able to retrieve from Impala the names of the Table's whose getTableType() evaluate to TImpalaTableType.VIEW that match an optional pattern. Similar to the SHOW TABLES statement, the following are supported after this patch. 1. SHOW VIEWS. 2. SHOW VIEWS "||", where denotes the N-th pattern, "|" denotes choice and each pattern may contain wildcards denoted by "*". 3. SHOW VIEWS IN , where denotes the name of a database. Refer to the added end-to-end test cases for further details. Recall that Hive distinguishes materialized views from views. To align Impala's behavior with that of Hive with respect to the SHOW VIEWS statement, in this patch we introduce TImpalaTableType.MATERIALIZED_VIEW as a table type that corresponds to the table type of 'MATERIALIZED_VIEW' for a org.apache.hadoop.hive.metastore.api.Table. Due to this we have to revise the mappings from Hive's table types to Impala's table types in various places and have to slightly modify the logic used to instantiate a table with an appropriate type. Testing: - Added various frontend and end-to-end tests to verify the behavior of the SHOW VIEWS statement. Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 --- M be/src/service/client-request-state.cc M be/src/service/frontend.cc M be/src/service/frontend.h M common/thrift/CatalogObjects.thrift M common/thrift/CatalogService.thrift M common/thrift/Frontend.thrift M fe/src/main/cup/sql-parser.cup M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java A fe/src/main/java/org/apache/impala/analysis/ShowTablesOrViewsStmt.java M fe/src/main/java/org/apache/impala/analysis/ShowTablesStmt.java A fe/src/main/java/org/apache/impala/analysis/ShowViewsStmt.java M fe/src/main/java/org/apache/impala/catalog/Catalog.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/catalog/FeCatalog.java M fe/src/main/java/org/apache/impala/catalog/FeDb.java M fe/src/main/java/org/apache/impala/catalog/Hive3MetastoreShimBase.java M fe/src/main/java/org/apache/impala/catalog/IncompleteTable.java M fe/src/main/java/org/apache/impala/catalog/Table.java M fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java M fe/src/main/jflex/sql-scanner.flex M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java A testdata/workloads/functional-query/queries/QueryTest/show_views.test M tests/custom_cluster/test_preload_table_types.py A tests/custom_cluster/test_show_views_statements.py 29 files changed, 616 insertions(+), 118 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/20669/11 -- To view, visit http://gerrit.cloudera.org:8080/20669 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 Gerrit-Change-Number: 20669 Gerrit-PatchSet: 11 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Sai Hemanth Gantasala
[Impala-ASF-CR] IMPALA-11501: Add flag to allow catalog-cache operations on masked tables
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20742 ) Change subject: IMPALA-11501: Add flag to allow catalog-cache operations on masked tables .. Patch Set 1: (5 comments) Thanks for the help Quanlong! In this round of review I tried to familiarize myself with the behavior of Impala after the flag is introduced based on the test cases added in test_ranger.py and found some differences between 2 catalog modes. I will continue the review and let you know if there are additional questions or comments on other parts of the patch. http://gerrit.cloudera.org:8080/#/c/20742/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20742/1//COMMIT_MSG@40 PS1, Line 40: doens't nit: doesn't http://gerrit.cloudera.org:8080/#/c/20742/1//COMMIT_MSG@41 PS1, Line 41: can't nit: not able to http://gerrit.cloudera.org:8080/#/c/20742/1//COMMIT_MSG@44 PS1, Line 44: lookup nit: look up http://gerrit.cloudera.org:8080/#/c/20742/1/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/20742/1/tests/authorization/test_ranger.py@1571 PS1, Line 1571: non_owner_client Should this be renamed to 'non_admin_client' or 'owner_client'? It seems in this test we use this client to submit queries as the user getuser(), which is the owner of the table involved in the test and is a non-administrative user. http://gerrit.cloudera.org:8080/#/c/20742/1/tests/authorization/test_ranger.py@1608 PS1, Line 1608: self.execute_query_expect_success( : non_owner_client, : "refresh functional.alltypestiny partition(year=2009, month=1)", user=user) I observed the following 2 differences between the legacy and the local catalog modes when '--allow_catalog_cache_op_from_masked_users=true' is added. Don't know if I missed something. Are those differences expected? The 1st difference: with respect to the REFRESH on a table. Specifically, in the legacy catalog mode, I found this REFRESH on partitions is already allowed even without 'user' executing "describe functional.alltypestiny", whereas REFRESH on the same table is denied. In addition, as shown in the following, in the legacy catalog mode, after a REFRESH on partitions is executed, the same "refresh functional.alltypestiny" will be allowed, which I think/guess is due to the table metadata being loaded (just like what happens after "describe functional.alltypestiny"). [localhost:21050] default> refresh functional.alltypestiny; Query: refresh functional.alltypestiny Query submitted at: 2023-12-03 11:38:56 (Coordinator: http://fangyu-upstream-dev.gce.cloudera.com:25000) ERROR: AuthorizationException: User 'fangyurao' does not have privileges to execute 'INVALIDATE METADATA/REFRESH' on: functional.alltypestiny [localhost:21050] default> refresh functional.alltypestiny partition(year=2009, month=1); Query: refresh functional.alltypestiny partition(year=2009, month=1) Query submitted at: 2023-12-03 11:39:04 (Coordinator: http://fangyu-upstream-dev.gce.cloudera.com:25000) Query state can be monitored at: http://fangyu-upstream-dev.gce.cloudera.com:25000/query_plan?query_id=ba4e95cf064405e3:22bc528e [localhost:21050] default> refresh functional.alltypestiny; Query: refresh functional.alltypestiny Query submitted at: 2023-12-03 11:39:10 (Coordinator: http://fangyu-upstream-dev.gce.cloudera.com:25000) Query state can be monitored at: http://fangyu-upstream-dev.gce.cloudera.com:25000/query_plan?query_id=7041cfffdf9bafac:0d1b5476 On the other hand, in the local catalog mode, both REFRESH on table and REFRESH on partitions are allowed. The 2nd difference: with respect to INVALIDATE METADATA on a table following a REFRESH on the same table. In the legacy catalog mode, a subsequent INVALIDATE METADATA on the same table is allowed once a REFRESH on partitions of this table is executed. However, in the local catalog mode, even we execute both a REFRESH on the table and a REFRESH on partitions of the same table as 'user' successfully, an INVALIDATE METADATA on the same table is not allowed. In this case 'user' has to execute DESCRIBE on the table to execute INVALIDATE METADATA on the table successfully. -- To view, visit http://gerrit.cloudera.org:8080/20742 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I45935654cbf05a55d740f1b04781022c271f7678 Gerrit-Change-Number: 20742 Gerrit-PatchSet: 1 Gerrit-Owner: Quanlong Huang Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Sun, 03 Dec 2023 21:23:09 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3268: Add support for SHOW VIEWS statement
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20669 ) Change subject: IMPALA-3268: Add support for SHOW VIEWS statement .. Patch Set 10: (2 comments) Thanks for the suggestions Quanlong! I slightly revised the commit message and removed the (flaky) tests against the flag of '--load_catalog_in_background=true'. Let me know if there are other suggestions. Thanks very much! http://gerrit.cloudera.org:8080/#/c/20669/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20669/9//COMMIT_MSG@16 PS9, Line 16: Impala the names of the Table's whose getTableType() evaluate to > I think if pull_table_types_and_comments is set to false, SHOW VIEWS can st Thanks for the detailed explanation Quanlong! I will revise the commit message accordingly in the next patch. http://gerrit.cloudera.org:8080/#/c/20669/9/tests/custom_cluster/test_show_views_statements.py File tests/custom_cluster/test_show_views_statements.py: http://gerrit.cloudera.org:8080/#/c/20669/9/tests/custom_cluster/test_show_views_statements.py@27 PS9, Line 27: > I don't think we should test on this flag. The metadata is loaded in backgr Thanks Quanlong! I will remove the tests against this flag in the next patch. -- To view, visit http://gerrit.cloudera.org:8080/20669 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 Gerrit-Change-Number: 20669 Gerrit-PatchSet: 10 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Sai Hemanth Gantasala Gerrit-Comment-Date: Thu, 30 Nov 2023 19:48:15 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3268: Add support for SHOW VIEWS statement
Hello Quanlong Huang, Aman Sinha, Sai Hemanth Gantasala, Michael Smith, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20669 to look at the new patch set (#10). Change subject: IMPALA-3268: Add support for SHOW VIEWS statement .. IMPALA-3268: Add support for SHOW VIEWS statement This patch adds the support for the SHOW VIEWS statement and does not change the current behavior of the SHOW TABLES statement. It is recommended that we start the catalog server with '--pull_table_types_and_comments=true' when executing this command so that the table type of every Table is always available. Specifically, after this patch, a user will be able to retrieve from Impala the names of the Table's whose getTableType() evaluate to TImpalaTableType.VIEW that match an optional pattern. Similar to the SHOW TABLES statement, the following are supported after this patch. 1. SHOW VIEWS. 2. SHOW VIEWS "||", where denotes the N-th pattern, "|" denotes choice and each pattern may contain wildcards denoted by "*". 3. SHOW VIEWS IN , where denotes the name of a database. Refer to the added end-to-end test cases for further details. Recall that Hive distinguishes materialized views from views. To align Impala's behavior with that of Hive with respect to the SHOW VIEWS statement, in this patch we introduce TImpalaTableType.MATERIALIZED_VIEW as a table type that corresponds to the table type of 'MATERIALIZED_VIEW' for a org.apache.hadoop.hive.metastore.api.Table. Due to this we have to revise the mappings from Hive's table types to Impala's table types in various places and have to slightly modify the logic used to instantiate a table with an appropriate type. Testing: - Added various frontend and end-to-end tests to verify the behavior of the SHOW VIEWS statement. Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 --- M be/src/service/client-request-state.cc M be/src/service/frontend.cc M be/src/service/frontend.h M common/thrift/CatalogObjects.thrift M common/thrift/CatalogService.thrift M common/thrift/Frontend.thrift M fe/src/main/cup/sql-parser.cup M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java A fe/src/main/java/org/apache/impala/analysis/ShowTablesOrViewsStmt.java M fe/src/main/java/org/apache/impala/analysis/ShowTablesStmt.java A fe/src/main/java/org/apache/impala/analysis/ShowViewsStmt.java M fe/src/main/java/org/apache/impala/catalog/Catalog.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/catalog/FeCatalog.java M fe/src/main/java/org/apache/impala/catalog/FeDb.java M fe/src/main/java/org/apache/impala/catalog/Hive3MetastoreShimBase.java M fe/src/main/java/org/apache/impala/catalog/IncompleteTable.java M fe/src/main/java/org/apache/impala/catalog/Table.java M fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java M fe/src/main/jflex/sql-scanner.flex M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java A testdata/workloads/functional-query/queries/QueryTest/show_views.test M tests/custom_cluster/test_preload_table_types.py A tests/custom_cluster/test_show_views_statements.py 29 files changed, 612 insertions(+), 118 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/20669/10 -- To view, visit http://gerrit.cloudera.org:8080/20669 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 Gerrit-Change-Number: 20669 Gerrit-PatchSet: 10 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Sai Hemanth Gantasala
[Impala-ASF-CR] IMPALA-3268: Add support for SHOW VIEWS statement
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20669 ) Change subject: IMPALA-3268: Add support for SHOW VIEWS statement .. Patch Set 9: (1 comment) I slightly revised the commit message of patch set 8 to address Michael's comment regarding the differences between the 2 startup flags of the catalog server. Let me know if there are additional suggestions. Thanks! http://gerrit.cloudera.org:8080/#/c/20669/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20669/6//COMMIT_MSG@14 PS6, Line 14: always load the table type of each Tab > I'm not really clear on the difference between these two options. When woul Thanks Michael! I am not familiar with the differences between these 2 flags. But by attaching a debugger to the Impala coordinator when executing the command "show views in functional", I found that "--pull_table_types_and_comments=true" is a more lightweight flag. Specifically, if the catalog server is started with "--pull_table_types_and_comments=true", then before a Table, e.g., 'functional.materialized_view', is referenced by a query, this Table will be an instance of IncompleteTable, where a lot of table metadata is not loaded, whereas with "--load_catalog_in_background=true", the same Table will be an instance of MaterializedViewHdfsTable with fully loaded table metadata from Hive MetaStore. As to where to set a break point, one possible place is https://gerrit.cloudera.org/c/20669/8/fe/src/main/java/org/apache/impala/catalog/Db.java#217. Recall that 'tableCache_.metadataCache_' is a Map from table names to Table's. We will be able to inspect the instance of a Table at runtime there. On the other hand, I also found the documentation of the flag '--load_catalog_in_background=true' at https://impala.apache.org/docs/build/html/topics/impala_config_options.html#:~:text=If%20set%20to%20true,service%20and%20Impala%20Daemon. although we do not elaborate on the differences between these 2 flags there. -- To view, visit http://gerrit.cloudera.org:8080/20669 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 Gerrit-Change-Number: 20669 Gerrit-PatchSet: 9 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Sai Hemanth Gantasala Gerrit-Comment-Date: Thu, 30 Nov 2023 00:06:55 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3268: Add support for SHOW VIEWS statement
Hello Quanlong Huang, Aman Sinha, Sai Hemanth Gantasala, Michael Smith, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20669 to look at the new patch set (#9). Change subject: IMPALA-3268: Add support for SHOW VIEWS statement .. IMPALA-3268: Add support for SHOW VIEWS statement This patch adds the support for the SHOW VIEWS statement and does not change the current behavior of the SHOW TABLES statement. In order for this newly supported statement to work, we have to start the catalog server with either '--pull_table_types_and_comments=true' or '--load_catalog_in_background=true', both of which will make Impala always load the table type of each Table from Hive MetaStore. The former flag is a more lightweight flag in that the table metadata of a Table is not fully loaded until it is referenced in a query. Specifically, after this patch, a user will be able to retrieve from Impala the names of the Table's whose getTableType() evaluate to TImpalaTableType.VIEW that match an optional pattern. Similar to the SHOW TABLES statement, the following are supported after this patch. 1. SHOW VIEWS. 2. SHOW VIEWS "||", where denotes the N-th pattern, "|" denotes choice and each pattern may contain wildcards denoted by "*". 3. SHOW VIEWS IN , where denotes the name of a database. Refer to the added end-to-end test cases for further details. Recall that Hive distinguishes materialized views from views. To align Impala's behavior with that of Hive with respect to the SHOW VIEWS statement, in this patch we introduce TImpalaTableType.MATERIALIZED_VIEW as a table type that corresponds to the table type of 'MATERIALIZED_VIEW' for a org.apache.hadoop.hive.metastore.api.Table. Due to this we have to revise the mappings from Hive's table types to Impala's table types in various places and have to slightly modify the logic used to instantiate a table with an appropriate type. Testing: - Added various frontend and end-to-end tests to verify the behavior of the SHOW VIEWS statement. Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 --- M be/src/service/client-request-state.cc M be/src/service/frontend.cc M be/src/service/frontend.h M common/thrift/CatalogObjects.thrift M common/thrift/CatalogService.thrift M common/thrift/Frontend.thrift M fe/src/main/cup/sql-parser.cup M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java A fe/src/main/java/org/apache/impala/analysis/ShowTablesOrViewsStmt.java M fe/src/main/java/org/apache/impala/analysis/ShowTablesStmt.java A fe/src/main/java/org/apache/impala/analysis/ShowViewsStmt.java M fe/src/main/java/org/apache/impala/catalog/Catalog.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/catalog/FeCatalog.java M fe/src/main/java/org/apache/impala/catalog/FeDb.java M fe/src/main/java/org/apache/impala/catalog/Hive3MetastoreShimBase.java M fe/src/main/java/org/apache/impala/catalog/IncompleteTable.java M fe/src/main/java/org/apache/impala/catalog/Table.java M fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java M fe/src/main/jflex/sql-scanner.flex M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java A testdata/workloads/functional-query/queries/QueryTest/show_views.test M tests/custom_cluster/test_preload_table_types.py A tests/custom_cluster/test_show_views_statements.py 29 files changed, 624 insertions(+), 118 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/20669/9 -- To view, visit http://gerrit.cloudera.org:8080/20669 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 Gerrit-Change-Number: 20669 Gerrit-PatchSet: 9 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Sai Hemanth Gantasala
[Impala-ASF-CR] IMPALA-3268: Add support for SHOW VIEWS statement
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20669 ) Change subject: IMPALA-3268: Add support for SHOW VIEWS statement .. Patch Set 8: (4 comments) Hi all, in the patch set 8 I have addressed most of Michael's and Quanlong's comments. Let me know if there are additional suggestions. Thanks very much for the help! http://gerrit.cloudera.org:8080/#/c/20669/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20669/6//COMMIT_MSG@14 PS6, Line 14: always load the table type of each Tab > I think what matters is whether the table type of the views are loaded. The Thanks Quanlong! Let me revise the commit message and add 2 end-to-end tests to exercise the code path related to the flag '--load_catalog_in_background=true' in the next patch. http://gerrit.cloudera.org:8080/#/c/20669/5/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/20669/5/be/src/service/client-request-state.cc@366 PS5, Line 366: const std::set& table_types = params->table_types; > nit: I'd probably do Thanks Michael! I will change the type of 'table_types' of the function GetTableNames() to a reference (instead of a pointer) to set based on your suggestion at https://gerrit.cloudera.org/c/20669/7/be/src/service/frontend.cc#210 and will change https://gerrit.cloudera.org/c/20669/5/be/src/service/client-request-state.cc#368 in the following accordingly. http://gerrit.cloudera.org:8080/#/c/20669/7/be/src/service/frontend.h File be/src/service/frontend.h: http://gerrit.cloudera.org:8080/#/c/20669/7/be/src/service/frontend.h@65 PS7, Line 65: Status GetTableNames(const std::string& db, const std::string* pattern, > I'm not really sure why this was split out as a separate function signature Thanks Michael! I split this function out also because this function is used at https://github.com/apache/impala/blob/master/be/src/service/impala-http-handler.cc#L930-L931. Do we want to make the change to the call site so that the call site will provide an empty set as 'table_types' when calling GetTableNames()? http://gerrit.cloudera.org:8080/#/c/20669/7/be/src/service/frontend.cc File be/src/service/frontend.cc: http://gerrit.cloudera.org:8080/#/c/20669/7/be/src/service/frontend.cc@210 PS7, Line 210: params.__set_table_types(table_types); > If table_types is null, this crashes. We should take table_types as a ref r Thanks Michael! You are correct. It's dangerous to not perform the null pointer check before dereferencing a pointer. I will change the type of the argument 'table_types' to a reference in the next patch. -- To view, visit http://gerrit.cloudera.org:8080/20669 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 Gerrit-Change-Number: 20669 Gerrit-PatchSet: 8 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Sai Hemanth Gantasala Gerrit-Comment-Date: Tue, 28 Nov 2023 23:15:19 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3268: Add support for SHOW VIEWS statement
Hello Quanlong Huang, Aman Sinha, Sai Hemanth Gantasala, Michael Smith, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20669 to look at the new patch set (#8). Change subject: IMPALA-3268: Add support for SHOW VIEWS statement .. IMPALA-3268: Add support for SHOW VIEWS statement This patch adds the support for the SHOW VIEWS statement and does not change the current behavior of the SHOW TABLES statement. In order for this newly supported statement to work, we have to start the catalog server with either '--pull_table_types_and_comments=true' or '--load_catalog_in_background=true', both of which will make Impala always load the table type of each Table from Hive MetaStore. Specifically, after this patch, a user will be able to retrieve from Impala the names of the Table's whose getTableType() evaluate to TImpalaTableType.VIEW that match an optional pattern. Similar to the SHOW TABLES statement, the following are supported after this patch. 1. SHOW VIEWS. 2. SHOW VIEWS "||", where denotes the N-th pattern, "|" denotes choice and each pattern may contain wildcards denoted by "*". 3. SHOW VIEWS IN , where denotes the name of a database. Refer to the added end-to-end test cases for further details. Recall that Hive distinguishes materialized views from views. To align Impala's behavior with that of Hive with respect to the SHOW VIEWS statement, in this patch we introduce TImpalaTableType.MATERIALIZED_VIEW as a table type that corresponds to the table type of 'MATERIALIZED_VIEW' for a org.apache.hadoop.hive.metastore.api.Table. Due to this we have to revise the mappings from Hive's table types to Impala's table types in various places and have to slightly modify the logic used to instantiate a table with an appropriate type. Testing: - Added various frontend and end-to-end tests to verify the behavior of the SHOW VIEWS statement. Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 --- M be/src/service/client-request-state.cc M be/src/service/frontend.cc M be/src/service/frontend.h M common/thrift/CatalogObjects.thrift M common/thrift/CatalogService.thrift M common/thrift/Frontend.thrift M fe/src/main/cup/sql-parser.cup M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java A fe/src/main/java/org/apache/impala/analysis/ShowTablesOrViewsStmt.java M fe/src/main/java/org/apache/impala/analysis/ShowTablesStmt.java A fe/src/main/java/org/apache/impala/analysis/ShowViewsStmt.java M fe/src/main/java/org/apache/impala/catalog/Catalog.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/catalog/FeCatalog.java M fe/src/main/java/org/apache/impala/catalog/FeDb.java M fe/src/main/java/org/apache/impala/catalog/Hive3MetastoreShimBase.java M fe/src/main/java/org/apache/impala/catalog/IncompleteTable.java M fe/src/main/java/org/apache/impala/catalog/Table.java M fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java M fe/src/main/jflex/sql-scanner.flex M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java A testdata/workloads/functional-query/queries/QueryTest/show_views.test M tests/custom_cluster/test_preload_table_types.py A tests/custom_cluster/test_show_views_statements.py 29 files changed, 624 insertions(+), 118 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/20669/8 -- To view, visit http://gerrit.cloudera.org:8080/20669 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 Gerrit-Change-Number: 20669 Gerrit-PatchSet: 8 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Sai Hemanth Gantasala
[Impala-ASF-CR] IMPALA-3268: Add support for SHOW VIEWS statement
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20669 ) Change subject: IMPALA-3268: Add support for SHOW VIEWS statement .. Patch Set 7: (10 comments) Hi all, I have revised the patch set 6 according to the comments from Quanlong. Let me know if there are additional suggestions. Thanks very much for the help! http://gerrit.cloudera.org:8080/#/c/20669/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20669/6//COMMIT_MSG@13 PS6, Line 13: y. > nit: "catalog server" Done http://gerrit.cloudera.org:8080/#/c/20669/6//COMMIT_MSG@14 PS6, Line 14: > Is this feature depends on this flag? I think if the views are loaded, they Thanks Quanlong! Yes. This feature depends on this flag. I will emphasize this in the commit message of the next patch. Specifically, if we start the catalog server without this flag, then Impala could not determine whether or not a given Table is a view without a user explicitly executing statements like REFRESH. In addition, the table metadata would be lost after the INVALIDATE METADATA statement. This could be shown in the following sequence of queries. On the other hand, if we start the catalog server with this flag, SHOW VIEWS always returns those tables whose getTableType() evaluate to TImpalaTableType.VIEW. [localhost:21050] default> show views in functional; Query: show views in functional Fetched 0 row(s) in 0.04s [localhost:21050] default> refresh functional.alltypes_view; Query: refresh functional.alltypes_view Query submitted at: 2023-11-27 11:41:34 (Coordinator: http://fangyu-upstream-dev.gce.cloudera.com:25000) Query state can be monitored at: http://fangyu-upstream-dev.gce.cloudera.com:25000/query_plan?query_id=4d4c7564e5a13fef:f3d8c131 [localhost:21050] default> show views in functional; Query: show views in functional +---+ | name | +---+ | alltypes_view | +---+ Fetched 1 row(s) in 0.00s [localhost:21050] default> invalidate metadata functional.alltypes_view; Query: invalidate metadata functional.alltypes_view Query submitted at: 2023-11-27 11:42:07 (Coordinator: http://fangyu-upstream-dev.gce.cloudera.com:25000) Query state can be monitored at: http://fangyu-upstream-dev.gce.cloudera.com:25000/query_plan?query_id=6b4c9396e5147b00:eeeba566 [localhost:21050] default> show views in functional; Query: show views in functional Fetched 0 row(s) in 0.00s http://gerrit.cloudera.org:8080/#/c/20669/6/be/src/service/frontend.cc File be/src/service/frontend.cc: http://gerrit.cloudera.org:8080/#/c/20669/6/be/src/service/frontend.cc@199 PS6, Line 199: strin > nit: we don't need explicitly using the "std" namespace for std::string and Thanks Quanlong! I will remove the namespace in the next patch. http://gerrit.cloudera.org:8080/#/c/20669/6/fe/src/main/java/org/apache/impala/analysis/ShowViewsStmt.java File fe/src/main/java/org/apache/impala/analysis/ShowViewsStmt.java: http://gerrit.cloudera.org:8080/#/c/20669/6/fe/src/main/java/org/apache/impala/analysis/ShowViewsStmt.java@54 PS6, Line 54: > nit: we can use com.google.common.collect.Sets directly as Done http://gerrit.cloudera.org:8080/#/c/20669/6/fe/src/main/java/org/apache/impala/catalog/Catalog.java File fe/src/main/java/org/apache/impala/catalog/Catalog.java: http://gerrit.cloudera.org:8080/#/c/20669/6/fe/src/main/java/org/apache/impala/catalog/Catalog.java@241 PS6, Line 241: Collections.emp > nit: use Collections.emptySet() to avoid creating new objects. Done http://gerrit.cloudera.org:8080/#/c/20669/6/fe/src/main/java/org/apache/impala/catalog/Db.java File fe/src/main/java/org/apache/impala/catalog/Db.java: http://gerrit.cloudera.org:8080/#/c/20669/6/fe/src/main/java/org/apache/impala/catalog/Db.java@208 PS6, Line 208: > nit: use Collections.emptySet() to avoid creating new objects. Done http://gerrit.cloudera.org:8080/#/c/20669/6/fe/src/main/java/org/apache/impala/catalog/Db.java@220 PS6, Line 220: lect(Collectors.toList()); > nit: This doesn't leverage the fact that 'tableTypes' is a set. I think it Thanks Quanlong! I will revise this in the next patch. http://gerrit.cloudera.org:8080/#/c/20669/6/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java File fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java: http://gerrit.cloudera.org:8080/#/c/20669/6/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java@124 PS6, Line 124: Collections.emp > nit: use Collections.emptySet() to avoid creating new objects. Done http://gerrit.cloudera.org:8080/#/c/20669/6/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java File fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java: http://gerrit.cloudera.org:8080/#/c/20669/6/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@162 PS6, Line 162: > nit: use
[Impala-ASF-CR] IMPALA-3268: Add support for SHOW VIEWS statement
Hello Quanlong Huang, Aman Sinha, Sai Hemanth Gantasala, Michael Smith, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20669 to look at the new patch set (#7). Change subject: IMPALA-3268: Add support for SHOW VIEWS statement .. IMPALA-3268: Add support for SHOW VIEWS statement This patch adds the support for the SHOW VIEWS statement and does not change the current behavior of the SHOW TABLES statement. Moreover, the catalog server has to be started with '--pull_table_types_and_comments=true' in order for this newly supported statement to work properly. Specifically, after this patch, a user will be able to retrieve from Impala the names of Table's whose getTableType() evaluate to TImpalaTableType.VIEW that match an optional pattern when the catalog server is started with '--pull_table_types_and_comments=true' which allows the catalog server to additionally load the table type of each Table from Hive MetaStore during startup. Similar to the SHOW TABLES statement, the following are supported after this patch. 1. SHOW VIEWS. 2. SHOW VIEWS "||", where denotes the N-th pattern, "|" denotes choice and each pattern may contain wildcards denoted by "*". 3. SHOW VIEWS IN , where denotes the name of a database. Refer to the added end-to-end test cases for further details. Recall that Hive distinguishes materialized views from views. To align Impala's behavior with that of Hive with respect to the SHOW VIEWS statement, in this patch we introduce TImpalaTableType.MATERIALIZED_VIEW as a table type that corresponds to the table type of 'MATERIALIZED_VIEW' for a org.apache.hadoop.hive.metastore.api.Table. Due to this we have to revise the mappings from Hive's table types to Impala's table types in various places and have to slightly modify the logic used to instantiate a table with an appropriate type. Testing: - Added various frontend and end-to-end tests to verify the behavior of the SHOW VIEWS statement. Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 --- M be/src/service/client-request-state.cc M be/src/service/frontend.cc M be/src/service/frontend.h M common/thrift/CatalogObjects.thrift M common/thrift/CatalogService.thrift M common/thrift/Frontend.thrift M fe/src/main/cup/sql-parser.cup M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java A fe/src/main/java/org/apache/impala/analysis/ShowTablesOrViewsStmt.java M fe/src/main/java/org/apache/impala/analysis/ShowTablesStmt.java A fe/src/main/java/org/apache/impala/analysis/ShowViewsStmt.java M fe/src/main/java/org/apache/impala/catalog/Catalog.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/catalog/FeCatalog.java M fe/src/main/java/org/apache/impala/catalog/FeDb.java M fe/src/main/java/org/apache/impala/catalog/Hive3MetastoreShimBase.java M fe/src/main/java/org/apache/impala/catalog/IncompleteTable.java M fe/src/main/java/org/apache/impala/catalog/Table.java M fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java M fe/src/main/jflex/sql-scanner.flex M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java A testdata/workloads/functional-query/queries/QueryTest/show_views.test M tests/custom_cluster/test_preload_table_types.py A tests/custom_cluster/test_show_views_statements.py 29 files changed, 612 insertions(+), 118 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/20669/7 -- To view, visit http://gerrit.cloudera.org:8080/20669 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 Gerrit-Change-Number: 20669 Gerrit-PatchSet: 7 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Sai Hemanth Gantasala
[Impala-ASF-CR] IMPALA-3268: Add support for SHOW VIEWS statement
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20669 ) Change subject: IMPALA-3268: Add support for SHOW VIEWS statement .. Patch Set 6: (3 comments) Hi all, I slightly revised the patch set 5 to address some of Michael's comments. Let me know if there are still additional suggestions. Thanks! http://gerrit.cloudera.org:8080/#/c/20669/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20669/5//COMMIT_MSG@7 PS5, Line 7: IMPALA-3268: Add support for SHOW VIEWS statement > The ticket mentions having a way to get both tables and views in a single c Thanks Michael! This patch does not change the current behavior of the SHOW TABLES statement. I will emphasize this in the commit message in the next patch. Specifically, for SHOW VIEWS, we explicitly set up the table types to retrieve at https://gerrit.cloudera.org/c/20669/5/fe/src/main/java/org/apache/impala/analysis/ShowViewsStmt.java#54. When we do not set up the table types to match, this field defaults to an empty set. For an empty set, we return all types of tables (refer to https://gerrit.cloudera.org/c/20669/5/fe/src/main/java/org/apache/impala/catalog/Db.java#224). I moved the following items out of the scope of IMPALA-3268 to a newly created one: IMPALA-12574 because we may need more discussion around whether we need to consider materialized views as tables or views. Currently SHOW TABLES in Cloudera's distribution of Hive returns both tables, views, and materialized views and SHOW VIEWS does not return materialized views (refer to https://gerrit.cloudera.org/c/20669/5/testdata/workloads/functional-query/queries/QueryTest/show_views.test#92). - SHOW TABLES should only return tables. - add a flag to either above commands to return all tables and views. http://gerrit.cloudera.org:8080/#/c/20669/5/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/20669/5/be/src/service/client-request-state.cc@366 PS5, Line 366: const std::set* table_types = &(params->table_types); > Can we take a ref of table_types? I don't see any reason to make a copy her Thanks for catching this Michael! Yes, we can pass the pointer to the set of table types to GetTableNames() instead. I will do this in the next patch. I will also reorder the arguments of GetTableNames() so that the returned 'table_names' will still be the last argument after this patch since we seem to always have the returned result as the last argument for functions in the class of Frontend. http://gerrit.cloudera.org:8080/#/c/20669/5/common/thrift/CatalogService.thrift File common/thrift/CatalogService.thrift: http://gerrit.cloudera.org:8080/#/c/20669/5/common/thrift/CatalogService.thrift@530 PS5, Line 530: UNKNOWN, > UNKNOWN seems like it should come after other types. Do we rely on that any Thanks Michael! I put the new type as the last one because I saw in IMPALA-8234 (https://gerrit.cloudera.org/c/12543/8/common/thrift/Metrics.thrift#49) that we reverted what we did in IMPALA-7694, where we inserted a field in the middle of the Metrics.TUnit enum (https://gerrit.cloudera.org/c/12069/22/common/thrift/Metrics.thrift#32). According to this, maybe it is better we always add the new field as the last one for backward compatibility? -- To view, visit http://gerrit.cloudera.org:8080/20669 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 Gerrit-Change-Number: 20669 Gerrit-PatchSet: 6 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Sai Hemanth Gantasala Gerrit-Comment-Date: Thu, 23 Nov 2023 19:12:20 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3268: Add support for SHOW VIEWS statement
Hello Quanlong Huang, Aman Sinha, Sai Hemanth Gantasala, Michael Smith, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20669 to look at the new patch set (#6). Change subject: IMPALA-3268: Add support for SHOW VIEWS statement .. IMPALA-3268: Add support for SHOW VIEWS statement This patch adds the support for the SHOW VIEWS statement and does not change the current behavior of the SHOW TABLES statement. Specifically, after this patch, a user will be able to retrieve from Impala the names of Table's whose getTableType() evaluate to TImpalaTableType.VIEW that match an optional pattern when the catalog daemon is started with '--pull_table_types_and_comments=true' which allows the catalog daemon to additionally load the table type of each Table from Hive MetaStore during startup. Similar to the SHOW TABLES statement, the following are supported after this patch. 1. SHOW VIEWS. 2. SHOW VIEWS "||", where denotes the N-th pattern, "|" denotes choice and each pattern may contain wildcards denoted by "*". 3. SHOW VIEWS IN , where denotes the name of a database. Refer to the added end-to-end test cases for further details. Recall that Hive distinguishes materialized views from views. To align Impala's behavior with that of Hive with respect to the SHOW VIEWS statement, in this patch we introduce TImpalaTableType.MATERIALIZED_VIEW as a table type that corresponds to the table type of 'MATERIALIZED_VIEW' for a org.apache.hadoop.hive.metastore.api.Table. Due to this we have to revise the mappings from Hive's table types to Impala's table types in various places and have to slightly modify the logic used to instantiate a table with an appropriate type. Testing: - Added various frontend and end-to-end tests to verify the behavior of the SHOW VIEWS statement. Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 --- M be/src/service/client-request-state.cc M be/src/service/frontend.cc M be/src/service/frontend.h M common/thrift/CatalogObjects.thrift M common/thrift/CatalogService.thrift M common/thrift/Frontend.thrift M fe/src/main/cup/sql-parser.cup M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java A fe/src/main/java/org/apache/impala/analysis/ShowTablesOrViewsStmt.java M fe/src/main/java/org/apache/impala/analysis/ShowTablesStmt.java A fe/src/main/java/org/apache/impala/analysis/ShowViewsStmt.java M fe/src/main/java/org/apache/impala/catalog/Catalog.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/catalog/FeCatalog.java M fe/src/main/java/org/apache/impala/catalog/FeDb.java M fe/src/main/java/org/apache/impala/catalog/Hive3MetastoreShimBase.java M fe/src/main/java/org/apache/impala/catalog/IncompleteTable.java M fe/src/main/java/org/apache/impala/catalog/Table.java M fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java M fe/src/main/jflex/sql-scanner.flex M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java A testdata/workloads/functional-query/queries/QueryTest/show_views.test M tests/custom_cluster/test_preload_table_types.py A tests/custom_cluster/test_show_views_statements.py 29 files changed, 619 insertions(+), 119 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/20669/6 -- To view, visit http://gerrit.cloudera.org:8080/20669 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 Gerrit-Change-Number: 20669 Gerrit-PatchSet: 6 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Sai Hemanth Gantasala
[Impala-ASF-CR] IMPALA-3268: Add support for SHOW VIEWS statement
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20669 ) Change subject: IMPALA-3268: Add support for SHOW VIEWS statement .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/20669/4/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/20669/4/fe/src/main/java/org/apache/impala/catalog/Table.java@494 PS4, Line 494: TImpalaTableType tableType = > line too long (92 > 90) This is fixed in patch set 5. -- To view, visit http://gerrit.cloudera.org:8080/20669 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 Gerrit-Change-Number: 20669 Gerrit-PatchSet: 5 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Sai Hemanth Gantasala Gerrit-Comment-Date: Fri, 17 Nov 2023 23:26:59 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3268: Add support for SHOW VIEWS statement
Hello Quanlong Huang, Aman Sinha, Sai Hemanth Gantasala, Michael Smith, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20669 to look at the new patch set (#5). Change subject: IMPALA-3268: Add support for SHOW VIEWS statement .. IMPALA-3268: Add support for SHOW VIEWS statement This patch adds the support for the SHOW VIEWS statement. Specifically, after this patch, a user will be able to retrieve from Impala the names of Table's whose getTableType() evaluate to TImpalaTableType.VIEW that match an optional pattern when the catalog daemon is started with '--pull_table_types_and_comments=true' which allows the catalog daemon to additionally load the table type of each Table from Hive MetaStore during startup. Similar to the SHOW TABLES statement, the following are supported after this patch. 1. SHOW VIEWS. 2. SHOW VIEWS "||", where denotes the N-th pattern, "|" denotes choice and each pattern may contain wildcards denoted by "*". 3. SHOW VIEWS IN , where denotes the name of a database. Refer to the added end-to-end test cases for further details. Recall that Hive distinguishes materialized views from views. To align Impala's behavior with that of Hive with respect to the SHOW VIEWS statement, in this patch we introduce TImpalaTableType.MATERIALIZED_VIEW as a table type that corresponds to the table type of 'MATERIALIZED_VIEW' for a org.apache.hadoop.hive.metastore.api.Table. Due to this we have to revise the mappings from Hive's table types to Impala's table types in various places and have to slightly modify the logic used to instantiate a table with an appropriate type. Testing: - Added various frontend and end-to-end tests to verify the behavior of the SHOW VIEWS statement. Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 --- M be/src/service/client-request-state.cc M be/src/service/frontend.cc M be/src/service/frontend.h M common/thrift/CatalogObjects.thrift M common/thrift/CatalogService.thrift M common/thrift/Frontend.thrift M fe/src/main/cup/sql-parser.cup M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java A fe/src/main/java/org/apache/impala/analysis/ShowTablesOrViewsStmt.java M fe/src/main/java/org/apache/impala/analysis/ShowTablesStmt.java A fe/src/main/java/org/apache/impala/analysis/ShowViewsStmt.java M fe/src/main/java/org/apache/impala/catalog/Catalog.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/catalog/FeCatalog.java M fe/src/main/java/org/apache/impala/catalog/FeDb.java M fe/src/main/java/org/apache/impala/catalog/Hive3MetastoreShimBase.java M fe/src/main/java/org/apache/impala/catalog/IncompleteTable.java M fe/src/main/java/org/apache/impala/catalog/Table.java M fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java M fe/src/main/jflex/sql-scanner.flex M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java A testdata/workloads/functional-query/queries/QueryTest/show_views.test M tests/custom_cluster/test_preload_table_types.py A tests/custom_cluster/test_show_views_statements.py 29 files changed, 608 insertions(+), 119 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/20669/5 -- To view, visit http://gerrit.cloudera.org:8080/20669 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 Gerrit-Change-Number: 20669 Gerrit-PatchSet: 5 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Sai Hemanth Gantasala
[Impala-ASF-CR] IMPALA-3268: Add support for SHOW VIEWS statement
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20669 ) Change subject: IMPALA-3268: Add support for SHOW VIEWS statement .. Patch Set 4: Hi all, I just revised the patch set 3 based on the broken tests found on https://jenkins.impala.io/job/gerrit-verify-dryrun/9931/console. A new test run against the patch set 4 was also triggered at https://jenkins.impala.io/job/gerrit-verify-dryrun-external/2242/console. Hopefully this time those broken tests will be fixed. -- To view, visit http://gerrit.cloudera.org:8080/20669 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 Gerrit-Change-Number: 20669 Gerrit-PatchSet: 4 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Sai Hemanth Gantasala Gerrit-Comment-Date: Fri, 17 Nov 2023 23:24:47 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3268: Add support for SHOW VIEWS statement
Hello Quanlong Huang, Aman Sinha, Sai Hemanth Gantasala, Michael Smith, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20669 to look at the new patch set (#4). Change subject: IMPALA-3268: Add support for SHOW VIEWS statement .. IMPALA-3268: Add support for SHOW VIEWS statement This patch adds the support for the SHOW VIEWS statement. Specifically, after this patch, a user will be able to retrieve from Impala the names of Table's whose getTableType() evaluate to TImpalaTableType.VIEW that match an optional pattern when the catalog daemon is started with '--pull_table_types_and_comments=true' which allows the catalog daemon to additionally load the table type of each Table from Hive MetaStore during startup. Similar to the SHOW TABLES statement, the following are supported after this patch. 1. SHOW VIEWS. 2. SHOW VIEWS "||", where denotes the N-th pattern, "|" denotes choice and each pattern may contain wildcards denoted by "*". 3. SHOW VIEWS IN , where denotes the name of a database. Refer to the added end-to-end test cases for further details. Recall that Hive distinguishes materialized views from views. To align Impala's behavior with that of Hive with respect to the SHOW VIEWS statement, in this patch we introduce TImpalaTableType.MATERIALIZED_VIEW as a table type that corresponds to the table type of 'MATERIALIZED_VIEW' for a org.apache.hadoop.hive.metastore.api.Table. Due to this we have to revise the mappings from Hive's table types to Impala's table types in various places and have to slightly modify the logic used to instantiate a table with an appropriate type. Testing: - Added various frontend and end-to-end tests to verify the behavior of the SHOW VIEWS statement. Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 --- M be/src/service/client-request-state.cc M be/src/service/frontend.cc M be/src/service/frontend.h M common/thrift/CatalogObjects.thrift M common/thrift/CatalogService.thrift M common/thrift/Frontend.thrift M fe/src/main/cup/sql-parser.cup M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java A fe/src/main/java/org/apache/impala/analysis/ShowTablesOrViewsStmt.java M fe/src/main/java/org/apache/impala/analysis/ShowTablesStmt.java A fe/src/main/java/org/apache/impala/analysis/ShowViewsStmt.java M fe/src/main/java/org/apache/impala/catalog/Catalog.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/catalog/FeCatalog.java M fe/src/main/java/org/apache/impala/catalog/FeDb.java M fe/src/main/java/org/apache/impala/catalog/Hive3MetastoreShimBase.java M fe/src/main/java/org/apache/impala/catalog/IncompleteTable.java M fe/src/main/java/org/apache/impala/catalog/Table.java M fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java M fe/src/main/jflex/sql-scanner.flex M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java A testdata/workloads/functional-query/queries/QueryTest/show_views.test M tests/custom_cluster/test_preload_table_types.py A tests/custom_cluster/test_show_views_statements.py 29 files changed, 607 insertions(+), 119 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/20669/4 -- To view, visit http://gerrit.cloudera.org:8080/20669 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 Gerrit-Change-Number: 20669 Gerrit-PatchSet: 4 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Sai Hemanth Gantasala
[Impala-ASF-CR] IMPALA-12398: Fix Ranger role not exists when altering db/table/view owner to a role
Fang-Yu Rao has posted comments on this change. (
http://gerrit.cloudera.org:8080/20508 )
Change subject: IMPALA-12398: Fix Ranger role not exists when altering
db/table/view owner to a role
..
Patch Set 7: Code-Review+1
(3 comments)
Very sorry for my late response and thanks a lot for addressing most of my
comments on the patch set 3! It's great we do not have duplicate code in
CatalogServiceTestCatalogWithRanger.java. :-)
I only have one very minor suggestion (adding a test case for the ALTER
DATABASE SET OWNER ROLE statement) and do not have any other comment.
http://gerrit.cloudera.org:8080/#/c/20508/7/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java
File
fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java:
http://gerrit.cloudera.org:8080/#/c/20508/7/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java@2548
PS7, Line 2548: ALTER VIEW SET OWNER ROLE
Could we also add a test case for the ALTER DATABASE SET OWNER ROLE statement?
I am asking since we also check the existence of a role at
https://gerrit.cloudera.org/c/20508/7/fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java#63.
http://gerrit.cloudera.org:8080/#/c/20508/3/fe/src/test/java/org/apache/impala/authorization/CatalogServiceTestCatalogWithRanger.java
File
fe/src/test/java/org/apache/impala/authorization/CatalogServiceTestCatalogWithRanger.java:
http://gerrit.cloudera.org:8080/#/c/20508/3/fe/src/test/java/org/apache/impala/authorization/CatalogServiceTestCatalogWithRanger.java@96
PS3, Line 96: // still exists in plugin cache
: rangerImpalaPlugin_.refreshPoliciesAndTags();
: authRole = super.removeRole(roleName);
: } catch (Exception ex) {
: ex.printStackTrace();
: }
: return authRole;
: }
: }
:
:
:
> Hi, Fang-Yu Rao, In our test cases, the set grantGroups is always empty, th
Thanks for the explanation Ji!
http://gerrit.cloudera.org:8080/#/c/20508/3/fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java
File fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java:
http://gerrit.cloudera.org:8080/#/c/20508/3/fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java@a75
PS3, Line 75:
> this change can be reverted, I wrongly thought the original line would exce
Thanks for the explanation!
--
To view, visit http://gerrit.cloudera.org:8080/20508
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b029bdb90111dbd0eab5189360cc81090225cda
Gerrit-Change-Number: 20508
Gerrit-PatchSet: 7
Gerrit-Owner: ji chen
Gerrit-Reviewer: Fang-Yu Rao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Quanlong Huang
Gerrit-Reviewer: ji chen
Gerrit-Comment-Date: Fri, 17 Nov 2023 03:35:49 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3268: Add support for SHOW VIEWS statement
Fang-Yu Rao has posted comments on this change. (
http://gerrit.cloudera.org:8080/20669 )
Change subject: IMPALA-3268: Add support for SHOW VIEWS statement
..
Patch Set 3:
(4 comments)
Hi all, I have addressed Quanlong's comments on the patch set 2. Please let me
know if there are additional suggestions.
I also triggered
https://jenkins.impala.io/job/gerrit-verify-dryrun-external/2241/ against this
patch to see if there are broken tests after patch set 3.
http://gerrit.cloudera.org:8080/#/c/20669/2/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/20669/2/be/src/service/client-request-state.cc@370
PS2, Line 370: return Status::OK();
> It seems the only difference between SHOW_VIEWS and the above SHOW_TABLES c
Thanks Quanlong!
Yes we could. I will merge them in the next patch.
http://gerrit.cloudera.org:8080/#/c/20669/2/be/src/service/frontend.h
File be/src/service/frontend.h:
http://gerrit.cloudera.org:8080/#/c/20669/2/be/src/service/frontend.h@79
PS2, Line 79: std::set());
> We can set a default value as 'views_only=false' then don't need to overrid
Thanks for the suggestion Quanlong!
Based on a discussion offline, I will change this argument to a const reference
to a std::set in the next patch set since it would make
it easier to implement statements like SHOW MATERIALIZED VIEWS in a follow-up
JIRA.
http://gerrit.cloudera.org:8080/#/c/20669/2/fe/src/main/java/org/apache/impala/analysis/ShowTablesStmt.java
File fe/src/main/java/org/apache/impala/analysis/ShowTablesStmt.java:
http://gerrit.cloudera.org:8080/#/c/20669/2/fe/src/main/java/org/apache/impala/analysis/ShowTablesStmt.java@43
PS2, Line 43: getPattern(
> getPattern?
Thanks for catching this Quanlong!
I will some tests in ToSqlTest in the next patch set.
http://gerrit.cloudera.org:8080/#/c/20669/2/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/20669/2/fe/src/main/java/org/apache/impala/service/Frontend.java@593
PS2, Line 593: } else if (analysis.isShowDbsStmt()) {
> nit: move to the above line to be right after "} "
Done
--
To view, visit http://gerrit.cloudera.org:8080/20669
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I321fc5350392a815949a4e7d2a64d60466689788
Gerrit-Change-Number: 20669
Gerrit-PatchSet: 3
Gerrit-Owner: Fang-Yu Rao
Gerrit-Reviewer: Aman Sinha
Gerrit-Reviewer: Fang-Yu Rao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Smith
Gerrit-Reviewer: Quanlong Huang
Gerrit-Reviewer: Sai Hemanth Gantasala
Gerrit-Comment-Date: Thu, 16 Nov 2023 01:07:38 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3268: Add support for SHOW VIEWS statement
Hello Quanlong Huang, Aman Sinha, Sai Hemanth Gantasala, Michael Smith, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20669 to look at the new patch set (#3). Change subject: IMPALA-3268: Add support for SHOW VIEWS statement .. IMPALA-3268: Add support for SHOW VIEWS statement This patch adds the support for the SHOW VIEWS statement. Specifically, after this patch, a user will be able to retrieve from Impala the names of Table's whose getTableType() evaluate to TImpalaTableType.VIEW that match an optional pattern when the catalog daemon is started with '--pull_table_types_and_comments=true' which allows the catalog daemon to additionally load the table type of each Table from Hive MetaStore during startup. Similar to the SHOW TABLES statement, the following are supported after this patch. 1. SHOW VIEWS. 2. SHOW VIEWS "||", where denotes the N-th pattern, "|" denotes choice and each pattern may contain wildcards denoted by "*". 3. SHOW VIEWS IN , where denotes the name of a database. Refer to the added end-to-end test cases for further details. Testing: - Added various frontend and end-to-end tests to verify the behavior of the SHOW VIEWS statement. Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 --- M be/src/service/client-request-state.cc M be/src/service/frontend.cc M be/src/service/frontend.h M common/thrift/CatalogObjects.thrift M common/thrift/CatalogService.thrift M common/thrift/Frontend.thrift M fe/src/main/cup/sql-parser.cup M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java A fe/src/main/java/org/apache/impala/analysis/ShowTablesOrViewsStmt.java M fe/src/main/java/org/apache/impala/analysis/ShowTablesStmt.java A fe/src/main/java/org/apache/impala/analysis/ShowViewsStmt.java M fe/src/main/java/org/apache/impala/catalog/Catalog.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/catalog/FeCatalog.java M fe/src/main/java/org/apache/impala/catalog/FeDb.java M fe/src/main/java/org/apache/impala/catalog/Hive3MetastoreShimBase.java M fe/src/main/java/org/apache/impala/catalog/IncompleteTable.java M fe/src/main/java/org/apache/impala/catalog/Table.java M fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java M fe/src/main/jflex/sql-scanner.flex M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java A testdata/workloads/functional-query/queries/QueryTest/show_views.test A tests/custom_cluster/test_show_views_statements.py 28 files changed, 600 insertions(+), 114 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/20669/3 -- To view, visit http://gerrit.cloudera.org:8080/20669 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 Gerrit-Change-Number: 20669 Gerrit-PatchSet: 3 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Sai Hemanth Gantasala
[Impala-ASF-CR] IMPALA-3268: Add support for SHOW VIEWS statement
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20669 ) Change subject: IMPALA-3268: Add support for SHOW VIEWS statement .. Patch Set 2: Hi all, please let me know if you have any comment on the patch set 2. At the moment, I have a question regarding whether we should treat materialized views as tables as HiveServer2 does (as shown at https://gerrit.cloudera.org/c/20669/2/testdata/workloads/functional-query/queries/QueryTest/show_views.test#94) and would like to discuss with you how to proceed. Recall that currently for a materialized view 'v', v.getTableType() evaluates to TImpalaTableType.VIEW if the respective table metadata retrieved from Hive metastore is available. Thank you very much for the help! -- To view, visit http://gerrit.cloudera.org:8080/20669 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 Gerrit-Change-Number: 20669 Gerrit-PatchSet: 2 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Sai Hemanth Gantasala Gerrit-Comment-Date: Thu, 09 Nov 2023 00:48:58 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3268: Add support for SHOW VIEWS statement
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20669 ) Change subject: IMPALA-3268: Add support for SHOW VIEWS statement .. Patch Set 2: > Patch Set 1: > > (1 comment) Thanks Michael! I have added some example in the commit message of patch set 2. -- To view, visit http://gerrit.cloudera.org:8080/20669 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 Gerrit-Change-Number: 20669 Gerrit-PatchSet: 2 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Comment-Date: Thu, 09 Nov 2023 00:39:12 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3268: Add support for SHOW VIEWS statement
Hello Michael Smith, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20669 to look at the new patch set (#2). Change subject: IMPALA-3268: Add support for SHOW VIEWS statement .. IMPALA-3268: Add support for SHOW VIEWS statement This patch adds the support for the SHOW VIEWS statement. Specifically, after this patch, a user will be able to retrieve from Impala the names of Table's whose getTableType() evaluate to TImpalaTableType.VIEW that match an optional pattern when the catalog daemon is started with '--pull_table_types_and_comments=true' which allows the catalog daemon to additionally load the table type of each Table from Hive metastore during startup. Similar to the SHOW TABLES statement, the following are supported after this patch. 1. SHOW VIEWS. 2. SHOW VIEWS "||", where denotes the N-th pattern, "|" denotes choice and each pattern may contain wildcards denoted by "*". 3. SHOW VIEWS IN , where denotes the name of a database. Refer to the added end-to-end test cases for further details. Testing: - Added various frontend and end-to-end tests to verify the behavior of the SHOW VIEWS statement. Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 --- M be/src/service/client-request-state.cc M be/src/service/frontend.cc M be/src/service/frontend.h M common/thrift/Frontend.thrift M fe/src/main/cup/sql-parser.cup M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java A fe/src/main/java/org/apache/impala/analysis/ShowTablesOrViewsStmt.java M fe/src/main/java/org/apache/impala/analysis/ShowTablesStmt.java A fe/src/main/java/org/apache/impala/analysis/ShowViewsStmt.java M fe/src/main/java/org/apache/impala/catalog/Catalog.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/catalog/FeCatalog.java M fe/src/main/java/org/apache/impala/catalog/FeDb.java M fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java M fe/src/main/jflex/sql-scanner.flex M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java A testdata/workloads/functional-query/queries/QueryTest/show_views.test A tests/custom_cluster/test_show_views_statements.py 22 files changed, 556 insertions(+), 112 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/20669/2 -- To view, visit http://gerrit.cloudera.org:8080/20669 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 Gerrit-Change-Number: 20669 Gerrit-PatchSet: 2 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith
[Impala-ASF-CR] [WIP] IMPALA-3268: Add support for SHOW VIEWS statement
Fang-Yu Rao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20669 Change subject: [WIP] IMPALA-3268: Add support for SHOW VIEWS statement .. [WIP] IMPALA-3268: Add support for SHOW VIEWS statement To-do's: - Update code comments. - Add end-to-end tests. Testing: - Briefly verified that SHOW VIEWS statement works whether Impala daemon's startup flag '--use_local_catalog' is true. Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 --- M be/src/service/client-request-state.cc M be/src/service/frontend.cc M be/src/service/frontend.h M common/thrift/Frontend.thrift M fe/src/main/cup/sql-parser.cup M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java A fe/src/main/java/org/apache/impala/analysis/ShowTablesOrViewsStmt.java M fe/src/main/java/org/apache/impala/analysis/ShowTablesStmt.java A fe/src/main/java/org/apache/impala/analysis/ShowViewsStmt.java M fe/src/main/java/org/apache/impala/catalog/Catalog.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/catalog/FeCatalog.java M fe/src/main/java/org/apache/impala/catalog/FeDb.java M fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniCatalog.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java M fe/src/main/jflex/sql-scanner.flex 19 files changed, 284 insertions(+), 96 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/20669/1 -- To view, visit http://gerrit.cloudera.org:8080/20669 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 Gerrit-Change-Number: 20669 Gerrit-PatchSet: 1 Gerrit-Owner: Fang-Yu Rao
[Impala-ASF-CR] IMPALA-12398: Ranger role not exists when altering db/table/view owner to a role
Fang-Yu Rao has posted comments on this change. (
http://gerrit.cloudera.org:8080/20508 )
Change subject: IMPALA-12398: Ranger role not exists when altering
db/table/view owner to a role
..
Patch Set 3:
(4 comments)
Thanks for the detailed explanation Ji! I took a closer look at the patch set 3
and understand now why it's necessary to have the class
CatalogServiceTestCatalogWithRanger.
It's because after the patch set 3, Impala's Analyzer starts checking whether
or not the role exists in the given ALTER statement, i.e.,
AnalysisUtils#checkRole() will be executed at runtime and in the JUnit tests,
e.g., AuthorizationStmtTest#testAlterDatabase().
I only have some minor comments. We could also wait for Quanlong to see he has
some additional suggestion.
http://gerrit.cloudera.org:8080/#/c/20508/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20508/3//COMMIT_MSG@7
PS3, Line 7: Ranger role not exists when altering db/table/view owner to a role
nit: Could we change the title to something like "Check the existence of the
role in the correct place in SET OWNER ROLE"?
http://gerrit.cloudera.org:8080/#/c/20508/3//COMMIT_MSG@9
PS3, Line 9: When Role '' is created with Ranger authorization
enabled, if
: 'ALTER TABLE SET OWNER ROLE ' statement is
: executed to assign role as the owner of the table, it will throw
: AnalysisException:Role '' does not exist.
:
: It is due to in the original design, AuthorizationPolicy class is
used to
: cache the roles/principal, So it is used to check the existence of
the role.
: However, this class is obsolete in certain degree in the current
design,
: in the catalogd side,create/drop role/show role will bypass the
: AuthorizationPolicy object, and directly use the ranger plugin to
perform
: operation on the roles, it also utilize cache within ranger
plugin. That is
: why no roles are available in AuthorizationPolicy object.
:
: This patch will directly use ranger impala plugin to check the
existence of the role,
: instead of using AuthorizationPolicy object.
nit: I was wondering if it would be clearer to change the commit message to the
following. It should be okay that we do not say too much about
AuthorizationPolicy.
Before this patch, given the ALTER DATABASE/TABLE/VIEW SET OWNER ROLE
statement, Impala always checked the existence of the given role in its
AuthorizationPolicy. However, when the support for role-related statements with
Ranger was added in IMPALA-10211, we only added the roles in RangerImpalaPlugin
instead of AuthorizationPolicy. Therefore, the statement above would fail even
though an authorized user tries to set the owner to an existing role in
RangerImpalaPlugin.
This patch fixes the problem by making Impala check the existence of a given
role in RangerImpalaPlugin when the authorization provider is Ranger.
http://gerrit.cloudera.org:8080/#/c/20508/3/fe/src/test/java/org/apache/impala/authorization/CatalogServiceTestCatalogWithRanger.java
File
fe/src/test/java/org/apache/impala/authorization/CatalogServiceTestCatalogWithRanger.java:
http://gerrit.cloudera.org:8080/#/c/20508/3/fe/src/test/java/org/apache/impala/authorization/CatalogServiceTestCatalogWithRanger.java@68
PS3, Line 68: cs.setRangerImpalaPlugin(rangerImpalaPlugin);
: cs.setAuthzManager(factory.newAuthorizationManager(cs));
:
cs.setMetastoreEventProcessor(NoOpEventProcessor.getInstance());
:
cs.setCatalogMetastoreServer(NoOpCatalogMetastoreServer.INSTANCE);
: cs.setCatalogOpExecutor(new CatalogOpExecutor(cs,
: new NoopAuthorizationFactory().getAuthorizationConfig(),
: new NoopAuthorizationFactory.NoopAuthorizationManager(),
: new TestHiveJavaFunctionFactory()));
: cs.setEventFactoryForSyncToLatestEvent(
: new MetastoreEvents.EventFactoryForSyncToLatestEvent(
: cs.getCatalogOpExecutor()));
: cs.reset();
CatalogServiceTestCatalogWithRanger#createWithAuth() shares these statements
with CatalogServiceTestCatalog#createWithAuth(). It would be good if we put
them in a method in the class CatalogServiceTestCatalog and call this method
here and in CatalogServiceTestCatalog#createWithAuth() to eliminate duplicate
code.
http://gerrit.cloudera.org:8080/#/c/20508/3/fe/src/test/java/org/apache/impala/authorization/CatalogServiceTestCatalogWithRanger.java@96
PS3, Line 96: List roleMemberList =
: grantGroups.stream()
: .map(new Function() {
: @Override
: public RangerRole.RoleMember apply(String s) {
[Impala-ASF-CR] IMPALA-12398: Ranger role not exists when altering db/table/view owner to a role
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20508 ) Change subject: IMPALA-12398: Ranger role not exists when altering db/table/view owner to a role .. Patch Set 3: (6 comments) Thanks for the effort Ji! I took a look at this patch and left some minor comments. One thing that's not that clear to me is why we need to instantiate 'authzCatalog_' by an instance of CatalogServiceTestCatalogWithRanger defined in this proposed patch. This patch resolves the reported issue by implementing the method checkRole() to determine whether a given role exists in the Ranger service and checkRole() is already on the code path exercised by the added test cases. Maybe I missed something important. I will take a second pass to better understand this patch. http://gerrit.cloudera.org:8080/#/c/20508/3/fe/src/main/java/org/apache/impala/analysis/AnalysisUtils.java File fe/src/main/java/org/apache/impala/analysis/AnalysisUtils.java: http://gerrit.cloudera.org:8080/#/c/20508/3/fe/src/main/java/org/apache/impala/analysis/AnalysisUtils.java@51 PS3, Line 51: checkRole This method seems to be more related to authorization (v.s. analysis). Would it be better if we define the method checkRole() in the interface AuthorizationManager () (https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/authorization/AuthorizationManager.java) and then implement the method in RangerImpaladAuthorizationManager? In the future if we are adopting another authorization provider other than Ranger, we could implement checkRole() in another class that extends AuthorizationManager.java like https://gerrit.cloudera.org/c/15833/8/fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java that was removed in IMPALA-9708. http://gerrit.cloudera.org:8080/#/c/20508/3/fe/src/main/java/org/apache/impala/authorization/ranger/RangerUtil.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerUtil.java: http://gerrit.cloudera.org:8080/#/c/20508/3/fe/src/main/java/org/apache/impala/authorization/ranger/RangerUtil.java@129 PS3, Line 129: roleExist nit: Maybe it's better to change the name to roleExists. http://gerrit.cloudera.org:8080/#/c/20508/3/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java File fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java: http://gerrit.cloudera.org:8080/#/c/20508/3/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java@2546 PS3, Line 2546: } nit: Add a newline after the right parenthesis. http://gerrit.cloudera.org:8080/#/c/20508/3/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java@2547 PS3, Line 2547: thrown analysis exception nit: throw an AnalysisException. http://gerrit.cloudera.org:8080/#/c/20508/3/fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java File fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java: http://gerrit.cloudera.org:8080/#/c/20508/3/fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java@126 PS3, Line 126: testCatalog_ = CatalogServiceTestCatalogWithRanger.createWithAuth(authzFactory_); Could you briefly explain why we need an instance of CatalogServiceTestCatalogWithRanger to instantiate 'authzCatalog_'? Specifically, what would be different in the frontend authorization tests after we instantiate 'authzCatalog_' as proposed by this patch? http://gerrit.cloudera.org:8080/#/c/20508/3/fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java File fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java: http://gerrit.cloudera.org:8080/#/c/20508/3/fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java@a75 PS3, Line 75: Not very sure if I missed something. But what was wrong with this instantiation? -- To view, visit http://gerrit.cloudera.org:8080/20508 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2b029bdb90111dbd0eab5189360cc81090225cda Gerrit-Change-Number: 20508 Gerrit-PatchSet: 3 Gerrit-Owner: ji chen Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 28 Sep 2023 01:09:03 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12383: (Addendum) Use named params
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20483 ) Change subject: IMPALA-12383: (Addendum) Use named params .. Patch Set 1: Code-Review+2 Thanks Michael! I do not have any comment. -- To view, visit http://gerrit.cloudera.org:8080/20483 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ief492d054abbd00338e27ded9988141db750707c Gerrit-Change-Number: 20483 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Thu, 14 Sep 2023 19:15:54 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12397: NullPointerException in SHOW ROLES when there are no roles
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20439 ) Change subject: IMPALA-12397: NullPointerException in SHOW ROLES when there are no roles .. Patch Set 4: Code-Review+1 Thanks for addressing my previous comments Ji! I do not have any other suggestion. -- To view, visit http://gerrit.cloudera.org:8080/20439 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id80fc2c9152a09194718da1b4266c5f804f0971f Gerrit-Change-Number: 20439 Gerrit-PatchSet: 4 Gerrit-Owner: ji chen Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Fri, 08 Sep 2023 18:05:23 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12397: NullPointerException in SHOW ROLES when there are no roles
Fang-Yu Rao has posted comments on this change. (
http://gerrit.cloudera.org:8080/20439 )
Change subject: IMPALA-12397: NullPointerException in SHOW ROLES when there are
no roles
..
Patch Set 3:
(4 comments)
Thanks for the patch and the new test cases Ji!
I only have some very minor comments.
http://gerrit.cloudera.org:8080/#/c/20439/3/fe/src/test/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManagerTest.java
File
fe/src/test/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManagerTest.java:
http://gerrit.cloudera.org:8080/#/c/20439/3/fe/src/test/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManagerTest.java@48
PS3, Line 48: showRolesParams.setRequesting_user("admin");
nit: It seems that the value of 'requesting_user' does not have to be "admin"
when 'isShowCurrentRole' is true.
In RangerImpaladAuthorizationManager#getRoles(), the operation would be
considered as non-administrative for the SHOW CURRENT ROLES command.
boolean adminOp =
!(groups.contains(params.getGrant_group()) ||
params.is_show_current_roles);
if (adminOp) {
RangerUtil.validateRangerAdmin(plugin_.get(),
params.getRequesting_user());
}
http://gerrit.cloudera.org:8080/#/c/20439/3/fe/src/test/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManagerTest.java@50
PS3, Line 50: }
nit: Add a newline after this right curly bracket.
http://gerrit.cloudera.org:8080/#/c/20439/3/fe/src/test/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManagerTest.java@64
PS3, Line 64: used
nit: thrown
http://gerrit.cloudera.org:8080/#/c/20439/3/tests/authorization/test_ranger.py
File tests/authorization/test_ranger.py:
http://gerrit.cloudera.org:8080/#/c/20439/3/tests/authorization/test_ranger.py@2304
PS3, Line 2304: reset_ranger=True
Thanks! I think this addresses Quanlong's comment at
https://gerrit.cloudera.org/c/20439/2/tests/authorization/test_ranger.py#2321.
When 'reset_ranger' is True, we reset all the policies in the Ranger service
and thus even if there were roles before this test, they will be deleted when
this test runs.
It may be a good idea to add a code comment stating that since the Ranger
policies are reset before this test, we do not have to worry about there could
be existing roles when the test is running.
--
To view, visit http://gerrit.cloudera.org:8080/20439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id80fc2c9152a09194718da1b4266c5f804f0971f
Gerrit-Change-Number: 20439
Gerrit-PatchSet: 3
Gerrit-Owner: ji chen
Gerrit-Reviewer: Fang-Yu Rao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Quanlong Huang
Gerrit-Comment-Date: Wed, 06 Sep 2023 23:05:10 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12311: Remove extra newlines in the updated golden file
Hello Riza Suminto, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20272 to look at the new patch set (#2). Change subject: IMPALA-12311: Remove extra newlines in the updated golden file .. IMPALA-12311: Remove extra newlines in the updated golden file This patch removes extra newlines added to subsections when we parse test queries in an end-to-end test file. Specifically, in parse_test_file_text(), we append an extra newline in every subsection of a test query, resulting in one extra newline in the updated golden file if we add '--update_results' when running this test file to produce the updated golden file. This could be seen by looking at the updated golding file under $IMPALA_HOME/logs/ee_tests after executing the following. $IMPALA_HOME/bin/impala-py.test \ --update_results \ $IMPALA_HOME/tests/query_test/test_tpcds_queries.py::TestTpcdsDecimalV2Query::test_tpcds_q1 The extra newline is needed for the verification of the subsections of RESULTS, DML_RESULTS, ERRORS to disambiguate the case of no lines from a single line with no text and will not be needed after the verification. To remove such extra newlines, we choose to do it in the place when write_test_file() is called to output the updated golden file since this requires fewer changes. An alternative could be to only add an extra newline for those 3 subsections mentioned above and also remove the last newline added in join_section_lines(), which would be called when the actual contents do not match the expected contents specified in the original golden file in the subsections of ERRORS, TYPES, LABELS, RESULTS and DML_RESULTS. Additional changes to TestTestFileParser are also required if we adopted the alternative. Testing: - Verified that the extra newlines are removed after this patch. Change-Id: Ic7668a437267bd76afecba8f87ead32d82580414 --- M tests/util/test_file_parser.py 1 file changed, 11 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/20272/2 -- To view, visit http://gerrit.cloudera.org:8080/20272 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic7668a437267bd76afecba8f87ead32d82580414 Gerrit-Change-Number: 20272 Gerrit-PatchSet: 2 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto
[Impala-ASF-CR] IMPALA-12311: Do not add a trailing newline when updating a golden file
Fang-Yu Rao has posted comments on this change. (
http://gerrit.cloudera.org:8080/20272 )
Change subject: IMPALA-12311: Do not add a trailing newline when updating a
golden file
..
Patch Set 1:
(1 comment)
> Patch Set 1:
>
> (1 comment)
Thanks for the feedback Riza!
I will try to see how to address your comment.
http://gerrit.cloudera.org:8080/#/c/20272/1/tests/util/test_file_parser.py
File tests/util/test_file_parser.py:
http://gerrit.cloudera.org:8080/#/c/20272/1/tests/util/test_file_parser.py@298
PS1, Line 298: return '\n'.join(lines)
> Are you sure this is enough? I try scenario from the JIRA and it still show
Thanks for checking this Riza!
Could you let me know what you changed in
testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test?
If you are changing the expected results in the subsection of ERRORS, I recall
there will still be two additional newlines in the subsection of ERRORS even
after this patch. This patch only removes one newline. But I will double-check
it.
This patch was originally intended for removing the additional newline in the
subsection of RESULTS since that additional newline would fail the verification
of the updated subsection of RESULTS.
And you are correct. After this patch, that code comment ("The inverse of
split_section_lines().") no longer holds. We could probably try removing
additional newlines until there is only one left in the end of last line as you
suggested.
--
To view, visit http://gerrit.cloudera.org:8080/20272
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic7668a437267bd76afecba8f87ead32d82580414
Gerrit-Change-Number: 20272
Gerrit-PatchSet: 1
Gerrit-Owner: Fang-Yu Rao
Gerrit-Reviewer: Fang-Yu Rao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Riza Suminto
Gerrit-Comment-Date: Fri, 28 Jul 2023 01:14:22 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12311: Do not add a trailing newline when updating a golden file
Fang-Yu Rao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20272 Change subject: IMPALA-12311: Do not add a trailing newline when updating a golden file .. IMPALA-12311: Do not add a trailing newline when updating a golden file join_section_lines() is called to join the lines returned from actual results to update the expected results of the subsection when the actual results do not match the original expected results of a subsection, which could be ERRORS, TYPES, LABELS, or RESULTS. Before this patch, join_section_lines() added a trailing newline at the end of the joined lines, causing the verification of the updated subsection of RESULTS to fail if we used the updated golden file to replace the original one. This additional newline only affected the subsection of RESULTS in that trailing newlines in the subsections of ERRORS, TYPES, and LABELS in a test file are removed when they are constructed in the test. For instance, if we changed a line in the subsection of RESULTS tpcds-decimal_v2-q1.test to make the expected results differ from the actual ones to trigger a call to join_section_lines() with the following on the command line, we would find that the updated subsection could not pass the verification unless we manually removed that additional newline. Recall that the updated golden file by default would be produced under $IMPALA_HOME/logs/ee_tests. $IMPALA_HOME/bin/impala-py.test \ --update_results \ $IMPALA_HOME/tests/query_test/test_tpcds_queries.py::TestTpcdsDecimalV2Query::test_tpcds_q1 This patch removes that trailing newline added in join_section_lines() to resolve the issue. Testing: - Manually verified that when join_section_lines() is called, the updated subsection of RESULTS could pass the verification. Change-Id: Ic7668a437267bd76afecba8f87ead32d82580414 --- M tests/util/test_file_parser.py 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/20272/1 -- To view, visit http://gerrit.cloudera.org:8080/20272 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ic7668a437267bd76afecba8f87ead32d82580414 Gerrit-Change-Number: 20272 Gerrit-PatchSet: 1 Gerrit-Owner: Fang-Yu Rao
[Impala-ASF-CR] IMPALA-12256: Fix DDLs losing create event ids in reloading partitions
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20145 ) Change subject: IMPALA-12256: Fix DDLs losing create event ids in reloading partitions .. Patch Set 6: (1 comment) Thanks for the explanation Quanlong! I do not have other questions. :-) http://gerrit.cloudera.org:8080/#/c/20145/6/tests/custom_cluster/test_events_custom_configs.py File tests/custom_cluster/test_events_custom_configs.py: http://gerrit.cloudera.org:8080/#/c/20145/6/tests/custom_cluster/test_events_custom_configs.py@858 PS6, Line 858: self.client.execute( : "alter table %s.part drop partition (p=0)" % unique_database) : self.client.execute( : "insert into %s.part partition(p=0) values (1),(2)" % unique_database) : self.client.execute(ddl) > > When we submit the 3 queries above to Impala, is it true that the order o Thanks for the detailed explanation Quanlong! :-) Now it's much clearer to me as to why "events-skipped" should be increased even in the case in which only the first DDL (ALTER TABLE DROP PARTITION) was finished when the event processor receives from HMS the corresponding DROP_PARTITION event. In such a case, 'hdfsPartition' within CatalogOpExecutor#canDropPartitionFromEvent() ( https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java#L4546) would be null and thus such a DROP_PARTITION event has to be skipped by the event processor. Thus, whether or not the 2nd DDL (INSERT INTO PARTITION) is finished when the DROP_PARTITION event is received, such an event will be skipped. -- To view, visit http://gerrit.cloudera.org:8080/20145 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I052faa093bda69fb16db0d424c1478bba103dad9 Gerrit-Change-Number: 20145 Gerrit-PatchSet: 6 Gerrit-Owner: Quanlong Huang Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Sai Hemanth Gantasala Gerrit-Comment-Date: Mon, 10 Jul 2023 02:25:20 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12256: Fix DDLs losing create event ids in reloading partitions
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20145 ) Change subject: IMPALA-12256: Fix DDLs losing create event ids in reloading partitions .. Patch Set 6: (1 comment) Sorry I have another question that I forgot to ask in my previous review. I asked this question mostly for my own understanding. Thanks! http://gerrit.cloudera.org:8080/#/c/20145/6/tests/custom_cluster/test_events_custom_configs.py File tests/custom_cluster/test_events_custom_configs.py: http://gerrit.cloudera.org:8080/#/c/20145/6/tests/custom_cluster/test_events_custom_configs.py@858 PS6, Line 858: self.client.execute( : "alter table %s.part drop partition (p=0)" % unique_database) : self.client.execute( : "insert into %s.part partition(p=0) values (1),(2)" % unique_database) : self.client.execute(ddl) I have another question regarding the events corresponding to the 3 queries above. When we submit the 3 queries above to Impala, is it true that the order of the respective events received from HMS is non-deterministic? What if we receive the DROP_PARTITION event prior to the events corresponding to the other 2 queries? Will this DROP_PARTITION event be skipped in this case? Or some logic ensures even in this case (the DROP_PARTITION event arriving first) this DROP_PARTITION will still be skipped? -- To view, visit http://gerrit.cloudera.org:8080/20145 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I052faa093bda69fb16db0d424c1478bba103dad9 Gerrit-Change-Number: 20145 Gerrit-PatchSet: 6 Gerrit-Owner: Quanlong Huang Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Sai Hemanth Gantasala Gerrit-Comment-Date: Sat, 08 Jul 2023 01:29:51 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12256: Fix DDLs losing create event ids in reloading partitions
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20145 ) Change subject: IMPALA-12256: Fix DDLs losing create event ids in reloading partitions .. Patch Set 6: Code-Review+1 (1 comment) Thanks for addressing my previous comments Quanlong! I do not have any other suggestion. http://gerrit.cloudera.org:8080/#/c/20145/4/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/20145/4/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2904 PS4, Line 2904: partBuilder.setCreateEventId(oldPartition.getCreateEventId()); : partBuilder.setLastCompactionId(oldPartition.getLastCompactionId()); > 1. The lastRefreshEventId is set below at line 2907. Its source value is fe Thanks for the explanation Quanlong! Yeah. You are correct. We do not need to use the 'lastRefreshEventId_' from 'oldPartition' since 'latestEventId' is always higher/fresher and I just realized that even when we are processing a non-self INSERT event, we still update the field of 'lastRefreshEventId_' with the event ID of this INSERT event. -- To view, visit http://gerrit.cloudera.org:8080/20145 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I052faa093bda69fb16db0d424c1478bba103dad9 Gerrit-Change-Number: 20145 Gerrit-PatchSet: 6 Gerrit-Owner: Quanlong Huang Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Sai Hemanth Gantasala Gerrit-Comment-Date: Mon, 03 Jul 2023 20:57:56 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12256: Fix DDLs losing partition-level create event ids in reloads
Fang-Yu Rao has posted comments on this change. (
http://gerrit.cloudera.org:8080/20145 )
Change subject: IMPALA-12256: Fix DDLs losing partition-level create event ids
in reloads
..
Patch Set 4: Code-Review+1
(3 comments)
Thanks for the analysis and the prompt fix Quanlong!
I only have some minor questions mostly because I know very little around this
area.
http://gerrit.cloudera.org:8080/#/c/20145/4/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
File fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java:
http://gerrit.cloudera.org:8080/#/c/20145/4/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@1326
PS4, Line 1326: loss
nit: Should we use "lose" instead?
http://gerrit.cloudera.org:8080/#/c/20145/4/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
http://gerrit.cloudera.org:8080/#/c/20145/4/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2904
PS4, Line 2904:
partBuilder.setCreateEventId(oldPartition.getCreateEventId());
:
partBuilder.setLastCompactionId(oldPartition.getLastCompactionId());
I have 2 questions.
1. At
https://gerrit.cloudera.org/c/20145/4/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java#1329
we also populate the field 'lastRefreshEventId_'. I wonder if we also need to
pick up the value of 'lastRefreshEventId_' like the following. Or we do not
really need to do this because we already populate the field
'lastRefreshEventId_' somewhere else and this field will be available when the
event processor is processing a reload event (e.g., at
https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java#L3891)?
partBuilder.setLastRefreshEventId(oldPartition.getLastRefreshEventId());
2. Here we also pick up the value of 'lastCompactionId_'. Will this piece of
information be required to fix the issue reported in this JIRA? Or we just
want to keep this field in sync with the same field in the partition that was
just updated by the DDL command?
http://gerrit.cloudera.org:8080/#/c/20145/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/20145/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4544
PS4, Line 4544: // if the partition has been created since the event was
generated, skip
: // the stale event.
: boolean isStale = hdfsPartition.getCreateEventId() >
eventId;
: LOG.info("{} partition {} of table {} since it's create
event id {} is {} than " +
: "eventid {}",
: isStale ? "Not dropping" : "Dropping",
: hdfsPartition.getPartitionName(),
hdfsTable.getFullName(),
: hdfsPartition.getCreateEventId(), isStale ? "higher" :
"not higher",
: eventId);
: return !isStale;
:
At
https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java#L3891
we have the following check within
CatalogServiceCatalog.java#isPartitionLoadedAfterEvent(). I wonder if it would
also be good for us to log the reload event?
HdfsPartition hdfsPartition = getHdfsPartition(dbName, tableName,
msPartition);
if (hdfsPartition.getLastRefreshEventId() > eventId) {
return true;
}
--
To view, visit http://gerrit.cloudera.org:8080/20145
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I052faa093bda69fb16db0d424c1478bba103dad9
Gerrit-Change-Number: 20145
Gerrit-PatchSet: 4
Gerrit-Owner: Quanlong Huang
Gerrit-Reviewer: Fang-Yu Rao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Quanlong Huang
Gerrit-Reviewer: Sai Hemanth Gantasala
Gerrit-Comment-Date: Sun, 02 Jul 2023 00:06:33 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12248: Add three configuration properties after RANGER-2895
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20129 ) Change subject: IMPALA-12248: Add three configuration properties after RANGER-2895 .. Patch Set 2: (1 comment) > Patch Set 1: > > (1 comment) > > Thank you for working on this, Fang-Yu. > I just have one request. Thanks for the review Riza! I have addressed your comments in the previous patch. Let me know if you have any other suggestion. http://gerrit.cloudera.org:8080/#/c/20129/1/testdata/cluster/ranger/ranger-admin-default-site.xml.template File testdata/cluster/ranger/ranger-admin-default-site.xml.template: http://gerrit.cloudera.org:8080/#/c/20129/1/testdata/cluster/ranger/ranger-admin-default-site.xml.template@305 PS1, Line 305: ranger.jpa.jdbc.initialpoolsize > In the JIRA, you mention "In this regard, we could probably add these 3 new Thanks Riza! I have revised the commit message in the next patch and also created IMPALA-12250 to keep track of the task of removing the deprecated property. -- To view, visit http://gerrit.cloudera.org:8080/20129 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I19a27e3fe3ab96a9f60566dc2c87bd72636b91ae Gerrit-Change-Number: 20129 Gerrit-PatchSet: 2 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Wed, 28 Jun 2023 05:52:42 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12248: Add three configuration properties after RANGER-2895
Hello Riza Suminto, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20129 to look at the new patch set (#2). Change subject: IMPALA-12248: Add three configuration properties after RANGER-2895 .. IMPALA-12248: Add three configuration properties after RANGER-2895 This patch adds 3 Ranger configuration properties that will be required after we start using a build that includes RANGER-2895 so that Ranger's HTTP server could be properly started. On the other hand, recall that a Ranger configuration property was deprecated in RANGER-2895, i.e., ranger.jpa.jdbc.idleconnectiontestperiod. Hence, we should also remove it when starting using a build that contains RANGER-2895. This task will be tracked by IMPALA-12250. Testing: - Verified that with this patch Ranger's HTTP server could be started and that we will be able to update the users, groups, and policies on the Ranger server with the current scripts in the Impala repository whether or not we are using a build that has RANGER-2895. Change-Id: I19a27e3fe3ab96a9f60566dc2c87bd72636b91ae --- M testdata/cluster/ranger/ranger-admin-default-site.xml.template 1 file changed, 18 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/20129/2 -- To view, visit http://gerrit.cloudera.org:8080/20129 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I19a27e3fe3ab96a9f60566dc2c87bd72636b91ae Gerrit-Change-Number: 20129 Gerrit-PatchSet: 2 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto
[Impala-ASF-CR] IMPALA-12248: Add missing configuration properties after RANGER-2895
Fang-Yu Rao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20129 Change subject: IMPALA-12248: Add missing configuration properties after RANGER-2895 .. IMPALA-12248: Add missing configuration properties after RANGER-2895 This patch adds 3 Ranger configuration properties that are required after RANGER-2895 so that the Ranger HTTP server could be properly started. Testing: - Verified that after this patch Ranger's HTTP server could be started and that we will be able to update the users, groups, and policies on the Ranger server with the current scripts in the Impala repository. Change-Id: I19a27e3fe3ab96a9f60566dc2c87bd72636b91ae --- M testdata/cluster/ranger/ranger-admin-default-site.xml.template 1 file changed, 18 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/20129/1 -- To view, visit http://gerrit.cloudera.org:8080/20129 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I19a27e3fe3ab96a9f60566dc2c87bd72636b91ae Gerrit-Change-Number: 20129 Gerrit-PatchSet: 1 Gerrit-Owner: Fang-Yu Rao
[Impala-ASF-CR] IMPALA-11912: Inline base::IsAarch64
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/19492 ) Change subject: IMPALA-11912: Inline base::IsAarch64 .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/19492/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19492/1//COMMIT_MSG@7 PS1, Line 7: IMPALA-11912 nit: Should this be IMPALA-11916 instead? -- To view, visit http://gerrit.cloudera.org:8080/19492 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id6d62de63a0cb7b94244a1d4f8dcc6b2e65b6d9c Gerrit-Change-Number: 19492 Gerrit-PatchSet: 1 Gerrit-Owner: Quanlong Huang Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Mon, 13 Feb 2023 05:36:54 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11845: Fix incorrect check of struct STAR path in resolvePathWithMasking
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/19429 ) Change subject: IMPALA-11845: Fix incorrect check of struct STAR path in resolvePathWithMasking .. Patch Set 6: Code-Review+1 Thanks for adding those 4 test queries in ranger_column_masking_complex_types.test to characterize Impala's current behavior! I do not have any additional suggestion. -- To view, visit http://gerrit.cloudera.org:8080/19429 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8f1e78e325baafbe23101909d47e82bf140a2d77 Gerrit-Change-Number: 19429 Gerrit-PatchSet: 6 Gerrit-Owner: Quanlong Huang Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Mon, 30 Jan 2023 05:42:29 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11845: Fix incorrect check of struct STAR path in resolvePathWithMasking
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/19429 ) Change subject: IMPALA-11845: Fix incorrect check of struct STAR path in resolvePathWithMasking .. Patch Set 5: (2 comments) Thanks for preparing a fix so quickly Quanlong! I only have 2 very minor comments and the second one regarding adding a code comment is only optional. I raised that question because I do not know enough about how paths are resolved in Impala's FE in detail. http://gerrit.cloudera.org:8080/#/c/19429/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19429/5//COMMIT_MSG@39 PS5, Line 39: as a nit: "as a" should be put in the next line. http://gerrit.cloudera.org:8080/#/c/19429/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/19429/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1196 PS5, Line 1196: resolvedPath.getMatchedTypes( Is it true that when 'pathType' is PathType.STAR and resolvedPath.getMatchedTypes() is not empty, 'resolvedPath' has to correspond to a struct column, e.g., the column of 'nested_struct' of the table 'complextypestbl' under the database 'functional_parquet'? If so, it may be good to add a more detailed code comment at the definition of 'matchedTypes_' in Path.java as well. -- To view, visit http://gerrit.cloudera.org:8080/19429 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8f1e78e325baafbe23101909d47e82bf140a2d77 Gerrit-Change-Number: 19429 Gerrit-PatchSet: 5 Gerrit-Owner: Quanlong Huang Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Mon, 23 Jan 2023 05:05:30 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10986 (Addendum): Add and refactor some E2E tests
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/19394 ) Change subject: IMPALA-10986 (Addendum): Add and refactor some E2E tests .. Patch Set 9: Thanks for the +2 Qifan! Does anyone else still have any comment on the patch? Thanks very much for the help! -- To view, visit http://gerrit.cloudera.org:8080/19394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieb4f69934401a745da66a983528a7a3679279c28 Gerrit-Change-Number: 19394 Gerrit-PatchSet: 9 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Mon, 16 Jan 2023 03:05:52 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10986 (Addendum): Add and refactor some E2E tests
Fang-Yu Rao has posted comments on this change. (
http://gerrit.cloudera.org:8080/19394 )
Change subject: IMPALA-10986 (Addendum): Add and refactor some E2E tests
..
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19394/9/tests/authorization/test_ranger.py
File tests/authorization/test_ranger.py:
http://gerrit.cloudera.org:8080/#/c/19394/9/tests/authorization/test_ranger.py@1337
PS9, Line 1337: err = ("AuthorizationException: User '{0}' does not have
privileges to execute"
: + " 'SELECT' on: {1}.foo").format(test_user,
test_db)
I revised this to get rid of the warning of "line break after binary operator."
--
To view, visit http://gerrit.cloudera.org:8080/19394
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb4f69934401a745da66a983528a7a3679279c28
Gerrit-Change-Number: 19394
Gerrit-PatchSet: 9
Gerrit-Owner: Fang-Yu Rao
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Fang-Yu Rao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Qifan Chen
Gerrit-Reviewer: Quanlong Huang
Gerrit-Comment-Date: Fri, 06 Jan 2023 23:36:32 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10986 (Addendum): Add and refactor some E2E tests
Fang-Yu Rao has posted comments on this change. (
http://gerrit.cloudera.org:8080/19394 )
Change subject: IMPALA-10986 (Addendum): Add and refactor some E2E tests
..
Patch Set 9:
(1 comment)
> Patch Set 7:
>
> (1 comment)
http://gerrit.cloudera.org:8080/#/c/19394/7/tests/authorization/test_ranger.py
File tests/authorization/test_ranger.py:
http://gerrit.cloudera.org:8080/#/c/19394/7/tests/authorization/test_ranger.py@329
PS7, Line 329: admin_client, "grant create on user_defined_fn {0}.{1}
to {2} {3}"
> The idention looks lengthy. Can we use the second idention style mentioned
Thanks Quanlong!
I have changed the indentation to make the code less lengthy in patch set 9.
--
To view, visit http://gerrit.cloudera.org:8080/19394
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb4f69934401a745da66a983528a7a3679279c28
Gerrit-Change-Number: 19394
Gerrit-PatchSet: 9
Gerrit-Owner: Fang-Yu Rao
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Fang-Yu Rao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Qifan Chen
Gerrit-Reviewer: Quanlong Huang
Gerrit-Comment-Date: Fri, 06 Jan 2023 23:25:01 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10986 (Addendum): Add and refactor some E2E tests
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/19394 ) Change subject: IMPALA-10986 (Addendum): Add and refactor some E2E tests .. Patch Set 9: (44 comments) I have fixed all the indentation problems pointed out by impala-flake8 in patch set 9. http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py@330 PS8, Line 330: m > flake8: E131 continuation line unaligned for hanging indent Done http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py@332 PS8, Line 332: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py@333 PS8, Line 333: [ > flake8: E122 continuation line missing indentation or outdented Done http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py@338 PS8, Line 338: m > flake8: E131 continuation line unaligned for hanging indent Done http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py@341 PS8, Line 341: [ > flake8: E122 continuation line missing indentation or outdented Done http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py@342 PS8, Line 342: [ > flake8: E122 continuation line missing indentation or outdented Done http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py@347 PS8, Line 347: m > flake8: E131 continuation line unaligned for hanging indent Done http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py@350 PS8, Line 350: [ > flake8: E122 continuation line missing indentation or outdented Done http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py@351 PS8, Line 351: [ > flake8: E122 continuation line missing indentation or outdented Done http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py@352 PS8, Line 352: [ > flake8: E122 continuation line missing indentation or outdented Done http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py@357 PS8, Line 357: m > flake8: E131 continuation line unaligned for hanging indent Done http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py@360 PS8, Line 360: [ > flake8: E122 continuation line missing indentation or outdented Done http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py@384 PS8, Line 384: , > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py@407 PS8, Line 407: m > flake8: E131 continuation line unaligned for hanging indent Done http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py@410 PS8, Line 410: [ > flake8: E122 continuation line missing indentation or outdented Done http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py@430 PS8, Line 430: m > flake8: E131 continuation line unaligned for hanging indent Done http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py@437 PS8, Line 437: m > flake8: E131 continuation line unaligned for hanging indent Done http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py@439 PS8, Line 439: [ > flake8: E122 continuation line missing indentation or outdented Done http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py@440 PS8, Line 440: [ > flake8: E122 continuation line missing indentation or outdented Done http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py@445 PS8, Line 445: m > flake8: E131 continuation line unaligned for hanging indent Done http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py@451 PS8, Line 451: m > flake8: E131 continuation line unaligned for hanging indent Done http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py@454 PS8, Line 454: [ > flake8: E122 continuation line missing indentation or outdented Done http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py@460 PS8, Line 460: m > flake8: E131 continuation line unaligned for hanging indent Done http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py@468 PS8, Line 468: m > flake8: E131 continuation line unaligned for hanging indent Done http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py@470 PS8, Line 470: [ > flake8: E122 continuation line missing indentation or outdented Done http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py@480 PS8, Line 480: > flake8: E241 multiple spaces after ',' Done http://gerrit.cloudera.org:8080/#/c/19394/8/tests/authorization/test_ranger.py@481 PS8, Line 481: m > flake8: E131 continuation line
[Impala-ASF-CR] IMPALA-10986 (Addendum): Add and refactor some E2E tests
Hello Quanlong Huang, Qifan Chen, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/19394 to look at the new patch set (#9). Change subject: IMPALA-10986 (Addendum): Add and refactor some E2E tests .. IMPALA-10986 (Addendum): Add and refactor some E2E tests This patch adds an additional test case in test_select_function to verify Impala's behavior when a user tries to execute a UDF in an INSERT statement. Moreover, some test functions added in IMPALA-10986 are refactored according to reviewers' suggestion. Change-Id: Ieb4f69934401a745da66a983528a7a3679279c28 --- M tests/authorization/test_ranger.py 1 file changed, 239 insertions(+), 242 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/19394/9 -- To view, visit http://gerrit.cloudera.org:8080/19394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ieb4f69934401a745da66a983528a7a3679279c28 Gerrit-Change-Number: 19394 Gerrit-PatchSet: 9 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Quanlong Huang
[Impala-ASF-CR] IMPALA-10986 (Addendum): Add and refactor some E2E tests
Hello Quanlong Huang, Qifan Chen, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/19394 to look at the new patch set (#8). Change subject: IMPALA-10986 (Addendum): Add and refactor some E2E tests .. IMPALA-10986 (Addendum): Add and refactor some E2E tests This patch adds an additional test case in test_select_function to verify Impala's behavior when a user tries to execute a UDF in an INSERT statement. Moreover, some test functions added in IMPALA-10986 are refactored according to reviewers' suggestion. Change-Id: Ieb4f69934401a745da66a983528a7a3679279c28 --- M tests/authorization/test_ranger.py 1 file changed, 237 insertions(+), 240 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/19394/8 -- To view, visit http://gerrit.cloudera.org:8080/19394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ieb4f69934401a745da66a983528a7a3679279c28 Gerrit-Change-Number: 19394 Gerrit-PatchSet: 8 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Quanlong Huang
[Impala-ASF-CR] IMPALA-10986 (Addendum): Add and refactor some E2E tests
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/19394 ) Change subject: IMPALA-10986 (Addendum): Add and refactor some E2E tests .. Patch Set 7: (1 comment) Thanks for the review Qifan! Let me know if anyone still has any suggestion. Thanks! http://gerrit.cloudera.org:8080/#/c/19394/6/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/19394/6/tests/authorization/test_ranger.py@1248 PS6, Line 1248: 'test_ > nit 'test_user' Done -- To view, visit http://gerrit.cloudera.org:8080/19394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieb4f69934401a745da66a983528a7a3679279c28 Gerrit-Change-Number: 19394 Gerrit-PatchSet: 7 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Tue, 03 Jan 2023 21:05:39 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10986 (Addendum): Add and refactor some E2E tests
Hello Quanlong Huang, Qifan Chen, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/19394 to look at the new patch set (#7). Change subject: IMPALA-10986 (Addendum): Add and refactor some E2E tests .. IMPALA-10986 (Addendum): Add and refactor some E2E tests This patch adds an additional test case in test_select_function to verify Impala's behavior when a user tries to execute a UDF in an INSERT statement. Moreover, some test functions added in IMPALA-10986 are refactored according to reviewers' suggestion. Change-Id: Ieb4f69934401a745da66a983528a7a3679279c28 --- M tests/authorization/test_ranger.py 1 file changed, 317 insertions(+), 240 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/19394/7 -- To view, visit http://gerrit.cloudera.org:8080/19394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ieb4f69934401a745da66a983528a7a3679279c28 Gerrit-Change-Number: 19394 Gerrit-PatchSet: 7 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Quanlong Huang
[Impala-ASF-CR] IMPALA-10986 (Addendum): Add and refactor some E2E tests
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/19394 ) Change subject: IMPALA-10986 (Addendum): Add and refactor some E2E tests .. Patch Set 6: Hi all, please let me know if there is any comment on this patch. This patch does not contain any functionality change. It was prepared according to Qifan's suggestions in the following. 1. https://gerrit.cloudera.org/c/19194/15//COMMIT_MSG#7. I added a new test case at https://gerrit.cloudera.org/c/19394/6/tests/authorization/test_ranger.py#1254 to include the scenario where UDF is called in an INSERT statement. 2. https://gerrit.cloudera.org/c/19194/15/tests/authorization/test_ranger.py#482. The code comment has been revised at https://gerrit.cloudera.org/c/19394/6/tests/authorization/test_ranger.py#528. 3. https://gerrit.cloudera.org/c/19194/15/tests/authorization/test_ranger.py#526. I refactored the code by wrapping the pattern in the function _update_privileges_and_verify() at https://gerrit.cloudera.org/c/19394/6/tests/authorization/test_ranger.py#192. This pattern is used throughout test_ranger.py. I only refactored the code added in IMPALA-10986 and not very sure whether I should also refactor the functions not touched in IMPALA-10986. -- To view, visit http://gerrit.cloudera.org:8080/19394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieb4f69934401a745da66a983528a7a3679279c28 Gerrit-Change-Number: 19394 Gerrit-PatchSet: 6 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Tue, 03 Jan 2023 17:23:55 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10986 (Addendum): Add and refactor some E2E tests
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/19394 ) Change subject: IMPALA-10986 (Addendum): Add and refactor some E2E tests .. Patch Set 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/19394/5/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/19394/5/tests/authorization/test_ranger.py@530 PS5, Line 530: > flake8: E501 line too long (93 > 90 characters) Done http://gerrit.cloudera.org:8080/#/c/19394/5/tests/authorization/test_ranger.py@549 PS5, Line 549: > flake8: E501 line too long (93 > 90 characters) Done -- To view, visit http://gerrit.cloudera.org:8080/19394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieb4f69934401a745da66a983528a7a3679279c28 Gerrit-Change-Number: 19394 Gerrit-PatchSet: 6 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 03 Jan 2023 01:37:49 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10986 (Addendum): Add and refactor some E2E tests
Hello Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/19394 to look at the new patch set (#6). Change subject: IMPALA-10986 (Addendum): Add and refactor some E2E tests .. IMPALA-10986 (Addendum): Add and refactor some E2E tests This patch adds an additional test case in test_select_function to verify Impala's behavior when a user tries to execute a UDF in an INSERT statement. Moreover, some test functions added in IMPALA-10986 are refactored according to reviewers' suggestion. Change-Id: Ieb4f69934401a745da66a983528a7a3679279c28 --- M tests/authorization/test_ranger.py 1 file changed, 317 insertions(+), 240 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/19394/6 -- To view, visit http://gerrit.cloudera.org:8080/19394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ieb4f69934401a745da66a983528a7a3679279c28 Gerrit-Change-Number: 19394 Gerrit-PatchSet: 6 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-10986 (Addendum): Add and refactor some E2E tests
Fang-Yu Rao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19394 Change subject: IMPALA-10986 (Addendum): Add and refactor some E2E tests .. IMPALA-10986 (Addendum): Add and refactor some E2E tests This patch adds an additional test case in test_select_function to verify Impala's behavior when a user tries to execute a UDF in an INSERT statement. Moreover, some test functions added in IMPALA-10986 are refactored according to reviewers' suggestion. Change-Id: Ieb4f69934401a745da66a983528a7a3679279c28 --- M tests/authorization/test_ranger.py 1 file changed, 317 insertions(+), 240 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/19394/5 -- To view, visit http://gerrit.cloudera.org:8080/19394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ieb4f69934401a745da66a983528a7a3679279c28 Gerrit-Change-Number: 19394 Gerrit-PatchSet: 5 Gerrit-Owner: Fang-Yu Rao
[Impala-ASF-CR] IMPALA-7942 (part 1): Add query hints for table cardinalities
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/18829 ) Change subject: IMPALA-7942 (part 1): Add query hints for table cardinalities .. Patch Set 10: > Patch Set 10: > > > Hi Quanlong, I submited pre-review-test, but failed in these tests: > > > > authorization.test_ranger.TestRanger.test_show_grant_hive_privilege > > generate_junitxml.buildall.run-custom-cluster-tests > > > > Seems unrelated to this patch. > > Here is the task url: > https://jenkins.impala.io/job/pre-review-test/1490/console Thanks Sheng! test_show_grant_hive_privilege is broken by IMPALA-10986 since the number of added Ranger policies in tests reaches a certain number after IMPALA-10986 in the pre-review-test. I think all (or most of) the authorization-related tests are run with a single instance of the Ranger server in the pre-review-test. We do not observe it in the gerrit-verify-dryrun-external probably because those authorization-related tests are run in different shards each having its own instance of Ranger server and the limit is not reached yet. So the failure was not caused by your patch. I have a patch at https://gerrit.cloudera.org/c/19373 that will be approved soon. -- To view, visit http://gerrit.cloudera.org:8080/18829 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9f0c773f4e67782a1428db64062f68afbd257af7 Gerrit-Change-Number: 18829 Gerrit-PatchSet: 10 Gerrit-Owner: wangsheng Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Fucun Chu Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: wangsheng Gerrit-Comment-Date: Mon, 26 Dec 2022 23:10:42 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10399, IMPALA-11060, IMPALA-11788: Reset Ranger policy repository in an E2E test
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/19373 ) Change subject: IMPALA-10399, IMPALA-11060, IMPALA-11788: Reset Ranger policy repository in an E2E test .. Patch Set 5: Thanks for the +2 Andrew! :-) I just found that there was a redundant space in patch set 4 at https://gerrit.cloudera.org/c/19373/4/tests/authorization/test_ranger.py#744 and have removed it in the next patch. -- To view, visit http://gerrit.cloudera.org:8080/19373 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iff56ec03ceeb2912039241ea302f4bb8948d61f8 Gerrit-Change-Number: 19373 Gerrit-PatchSet: 5 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Mon, 26 Dec 2022 22:57:39 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10399, IMPALA-11060, IMPALA-11788: Reset Ranger policy repository in an E2E test
Hello Quanlong Huang, Andrew Sherman, Csaba Ringhofer, Michael Smith, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/19373 to look at the new patch set (#5). Change subject: IMPALA-10399, IMPALA-11060, IMPALA-11788: Reset Ranger policy repository in an E2E test .. IMPALA-10399, IMPALA-11060, IMPALA-11788: Reset Ranger policy repository in an E2E test test_show_grant_hive_privilege() uses Ranger's REST API to get all the existing policies from the Ranger server after creating a policy that grants the LOCK and SELECT privileges on all the tables and columns in the unique database in order to verify the granted privileges indeed exist in Ranger's policy repository. The way we download all the policies from the Ranger server in test_show_grant_hive_privilege(), however, did not always work. Specifically, when there were already a lot of existing policies in Ranger, the policy that granted the LOCK and SELECT privileges would not be included in the result returned via one single GET request. We found that to reproduce the issue it suffices to add 300 Ranger policies before adding the policy granting those 2 privileges. Moreover, we found that even we set the argument 'stream' of requests.get() to True and used iter_content() to read the response in chunks, we still could not retrieve the policy added in test_show_grant_hive_privilege(). As a workaround, instead of changing how we download all the policies from the Ranger server, this patch resets Ranger's policy repository for Impala before we create the policy granting those 2 privileges so that this test will be more resilient to the number of existing policies in the repository. Change-Id: Iff56ec03ceeb2912039241ea302f4bb8948d61f8 --- M testdata/bin/create-load-data.sh A testdata/bin/setup-ranger.sh M tests/authorization/test_ranger.py M tests/common/custom_cluster_test_suite.py 4 files changed, 115 insertions(+), 73 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/19373/5 -- To view, visit http://gerrit.cloudera.org:8080/19373 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iff56ec03ceeb2912039241ea302f4bb8948d61f8 Gerrit-Change-Number: 19373 Gerrit-PatchSet: 5 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Quanlong Huang
[Impala-ASF-CR] IMPALA-10399, IMPALA-11060, IMPALA-11788: Reset Ranger policy repository in an E2E test
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/19373 ) Change subject: IMPALA-10399, IMPALA-11060, IMPALA-11788: Reset Ranger policy repository in an E2E test .. Patch Set 4: Only the commit message was slightly updated in patch set 4 according to Quanlong's suggestion previously. Let me know if there are additional comments. Thanks! -- To view, visit http://gerrit.cloudera.org:8080/19373 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iff56ec03ceeb2912039241ea302f4bb8948d61f8 Gerrit-Change-Number: 19373 Gerrit-PatchSet: 4 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Wed, 21 Dec 2022 18:14:39 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10399, IMPALA-11060, IMPALA-11788: Reset Ranger policy repository in an E2E test
Hello Quanlong Huang, Csaba Ringhofer, Michael Smith, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/19373 to look at the new patch set (#4). Change subject: IMPALA-10399, IMPALA-11060, IMPALA-11788: Reset Ranger policy repository in an E2E test .. IMPALA-10399, IMPALA-11060, IMPALA-11788: Reset Ranger policy repository in an E2E test test_show_grant_hive_privilege() uses Ranger's REST API to get all the existing policies from the Ranger server after creating a policy that grants the LOCK and SELECT privileges on all the tables and columns in the unique database in order to verify the granted privileges indeed exist in Ranger's policy repository. The way we download all the policies from the Ranger server in test_show_grant_hive_privilege(), however, did not always work. Specifically, when there were already a lot of existing policies in Ranger, the policy that granted the LOCK and SELECT privileges would not be included in the result returned via one single GET request. We found that to reproduce the issue it suffices to add 300 Ranger policies before adding the policy granting those 2 privileges. Moreover, we found that even we set the argument 'stream' of requests.get() to True and used iter_content() to read the response in chunks, we still could not retrieve the policy added in test_show_grant_hive_privilege(). As a workaround, instead of changing how we download all the policies from the Ranger server, this patch resets Ranger's policy repository for Impala before we create the policy granting those 2 privileges so that this test will be more resilient to the number of existing policies in the repository. Change-Id: Iff56ec03ceeb2912039241ea302f4bb8948d61f8 --- M testdata/bin/create-load-data.sh A testdata/bin/setup-ranger.sh M tests/authorization/test_ranger.py M tests/common/custom_cluster_test_suite.py 4 files changed, 116 insertions(+), 73 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/19373/4 -- To view, visit http://gerrit.cloudera.org:8080/19373 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iff56ec03ceeb2912039241ea302f4bb8948d61f8 Gerrit-Change-Number: 19373 Gerrit-PatchSet: 4 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Quanlong Huang
[Impala-ASF-CR] IMPALA-10399, IMPALA-11060, IMPALA-11788: Reset Ranger policy repository in an E2E test
Fang-Yu Rao has posted comments on this change. (
http://gerrit.cloudera.org:8080/19373 )
Change subject: IMPALA-10399, IMPALA-11060, IMPALA-11788: Reset Ranger policy
repository in an E2E test
..
Patch Set 3:
(2 comments)
Hi all, I have addressed Quanlong's comments in the previous iteration.
Let me know if there is any additional suggestion.
Thanks very much for the help!
http://gerrit.cloudera.org:8080/#/c/19373/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19373/2//COMMIT_MSG@15
PS2, Line 15: patch, th
> nit: download
Thanks Quanlong!
To emphasize this was the behavior prior to this patch, in the updated commit
message, I will add "Before this patch" in front the this sentence.
http://gerrit.cloudera.org:8080/#/c/19373/2/testdata/bin/setup-ranger.sh
File testdata/bin/setup-ranger.sh:
http://gerrit.cloudera.org:8080/#/c/19373/2/testdata/bin/setup-ranger.sh@25
PS2, Line 25: function setup-ranger {
> Thanks for extracting this! It also fixes IMPALA-11060.
Ack
--
To view, visit http://gerrit.cloudera.org:8080/19373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iff56ec03ceeb2912039241ea302f4bb8948d61f8
Gerrit-Change-Number: 19373
Gerrit-PatchSet: 3
Gerrit-Owner: Fang-Yu Rao
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Fang-Yu Rao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Smith
Gerrit-Reviewer: Quanlong Huang
Gerrit-Comment-Date: Wed, 21 Dec 2022 05:12:54 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10399, IMPALA-11060, IMPALA-11788: Reset Ranger policy repository in an E2E test
Hello Quanlong Huang, Csaba Ringhofer, Michael Smith, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/19373 to look at the new patch set (#3). Change subject: IMPALA-10399, IMPALA-11060, IMPALA-11788: Reset Ranger policy repository in an E2E test .. IMPALA-10399, IMPALA-11060, IMPALA-11788: Reset Ranger policy repository in an E2E test test_show_grant_hive_privilege() uses Ranger's REST API to get all the existing policies from the Ranger server after creating a policy that grants the LOCK and SELECT privileges on all the tables and columns in the unique database in order to verify the granted privileges indeed exist in Ranger's policy repository. Before this patch, the way we downloaded all the policies from the Ranger server in test_show_grant_hive_privilege(), however, did not always work. Specifically, when there were already a lot of existing policies in Ranger, the policy that granted the LOCK and SELECT privileges would not be included in the result returned via one single GET request. We found that to reproduce the issue it suffices to add 300 Ranger policies before adding the policy granting those 2 privileges. Moreover, we found that even we set the argument 'stream' of requests.get() to True and used iter_content() to read the response in chunks, we still could not retrieve the policy added in test_show_grant_hive_privilege(). As a workaround, this patch resets Ranger's policy repository for Impala before we create the policy granting those 2 privileges so that this test will be more resilient to the number of existing policies in the repository. Change-Id: Iff56ec03ceeb2912039241ea302f4bb8948d61f8 --- M testdata/bin/create-load-data.sh A testdata/bin/setup-ranger.sh M tests/authorization/test_ranger.py M tests/common/custom_cluster_test_suite.py 4 files changed, 116 insertions(+), 73 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/19373/3 -- To view, visit http://gerrit.cloudera.org:8080/19373 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iff56ec03ceeb2912039241ea302f4bb8948d61f8 Gerrit-Change-Number: 19373 Gerrit-PatchSet: 3 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Quanlong Huang
[Impala-ASF-CR] IMPALA-10399, IMPALA-11788: Reset Ranger policy repository in an E2E test
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/19373 ) Change subject: IMPALA-10399, IMPALA-11788: Reset Ranger policy repository in an E2E test .. Patch Set 2: Hi all, please let me know if you have any comment on the patch. It passed the dryrun tests according to https://jenkins.impala.io/job/gerrit-verify-dryrun/8927/. I just found that part of this patch also implements what we would like to do in IMPALA-11060. Hence I will update the subject of the patch in the next patch set. Thanks! -- To view, visit http://gerrit.cloudera.org:8080/19373 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iff56ec03ceeb2912039241ea302f4bb8948d61f8 Gerrit-Change-Number: 19373 Gerrit-PatchSet: 2 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Mon, 19 Dec 2022 01:55:17 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10399, IMPALA-11788: Reset Ranger policy repository in an E2E test
Fang-Yu Rao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19373 Change subject: IMPALA-10399, IMPALA-11788: Reset Ranger policy repository in an E2E test .. IMPALA-10399, IMPALA-11788: Reset Ranger policy repository in an E2E test test_show_grant_hive_privilege() uses Ranger's REST API to get all the existing policies from the Ranger server after creating a policy that grants the LOCK and SELECT privileges on all the tables and columns in the unique database in order to verify the granted privileges indeed exist in Ranger's policy repository. The way we downloaded all the policies from the Ranger server in test_show_grant_hive_privilege(), however, did not always work. Specifically, when there were already a lot of existing policies in Ranger, the policy that granted the LOCK and SELECT privileges would not be included in the result returned via one single GET request. We found that to reproduce the issue it suffices to add 300 Ranger policies before adding the policy granting those 2 privileges. Moreover, we found that even we set the argument 'stream' of requests.get() to True and used iter_content() to read the response in chunks, we still could not retrieve the policy added in test_show_grant_hive_privilege(). As a workaround, this patch resets Ranger's policy repository for Impala before we create the policy granting those 2 privileges so that this test will be more resilient to the number of existing policies in the repository. Change-Id: Iff56ec03ceeb2912039241ea302f4bb8948d61f8 --- M testdata/bin/create-load-data.sh A testdata/bin/setup-ranger.sh M tests/authorization/test_ranger.py 3 files changed, 111 insertions(+), 71 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/19373/2 -- To view, visit http://gerrit.cloudera.org:8080/19373 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Iff56ec03ceeb2912039241ea302f4bb8948d61f8 Gerrit-Change-Number: 19373 Gerrit-PatchSet: 2 Gerrit-Owner: Fang-Yu Rao
