You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Fang-Yu Rao (Code Review)" <ge...@cloudera.org> on 2020/04/08 19:43:39 UTC
[Impala-ASF-CR] IMPALA-9625: Convert fully-qualified table name to lowercase in getTable()
Fang-Yu Rao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15689
Change subject: IMPALA-9625: Convert fully-qualified table name to lowercase in getTable()
......................................................................
IMPALA-9625: Convert fully-qualified table name to lowercase in getTable()
Impala's COMPUTE STATS statement results in two registrations of the
ALTER event for the corresponding table identified by its
fully-qualified table name and a Set is used to maintained the audits.
The first registration is in Analyzer#registerAuthAndAuditEvent() and
the second is in Analyzer#getTable().
In registerAuthAndAuditEvent(), the corresponding full table name
table.getFullName() is produced by a call to Analyzer#resolveTableRef().
The resulting database and table names are both in lowercase.
However, in getTable(), the fully-qualified table name is produced by a
call to Analyzer#getFqTableName(). The resulting database and table
names are in their originally unconverted form provided by the user from
the Impala shell. Hence, there is no guarantee that the database and
table names are both in lowercase.
Therefore, if a user does not provide lowercase database and table
names, the returned full table names from registerAuthAndAuditEvent()
and getTable() would differ, resulting in duplicate ALTER events for the
same table. This patch fixes the inconsistencies by converting the
fully-qualified table name to lowercase in getTable().
Testing:
- Revised a FE test to verified we do not have duplicate ALTER events for
the COMPUTE STATS statement.
Change-Id: If0d9ba58da891921fafbfe7c6db358b51965e178
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
2 files changed, 15 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/15689/1
--
To view, visit http://gerrit.cloudera.org:8080/15689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If0d9ba58da891921fafbfe7c6db358b51965e178
Gerrit-Change-Number: 15689
Gerrit-PatchSet: 1
Gerrit-Owner: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
[Impala-ASF-CR] IMPALA-9625: Convert the name of a TAccessEvent to lowercase
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15689 )
Change subject: IMPALA-9625: Convert the name of a TAccessEvent to lowercase
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5644/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/15689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0d9ba58da891921fafbfe7c6db358b51965e178
Gerrit-Change-Number: 15689
Gerrit-PatchSet: 5
Gerrit-Owner: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Mon, 13 Apr 2020 08:26:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9625: Convert the name of a TAccessEvent to lowercase
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15689 )
Change subject: IMPALA-9625: Convert the name of a TAccessEvent to lowercase
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5776/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0d9ba58da891921fafbfe7c6db358b51965e178
Gerrit-Change-Number: 15689
Gerrit-PatchSet: 4
Gerrit-Owner: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Apr 2020 22:05:11 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9625: Convert the name of a TAccessEvent to lowercase
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15689 )
Change subject: IMPALA-9625: Convert the name of a TAccessEvent to lowercase
......................................................................
Patch Set 4: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/5638/
--
To view, visit http://gerrit.cloudera.org:8080/15689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0d9ba58da891921fafbfe7c6db358b51965e178
Gerrit-Change-Number: 15689
Gerrit-PatchSet: 4
Gerrit-Owner: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 Apr 2020 19:31:28 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9625: Convert the name of a TAccessEvent to lowercase
Posted by "Fang-Yu Rao (Code Review)" <ge...@cloudera.org>.
Fang-Yu Rao has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/15689 )
Change subject: IMPALA-9625: Convert the name of a TAccessEvent to lowercase
......................................................................
IMPALA-9625: Convert the name of a TAccessEvent to lowercase
Impala's COMPUTE STATS statement results in two registrations of the
ALTER event for the corresponding table identified by its
fully-qualified table name and a Set is used to maintained the audits.
The first registration is in Analyzer#registerAuthAndAuditEvent() and
the second is in Analyzer#getTable().
In registerAuthAndAuditEvent(), the corresponding full table name
table.getFullName() is produced by a call to Analyzer#resolveTableRef().
The resulting database and table names are both in lowercase.
However, in getTable(), the fully-qualified table name is produced by a
call to Analyzer#getFqTableName(). The resulting database and table
names are in their originally unconverted form provided by the user from
the Impala shell. Hence, there is no guarantee that the database and
table names are both in lowercase.
Therefore, if a user does not provide lowercase database and table
names, the returned full table names from registerAuthAndAuditEvent()
and getTable() would differ, resulting in duplicate ALTER events for the
same table. This patch resolves the inconsistencies by converting the
name of a TAccessEvent to lowercase in addAccessEvent(), which is called
in getTable().
Testing:
- Revised a FE test to verified we do not have duplicate ALTER events for
the COMPUTE STATS statement.
- Verified that the patch passes the exhaustive tests in the DEBUG build
except for a flaky E2E test of test_column_storage_attributes.
Change-Id: If0d9ba58da891921fafbfe7c6db358b51965e178
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
2 files changed, 18 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/15689/4
--
To view, visit http://gerrit.cloudera.org:8080/15689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If0d9ba58da891921fafbfe7c6db358b51965e178
Gerrit-Change-Number: 15689
Gerrit-PatchSet: 4
Gerrit-Owner: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
[Impala-ASF-CR] IMPALA-9625: Convert the name of a TAccessEvent to lowercase
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15689 )
Change subject: IMPALA-9625: Convert the name of a TAccessEvent to lowercase
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/15689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0d9ba58da891921fafbfe7c6db358b51965e178
Gerrit-Change-Number: 15689
Gerrit-PatchSet: 5
Gerrit-Owner: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Mon, 13 Apr 2020 12:57:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9625: Convert the name of a TAccessEvent to lowercase
Posted by "Fang-Yu Rao (Code Review)" <ge...@cloudera.org>.
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15689 )
Change subject: IMPALA-9625: Convert the name of a TAccessEvent to lowercase
......................................................................
Patch Set 4:
Thanks Norbert for giving a +2!
--
To view, visit http://gerrit.cloudera.org:8080/15689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0d9ba58da891921fafbfe7c6db358b51965e178
Gerrit-Change-Number: 15689
Gerrit-PatchSet: 4
Gerrit-Owner: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 Apr 2020 15:48:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9625: Convert the name of a TAccessEvent to lowercase
Posted by "Fang-Yu Rao (Code Review)" <ge...@cloudera.org>.
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15689 )
Change subject: IMPALA-9625: Convert the name of a TAccessEvent to lowercase
......................................................................
Patch Set 6:
> Patch Set 6:
>
> Hi Fang-Yu, thanks for restarting the verification build, I see this time it passed.
Hi Norbert, not a problem! I think it is Quanlong who helped us with restarting the build. :-)
--
To view, visit http://gerrit.cloudera.org:8080/15689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0d9ba58da891921fafbfe7c6db358b51965e178
Gerrit-Change-Number: 15689
Gerrit-PatchSet: 6
Gerrit-Owner: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 Apr 2020 17:20:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9625: Convert the name of a TAccessEvent to lowercase
Posted by "Fang-Yu Rao (Code Review)" <ge...@cloudera.org>.
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15689 )
Change subject: IMPALA-9625: Convert the name of a TAccessEvent to lowercase
......................................................................
Patch Set 4:
Hi Norbert, it seems we hit https://issues.apache.org/jira/browse/IMPALA-9596 (https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/10189/testReport/junit/query_test.test_nested_types/TestNestedTypesNoMtDop/test_tpch_mem_limit_single_node_protocol__beeswax___exec_option____batch_size___0___num_nodes___0___disable_codegen_rows_threshold___0___disable_codegen___False___abort_on_error___1___exec_single_node_rows_threshold___0____table_format__orc_def_block_/). This may not be related to our patch here.
--
To view, visit http://gerrit.cloudera.org:8080/15689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0d9ba58da891921fafbfe7c6db358b51965e178
Gerrit-Change-Number: 15689
Gerrit-PatchSet: 4
Gerrit-Owner: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 Apr 2020 23:41:13 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9625: Convert the name of a TAccessEvent to lowercase
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15689 )
Change subject: IMPALA-9625: Convert the name of a TAccessEvent to lowercase
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0d9ba58da891921fafbfe7c6db358b51965e178
Gerrit-Change-Number: 15689
Gerrit-PatchSet: 5
Gerrit-Owner: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Mon, 13 Apr 2020 08:26:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9625: Convert fully-qualified table name to lowercase in getTable()
Posted by "Fang-Yu Rao (Code Review)" <ge...@cloudera.org>.
Fang-Yu Rao has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/15689 )
Change subject: IMPALA-9625: Convert fully-qualified table name to lowercase in getTable()
......................................................................
IMPALA-9625: Convert fully-qualified table name to lowercase in getTable()
Impala's COMPUTE STATS statement results in two registrations of the
ALTER event for the corresponding table identified by its
fully-qualified table name and a Set is used to maintained the audits.
The first registration is in Analyzer#registerAuthAndAuditEvent() and
the second is in Analyzer#getTable().
In registerAuthAndAuditEvent(), the corresponding full table name
table.getFullName() is produced by a call to Analyzer#resolveTableRef().
The resulting database and table names are both in lowercase.
However, in getTable(), the fully-qualified table name is produced by a
call to Analyzer#getFqTableName(). The resulting database and table
names are in their originally unconverted form provided by the user from
the Impala shell. Hence, there is no guarantee that the database and
table names are both in lowercase.
Therefore, if a user does not provide lowercase database and table
names, the returned full table names from registerAuthAndAuditEvent()
and getTable() would differ, resulting in duplicate ALTER events for the
same table. This patch resolves the inconsistencies by converting the
fully-qualified table name to lowercase in getTable().
Testing:
- Revised a FE test to verified we do not have duplicate ALTER events for
the COMPUTE STATS statement.
- Verified that the patch passes the exhaustive tests in the DEBUG build
except for a flaky E2E test of test_column_storage_attributes.
Change-Id: If0d9ba58da891921fafbfe7c6db358b51965e178
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
2 files changed, 18 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/15689/2
--
To view, visit http://gerrit.cloudera.org:8080/15689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If0d9ba58da891921fafbfe7c6db358b51965e178
Gerrit-Change-Number: 15689
Gerrit-PatchSet: 2
Gerrit-Owner: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
[Impala-ASF-CR] IMPALA-9625: Convert the name of a TAccessEvent to lowercase
Posted by "Norbert Luksa (Code Review)" <ge...@cloudera.org>.
Norbert Luksa has posted comments on this change. ( http://gerrit.cloudera.org:8080/15689 )
Change subject: IMPALA-9625: Convert the name of a TAccessEvent to lowercase
......................................................................
Patch Set 6:
Hi Fang-Yu, thanks for restarting the verification build, I see this time it passed.
--
To view, visit http://gerrit.cloudera.org:8080/15689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0d9ba58da891921fafbfe7c6db358b51965e178
Gerrit-Change-Number: 15689
Gerrit-PatchSet: 6
Gerrit-Owner: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 Apr 2020 07:44:25 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9625: Convert the name of a TAccessEvent to lowercase
Posted by "Norbert Luksa (Code Review)" <ge...@cloudera.org>.
Norbert Luksa has posted comments on this change. ( http://gerrit.cloudera.org:8080/15689 )
Change subject: IMPALA-9625: Convert the name of a TAccessEvent to lowercase
......................................................................
Patch Set 4: Code-Review+2
(1 comment)
Thanks Fang-Yu, since the exhaustive tests passed, I think it is good to go in now.
http://gerrit.cloudera.org:8080/#/c/15689/1/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/15689/1/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2758
PS1, Line 2758: public void addAccessEvent(TAccessEvent event) {
> Hi Norbert, I just realized that simply adding "Preconditions.checkState(ev
Hi Fang-Yu, I agree, this looks like the safer option.
--
To view, visit http://gerrit.cloudera.org:8080/15689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0d9ba58da891921fafbfe7c6db358b51965e178
Gerrit-Change-Number: 15689
Gerrit-PatchSet: 4
Gerrit-Owner: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 Apr 2020 14:52:24 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9625: Convert the name of a TAccessEvent to lowercase
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15689 )
Change subject: IMPALA-9625: Convert the name of a TAccessEvent to lowercase
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5638/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/15689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0d9ba58da891921fafbfe7c6db358b51965e178
Gerrit-Change-Number: 15689
Gerrit-PatchSet: 4
Gerrit-Owner: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 Apr 2020 14:55:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9625: Convert fully-qualified table name to lowercase in getTable()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15689 )
Change subject: IMPALA-9625: Convert fully-qualified table name to lowercase in getTable()
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5772/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0d9ba58da891921fafbfe7c6db358b51965e178
Gerrit-Change-Number: 15689
Gerrit-PatchSet: 2
Gerrit-Owner: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Apr 2020 17:33:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9625: Convert fully-qualified table name to lowercase in getTable()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15689 )
Change subject: IMPALA-9625: Convert fully-qualified table name to lowercase in getTable()
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5765/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0d9ba58da891921fafbfe7c6db358b51965e178
Gerrit-Change-Number: 15689
Gerrit-PatchSet: 1
Gerrit-Owner: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Apr 2020 20:22:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9625: Convert fully-qualified table name to lowercase in getTable()
Posted by "Norbert Luksa (Code Review)" <ge...@cloudera.org>.
Norbert Luksa has posted comments on this change. ( http://gerrit.cloudera.org:8080/15689 )
Change subject: IMPALA-9625: Convert fully-qualified table name to lowercase in getTable()
......................................................................
Patch Set 1: Code-Review+1
(2 comments)
Hi Fang-Yu, left some comments, otherwise LGTM.
http://gerrit.cloudera.org:8080/#/c/15689/1/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/15689/1/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2758
PS1, Line 2758: public void addAccessEvent(TAccessEvent event) {
I think it would be safer to check if the name is lower case here with a precondition.
Another option is to convert the name to lower case here.
http://gerrit.cloudera.org:8080/#/c/15689/1/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2949
PS1, Line 2949: globalState_.accessEvents.add(new TAccessEvent(
Here we should also use the addAccessEvent method.
--
To view, visit http://gerrit.cloudera.org:8080/15689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0d9ba58da891921fafbfe7c6db358b51965e178
Gerrit-Change-Number: 15689
Gerrit-PatchSet: 1
Gerrit-Owner: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Apr 2020 08:53:47 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9625: Convert the name of a TAccessEvent to lowercase
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15689 )
Change subject: IMPALA-9625: Convert the name of a TAccessEvent to lowercase
......................................................................
IMPALA-9625: Convert the name of a TAccessEvent to lowercase
Impala's COMPUTE STATS statement results in two registrations of the
ALTER event for the corresponding table identified by its
fully-qualified table name and a Set is used to maintained the audits.
The first registration is in Analyzer#registerAuthAndAuditEvent() and
the second is in Analyzer#getTable().
In registerAuthAndAuditEvent(), the corresponding full table name
table.getFullName() is produced by a call to Analyzer#resolveTableRef().
The resulting database and table names are both in lowercase.
However, in getTable(), the fully-qualified table name is produced by a
call to Analyzer#getFqTableName(). The resulting database and table
names are in their originally unconverted form provided by the user from
the Impala shell. Hence, there is no guarantee that the database and
table names are both in lowercase.
Therefore, if a user does not provide lowercase database and table
names, the returned full table names from registerAuthAndAuditEvent()
and getTable() would differ, resulting in duplicate ALTER events for the
same table. This patch resolves the inconsistencies by converting the
name of a TAccessEvent to lowercase in addAccessEvent(), which is called
in getTable().
Testing:
- Revised a FE test to verified we do not have duplicate ALTER events for
the COMPUTE STATS statement.
- Verified that the patch passes the exhaustive tests in the DEBUG build
except for a flaky E2E test of test_column_storage_attributes.
Change-Id: If0d9ba58da891921fafbfe7c6db358b51965e178
Reviewed-on: http://gerrit.cloudera.org:8080/15689
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
2 files changed, 18 insertions(+), 4 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/15689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: If0d9ba58da891921fafbfe7c6db358b51965e178
Gerrit-Change-Number: 15689
Gerrit-PatchSet: 6
Gerrit-Owner: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>