You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "liuyao (Code Review)" <ge...@cloudera.org> on 2021/06/19 11:24:23 UTC
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
liuyao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17604
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
IMPALA-9763: builtin functions throw unknown exception
This problem is caused by shallow copies. In the function
FromClause::analyze(Analyzer analyzer), if the table is FeView,
the analyzer will clone SelectStmt from the view's MetaData,
which should be a deep copy. But in the copy constructor of
FunctionCallExpr, fnName_ = other.fnName_;, this is a shallow copy,
which makes the fnName_ of multiple queries refer to the same block of
memory. When the query is analyzed, the database of fnName_ will be set.
But this will change the fnName_ of other queries.
For example:
create view1 as select coalesce(col1, 2) from table1;
User A and User B check the same view1 at the same time. When
user A’s query is analyzed, it runs to db_ = analyzer.getDefaultDb() of
FunctionName::analyze. User B’s query is also analyzed and runs to
FunctionCallExpr::analyzeImpl. And just completed FunctionName::analyze;
at this time, the step of user A's db_ = analyzer.getDefaultDb() will
change the db_ attribute of user B's fnName_ to defultdb, and then
user B will continue to check whether the function is pure time ,
Throw an exception: coalesce() unknown for database .......
Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
---
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/17604/1
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 1
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 11: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 11
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Thu, 24 Jun 2021 15:18:01 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7243/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 4
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Tue, 22 Jun 2021 03:47:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
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/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
IMPALA-9763: builtin functions throw unknown exception
There is a read-write contention in FunctionName#analyze()
when a FunctionName object is shared to two threads: check if there
is a builtin function in wrong database, when 'db_' is temporarily
modified by the other thread.
This patch fix this issue. For analyze FunctionName, if no database is
specified, check builtinDb first; if FunctionName is not in builtinDb,
set the database to defaultDb.Avoid setting the database of
FunctionName to a temporary value
Detailed description:
This problem is caused by shallow copies. In the function
FromClause::analyze(Analyzer analyzer), if the table is FeView,
the analyzer will clone SelectStmt from the view's MetaData,
which should be a deep copy. But in the copy constructor of
FunctionCallExpr, fnName_ = other.fnName_, this is a shallow copy,
which makes the fnName_ of multiple queries refer to the same block of
memory. When the query is analyzed, the database of fnName_ will be
set.But this will change the fnName_ of other queries.
For example:
create view1 as select coalesce(col1, 2) from table1;
User A and User B check the same view1 at the same time. When
user A’s query is analyzed, it runs to db_ = analyzer.getDefaultDb() of
FunctionName::analyze. User B’s query is also analyzed and runs to
FunctionCallExpr::analyzeImpl, and just completed FunctionName::analyze
at this time, the step of user A's db_ = analyzer.getDefaultDb() will
change the db_ attribute of user B's fnName_ to defaultDb, and then
user B will continue to check whether the function is defaultDb,
Throw an exception: coalesce() unknown for database .......
Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Reviewed-on: http://gerrit.cloudera.org:8080/17604
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/FunctionName.java
1 file changed, 2 insertions(+), 1 deletion(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 12
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "liuyao (Code Review)" <ge...@cloudera.org>.
Hello Quanlong Huang, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17604
to look at the new patch set (#7).
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
IMPALA-9763: builtin functions throw unknown exception
There is a read-write contention in FunctionName#analyze()
when a FunctionName object is shared to two threads: check if there
is a builtin function in wrong database, when 'db_' is temporarily
modified by the other thread.
This problem is caused by shallow copies. In the function
FromClause::analyze(Analyzer analyzer), if the table is FeView,
the analyzer will clone SelectStmt from the view's MetaData,
which should be a deep copy. But in the copy constructor of
FunctionCallExpr, fnName_ = other.fnName_, this is a shallow copy,
which makes the fnName_ of multiple queries refer to the same block of
memory. When the query is analyzed, the database of fnName_ will be set.
But this will change the fnName_ of other queries.
For example:
create view1 as select coalesce(col1, 2) from table1;
User A and User B check the same view1 at the same time. When
user A’s query is analyzed, it runs to db_ = analyzer.getDefaultDb() of
FunctionName::analyze. User B’s query is also analyzed and runs to
FunctionCallExpr::analyzeImpl, and just completed FunctionName::analyze;
at this time, the step of user A's db_ = analyzer.getDefaultDb() will
change the db_ attribute of user B's fnName_ to defaultDb, and then
user B will continue to check whether the function is defaultDb,
Throw an exception: coalesce() unknown for database .......
Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
---
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionName.java
2 files changed, 3 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/17604/7
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 7
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 10: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 10
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Thu, 24 Jun 2021 13:47:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 11: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 11
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Thu, 24 Jun 2021 09:13:39 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/8954/ : 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/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 3
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Sun, 20 Jun 2021 03:09:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 10: Code-Review+2
Carrying Aman's +1 to +2. Nice work on debugging! Thank Liu Yao!
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 10
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Thu, 24 Jun 2021 09:10:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 2: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/7237/
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 2
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Sat, 19 Jun 2021 17:18:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "liuyao (Code Review)" <ge...@cloudera.org>.
liuyao has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17604/5/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
File fe/src/main/java/org/apache/impala/analysis/QueryStmt.java:
http://gerrit.cloudera.org:8080/#/c/17604/5/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@274
PS5, Line 274: orderByElement.getExpr().collectAll(
: Predicates.instanceOf(FunctionCallExpr.class), fnExprs);
: }
: substituteOrdinalsAndAliases(orderingExprs, "ORDER BY", analyzer);
:
: for (FunctionCallExpr fn: fnExprs) fn.getFnName().analyze(analyzer);
> I'm not clear about this. You are collecting fnExprs from the
> original 'orderByElements_' but not the cloned ones inside
> 'orderingExprs'. And then analyze these original exprs.
>
> Do we require all FunctionName objects being analyzed now? Just
> concerning that it will introduce a new requirement that may be
> broken in many other places.
Previously, the code here relied on shallow copy. When executing substituteOrdinalsAndAliases(orderingExprs, "ORDER BY", analyzer), the FunctionName in orderingExprs was analyzed and the FunctionName in orderByElements_ was also analyzed.
After changing to a deep copy, if the FunctionName in orderByElements_ is not analyzed here, then NormalizeCountStarRule#apply will report an exception: NullPointorException, because the FunctionName in orderByElements_ has not been analyzed, origExpr.getFnName().getFunction() returns Is null(line 41).
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 6
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Wed, 23 Jun 2021 09:37:55 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7250/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 5
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Wed, 23 Jun 2021 02:30:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "liuyao (Code Review)" <ge...@cloudera.org>.
Hello Quanlong Huang, Aman Sinha, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17604
to look at the new patch set (#10).
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
IMPALA-9763: builtin functions throw unknown exception
There is a read-write contention in FunctionName#analyze()
when a FunctionName object is shared to two threads: check if there
is a builtin function in wrong database, when 'db_' is temporarily
modified by the other thread.
This patch fix this issue. For analyze FunctionName, if no database is
specified, check builtinDb first; if FunctionName is not in builtinDb,
set the database to defaultDb.Avoid setting the database of
FunctionName to a temporary value
Detailed description:
This problem is caused by shallow copies. In the function
FromClause::analyze(Analyzer analyzer), if the table is FeView,
the analyzer will clone SelectStmt from the view's MetaData,
which should be a deep copy. But in the copy constructor of
FunctionCallExpr, fnName_ = other.fnName_, this is a shallow copy,
which makes the fnName_ of multiple queries refer to the same block of
memory. When the query is analyzed, the database of fnName_ will be
set.But this will change the fnName_ of other queries.
For example:
create view1 as select coalesce(col1, 2) from table1;
User A and User B check the same view1 at the same time. When
user A’s query is analyzed, it runs to db_ = analyzer.getDefaultDb() of
FunctionName::analyze. User B’s query is also analyzed and runs to
FunctionCallExpr::analyzeImpl, and just completed FunctionName::analyze
at this time, the step of user A's db_ = analyzer.getDefaultDb() will
change the db_ attribute of user B's fnName_ to defaultDb, and then
user B will continue to check whether the function is defaultDb,
Throw an exception: coalesce() unknown for database .......
Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
---
M fe/src/main/java/org/apache/impala/analysis/FunctionName.java
1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/17604/10
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 10
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 9: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 9
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Thu, 24 Jun 2021 12:15:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7245/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 5
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Tue, 22 Jun 2021 07:56:01 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 11:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7262/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 11
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Thu, 24 Jun 2021 09:13:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 5:
(3 comments)
> Patch Set 5:
>
> Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/7250/
The failures are inrelated. Rebase may help to fix the test failure.
http://gerrit.cloudera.org:8080/#/c/17604/3/fe/src/main/java/org/apache/impala/analysis/FunctionName.java
File fe/src/main/java/org/apache/impala/analysis/FunctionName.java:
http://gerrit.cloudera.org:8080/#/c/17604/3/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@64
PS3, Line 64: fnNamePath_ = (other.getFnNamePath() != null) ?
> > Sorry, I'm still not clear why we need to clone a field that is
I mean we can just use shallow copy, i.e. "fnNamePath_ = other.getFnNamePath()", because it's private and never modified.
Nevermind, it's ok in either ways.
http://gerrit.cloudera.org:8080/#/c/17604/3/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@66
PS3, Line 66: db_ = other.getDb();
: fn_ = other.getFunction();
: isBuiltin_ = other.isBuiltin();
: isAnalyzed_ = other.isFnAnalyzed();
> > > I tried to do this, but an error occurred. Because
OK, so it seems risky to call reset() since we have inconsistent usages on the clone-reset-analyze pattern. This patch reveals that we do have places that don't follow this pattern. Thanks for the explanation!
http://gerrit.cloudera.org:8080/#/c/17604/5/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
File fe/src/main/java/org/apache/impala/analysis/QueryStmt.java:
http://gerrit.cloudera.org:8080/#/c/17604/5/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@274
PS5, Line 274: orderByElement.getExpr().collectAll(
: Predicates.instanceOf(FunctionCallExpr.class), fnExprs);
: }
: substituteOrdinalsAndAliases(orderingExprs, "ORDER BY", analyzer);
:
: for (FunctionCallExpr fn: fnExprs) fn.getFnName().analyze(analyzer);
I'm not clear about this. You are collecting fnExprs from the original 'orderByElements_' but not the cloned ones inside 'orderingExprs'. And then analyze these original exprs.
Do we require all FunctionName objects being analyzed now? Just concerning that it will introduce a new requirement that may be broken in many other places.
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 5
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Wed, 23 Jun 2021 08:15:56 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17604/6/fe/src/main/java/org/apache/impala/analysis/FunctionName.java
File fe/src/main/java/org/apache/impala/analysis/FunctionName.java:
http://gerrit.cloudera.org:8080/#/c/17604/6/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@144
PS6, Line 144: if (!isFullyQualified()) {
: db_ = analyzer.getDefaultDb();
: if (preferBuiltinsDb && builtinDb.containsFunction(fn_)) {
: db_ = BuiltinsDb.NAME;
: }
: }
: Preconditions.checkNotNull(db_);
: isBuiltin_ = db_.equals(BuiltinsDb.NAME) &&
: builtinDb.containsFunction(fn_);
I think a simpler solution is resolving the read-write contention here, i.e. making each fields being assigned only once.
diff --git a/fe/src/main/java/org/apache/impala/analysis/FunctionName.java b/fe/src/main/java/org/apache/impala/analysis/FunctionName.java
index f3b3d5d96..288f7cb6e 100644
--- a/fe/src/main/java/org/apache/impala/analysis/FunctionName.java
+++ b/fe/src/main/java/org/apache/impala/analysis/FunctionName.java
@@ -121,9 +121,10 @@ public class FunctionName {
// Resolve the database for this function.
Db builtinDb = BuiltinsDb.getInstance();
if (!isFullyQualified()) {
- db_ = analyzer.getDefaultDb();
if (preferBuiltinsDb && builtinDb.containsFunction(fn_)) {
db_ = BuiltinsDb.NAME;
+ } else {
+ db_ = analyzer.getDefaultDb();
}
}
Preconditions.checkNotNull(db_);
So we won't get ephemeral value on db_. This allows the current shallow copy pattern and doesn't require changing other codes. What do you think?
http://gerrit.cloudera.org:8080/#/c/17604/5/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
File fe/src/main/java/org/apache/impala/analysis/QueryStmt.java:
http://gerrit.cloudera.org:8080/#/c/17604/5/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@274
PS5, Line 274: orderByElement.getExpr().collectAll(
: Predicates.instanceOf(FunctionCallExpr.class), fnExprs);
: }
: substituteOrdinalsAndAliases(orderingExprs, "ORDER BY", analyzer);
:
: for (FunctionCallExpr fn: fnExprs) fn.getFnName().analyze(analyzer);
> > I'm not clear about this. You are collecting fnExprs from the
I see. So we do need to take care of existing usage pattern on FunctionName. Not sure if we have other places like this that are not revealed by tests.
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 6
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Wed, 23 Jun 2021 11:14:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 8:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/8993/ : 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/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 8
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Thu, 24 Jun 2021 06:32:30 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "liuyao (Code Review)" <ge...@cloudera.org>.
Hello Quanlong Huang, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17604
to look at the new patch set (#8).
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
IMPALA-9763: builtin functions throw unknown exception
There is a read-write contention in FunctionName#analyze()
when a FunctionName object is shared to two threads: check if there
is a builtin function in wrong database, when 'db_' is temporarily
modified by the other thread.
This problem is caused by shallow copies. In the function
FromClause::analyze(Analyzer analyzer), if the table is FeView,
the analyzer will clone SelectStmt from the view's MetaData,
which should be a deep copy. But in the copy constructor of
FunctionCallExpr, fnName_ = other.fnName_, this is a shallow copy,
which makes the fnName_ of multiple queries refer to the same block of
memory. When the query is analyzed, the database of fnName_ will be set.
But this will change the fnName_ of other queries.
For example:
create view1 as select coalesce(col1, 2) from table1;
User A and User B check the same view1 at the same time. When
user A’s query is analyzed, it runs to db_ = analyzer.getDefaultDb() of
FunctionName::analyze. User B’s query is also analyzed and runs to
FunctionCallExpr::analyzeImpl, and just completed FunctionName::analyze;
at this time, the step of user A's db_ = analyzer.getDefaultDb() will
change the db_ attribute of user B's fnName_ to defaultDb, and then
user B will continue to check whether the function is defaultDb,
Throw an exception: coalesce() unknown for database .......
Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
---
M fe/src/main/java/org/apache/impala/analysis/FunctionName.java
1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/17604/8
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 8
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "liuyao (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17604
to look at the new patch set (#2).
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
IMPALA-9763: builtin functions throw unknown exception
This problem is caused by shallow copies. In the function
FromClause::analyze(Analyzer analyzer), if the table is FeView,
the analyzer will clone SelectStmt from the view's MetaData,
which should be a deep copy. But in the copy constructor of
FunctionCallExpr, fnName_ = other.fnName_;, this is a shallow copy,
which makes the fnName_ of multiple queries refer to the same block of
memory. When the query is analyzed, the database of fnName_ will be set.
But this will change the fnName_ of other queries.
For example:
create view1 as select coalesce(col1, 2) from table1;
User A and User B check the same view1 at the same time. When
user A’s query is analyzed, it runs to db_ = analyzer.getDefaultDb() of
FunctionName::analyze. User B’s query is also analyzed and runs to
FunctionCallExpr::analyzeImpl. And just completed FunctionName::analyze;
at this time, the step of user A's db_ = analyzer.getDefaultDb() will
change the db_ attribute of user B's fnName_ to defultdb, and then
user B will continue to check whether the function is pure time ,
Throw an exception: coalesce() unknown for database .......
Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
---
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionName.java
2 files changed, 14 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/17604/2
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 2
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/17604/4/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java:
http://gerrit.cloudera.org:8080/#/c/17604/4/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@173
PS4, Line 173: fnName_ = other.fnName_.clone();
nit: Could you add a Preconditions check that other.fnName_ != null?
http://gerrit.cloudera.org:8080/#/c/17604/3/fe/src/main/java/org/apache/impala/analysis/FunctionName.java
File fe/src/main/java/org/apache/impala/analysis/FunctionName.java:
http://gerrit.cloudera.org:8080/#/c/17604/3/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@64
PS3, Line 64: fnNamePath_ = (other.getFnNamePath() != null) ?
> I removed the "shallow copy" in the copy constructor of FunctionCallExpr, s
Sorry, I'm still not clear why we need to clone a field that is never modified... Did I miss anything?
http://gerrit.cloudera.org:8080/#/c/17604/3/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@66
PS3, Line 66: db_ = other.getDb();
: fn_ = other.getFunction();
: isBuiltin_ = other.isBuiltin();
: isAnalyzed_ = other.isFnAnalyzed();
> I tried to do this, but an error occurred. Because FunctionName::fnNamePath_ is null.
>
>use functional;
>Select count(1) from alltypesagg;
>
>I0621 19:03:29.787499 46677 jni-util.cc:286] 0649db5f6bc675d6:6b92f3b600000000] java.lang.NullPointerException
>at org.apache.impala.analysis.FunctionCallExpr.isConstantImpl(FunctionCallExpr.java:372)
>at org.apache.impala.analysis.Expr.isConstant(Expr.java:1460)
>at org.apache.impala.analysis.Expr.computeNumDistinctValues(Expr.java:575)
>at org.apache.impala.analysis.Expr.analyze(Expr.java:500)
>......
hmm, did you reset fnNamePath_ to null so it cause the NullPointerException? In reset() we only reset the analysis results which are db_, fn_, isBuiltin_ and isAnalyzed_. The fnNamePath_ comes from the parser so should not be reset.
> I modified the copy constructor of FunctionCallExpr again. I clone QueryStmt in the constructor of InlineViewRef. At this time, db_ and fn_ are both null, and isBuiltin_ and isAnalyzed_ are both false, because only fnNamePath_ is stored in the stmtTableCache, and no other values are stored.
OK, this answers my question that cloning the analysis results are safe, because the source objects are in the StmtTableCache and they are never analyzed. All code paths go here will see 'other' has null db_ , fn_ etc.
However, this depends on the caller implementation far away from this class. I think adding a reset() method to reset db_, fn_, isBuiltin_ and isAnalyzed_ help to prevent future bugs.
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 4
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Mon, 21 Jun 2021 12:46:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "liuyao (Code Review)" <ge...@cloudera.org>.
Hello Quanlong Huang, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17604
to look at the new patch set (#4).
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
IMPALA-9763: builtin functions throw unknown exception
There is a read-write contention in FunctionName#analyze()
when a FunctionName object is shared to two threads: check if there
is a builtin function in wrong database, when 'db_' is temporarily
modified by the other thread.
This problem is caused by shallow copies. In the function
FromClause::analyze(Analyzer analyzer), if the table is FeView,
the analyzer will clone SelectStmt from the view's MetaData,
which should be a deep copy. But in the copy constructor of
FunctionCallExpr, fnName_ = other.fnName_, this is a shallow copy,
which makes the fnName_ of multiple queries refer to the same block of
memory. When the query is analyzed, the database of fnName_ will be set.
But this will change the fnName_ of other queries.
For example:
create view1 as select coalesce(col1, 2) from table1;
User A and User B check the same view1 at the same time. When
user A’s query is analyzed, it runs to db_ = analyzer.getDefaultDb() of
FunctionName::analyze. User B’s query is also analyzed and runs to
FunctionCallExpr::analyzeImpl, and just completed FunctionName::analyze;
at this time, the step of user A's db_ = analyzer.getDefaultDb() will
change the db_ attribute of user B's fnName_ to defaultDb, and then
user B will continue to check whether the function is defaultDb,
Throw an exception: coalesce() unknown for database .......
Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
---
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionName.java
M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
3 files changed, 20 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/17604/4
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 4
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/8955/ : 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/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 4
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Mon, 21 Jun 2021 12:15:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7241/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 4
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Mon, 21 Jun 2021 11:54:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/8967/ : 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/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 5
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Tue, 22 Jun 2021 08:04:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 5:
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/7250/
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 5
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Wed, 23 Jun 2021 06:58:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 9:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7260/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 9
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Thu, 24 Jun 2021 06:12:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/8953/ : 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/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 2
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Sat, 19 Jun 2021 11:48:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 1:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/8952/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 1
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Sat, 19 Jun 2021 11:35:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "liuyao (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17604
to look at the new patch set (#3).
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
IMPALA-9763: builtin functions throw unknown exception
This problem is caused by shallow copies. In the function
FromClause::analyze(Analyzer analyzer), if the table is FeView,
the analyzer will clone SelectStmt from the view's MetaData,
which should be a deep copy. But in the copy constructor of
FunctionCallExpr, fnName_ = other.fnName_;, this is a shallow copy,
which makes the fnName_ of multiple queries refer to the same block of
memory. When the query is analyzed, the database of fnName_ will be set.
But this will change the fnName_ of other queries.
For example:
create view1 as select coalesce(col1, 2) from table1;
User A and User B check the same view1 at the same time. When
user A’s query is analyzed, it runs to db_ = analyzer.getDefaultDb() of
FunctionName::analyze. User B’s query is also analyzed and runs to
FunctionCallExpr::analyzeImpl. And just completed FunctionName::analyze;
at this time, the step of user A's db_ = analyzer.getDefaultDb() will
change the db_ attribute of user B's fnName_ to defultdb, and then
user B will continue to check whether the function is pure time ,
Throw an exception: coalesce() unknown for database .......
Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
---
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionName.java
2 files changed, 18 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/17604/3
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 3
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 2:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7237/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 2
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Sat, 19 Jun 2021 11:26:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 6: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/7255/
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 6
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Wed, 23 Jun 2021 18:48:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 5: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/7245/
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 5
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Tue, 22 Jun 2021 13:49:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 4: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 4
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Tue, 22 Jun 2021 09:38:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Aman Sinha (Code Review)" <ge...@cloudera.org>.
Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 9: Code-Review+1
Thanks liuyao for working on this. The simpler change suggested by Quanlong makes sense to me. My only other comment is to update the commit message to describe the final fix (currently, it describes the problem but not the fix).
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 9
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Thu, 24 Jun 2021 06:52:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7238/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 3
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Sun, 20 Jun 2021 02:49:55 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "liuyao (Code Review)" <ge...@cloudera.org>.
Hello Quanlong Huang, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17604
to look at the new patch set (#5).
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
IMPALA-9763: builtin functions throw unknown exception
There is a read-write contention in FunctionName#analyze()
when a FunctionName object is shared to two threads: check if there
is a builtin function in wrong database, when 'db_' is temporarily
modified by the other thread.
This problem is caused by shallow copies. In the function
FromClause::analyze(Analyzer analyzer), if the table is FeView,
the analyzer will clone SelectStmt from the view's MetaData,
which should be a deep copy. But in the copy constructor of
FunctionCallExpr, fnName_ = other.fnName_, this is a shallow copy,
which makes the fnName_ of multiple queries refer to the same block of
memory. When the query is analyzed, the database of fnName_ will be set.
But this will change the fnName_ of other queries.
For example:
create view1 as select coalesce(col1, 2) from table1;
User A and User B check the same view1 at the same time. When
user A’s query is analyzed, it runs to db_ = analyzer.getDefaultDb() of
FunctionName::analyze. User B’s query is also analyzed and runs to
FunctionCallExpr::analyzeImpl, and just completed FunctionName::analyze;
at this time, the step of user A's db_ = analyzer.getDefaultDb() will
change the db_ attribute of user B's fnName_ to defaultDb, and then
user B will continue to check whether the function is defaultDb,
Throw an exception: coalesce() unknown for database .......
Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
---
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionName.java
M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
3 files changed, 28 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/17604/5
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 5
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 7:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/8992/ : 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/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 7
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Thu, 24 Jun 2021 06:31:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 10:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7261/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 10
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Thu, 24 Jun 2021 07:41:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "liuyao (Code Review)" <ge...@cloudera.org>.
liuyao has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/17604/4/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java:
http://gerrit.cloudera.org:8080/#/c/17604/4/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@173
PS4, Line 173: Preconditions.checkState(other.fnName_ != null);
> nit: Could you add a Preconditions check that other.fnName_ != null?
Done
http://gerrit.cloudera.org:8080/#/c/17604/3/fe/src/main/java/org/apache/impala/analysis/FunctionName.java
File fe/src/main/java/org/apache/impala/analysis/FunctionName.java:
http://gerrit.cloudera.org:8080/#/c/17604/3/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@64
PS3, Line 64: fnNamePath_ = (other.getFnNamePath() != null) ?
> Sorry, I'm still not clear why we need to clone a field that is
> never modified... Did I miss anything?
Only fnNamePath_ is saved in StmtTableCache, db_, fn_ are not saved, isBuiltin_ and isAnalyzed_ are both false
http://gerrit.cloudera.org:8080/#/c/17604/3/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@66
PS3, Line 66: db_ = other.getDb();
: fn_ = other.getFunction();
: isBuiltin_ = other.isBuiltin();
: isAnalyzed_ = other.isFnAnalyzed();
> > I tried to do this, but an error occurred. Because
> FunctionName::fnNamePath_ is null.
> >
> >use functional;
> >Select count(1) from alltypesagg;
> >
> >I0621 19:03:29.787499 46677 jni-util.cc:286] 0649db5f6bc675d6:6b92f3b600000000]
> java.lang.NullPointerException
> >at org.apache.impala.analysis.FunctionCallExpr.isConstantImpl(FunctionCallExpr.java:372)
> >at org.apache.impala.analysis.Expr.isConstant(Expr.java:1460)
> >at org.apache.impala.analysis.Expr.computeNumDistinctValues(Expr.java:575)
> >at org.apache.impala.analysis.Expr.analyze(Expr.java:500)
> >......
>
> hmm, did you reset fnNamePath_ to null so it cause the
> NullPointerException? In reset() we only reset the analysis results
> which are db_, fn_, isBuiltin_ and isAnalyzed_. The fnNamePath_
> comes from the parser so should not be reset.
>
> > I modified the copy constructor of FunctionCallExpr again. I
> clone QueryStmt in the constructor of InlineViewRef. At this time,
> db_ and fn_ are both null, and isBuiltin_ and isAnalyzed_ are both
> false, because only fnNamePath_ is stored in the stmtTableCache,
> and no other values are stored.
>
> OK, this answers my question that cloning the analysis results are
> safe, because the source objects are in the StmtTableCache and they
> are never analyzed. All code paths go here will see 'other' has
> null db_ , fn_ etc.
> However, this depends on the caller implementation far away from
> this class. I think adding a reset() method to reset db_, fn_,
> isBuiltin_ and isAnalyzed_ help to prevent future bugs.
I added the reset() function, but I didn’t call it in FunctionCallExpr#resetAnalysisState for the following reasons:
1. FunctionName is created in some places, only fn_ is set, fnNamePath_ is not set, if reset is called, both fn_ and fnNamePath_ are null, for example, AggregateInfo#createCountDistinctAggExprParam,NormalizeCountStarRule#apply
2. In some places, fnNamePath_ is set, but if reset is called, FunctionName will continue to be used without analysis.
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 5
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Tue, 22 Jun 2021 07:44:36 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 3
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Sun, 20 Jun 2021 08:47:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 3:
(7 comments)
Nice finding! Thanks for fixing this bug! I just have some minor comments.
http://gerrit.cloudera.org:8080/#/c/17604/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17604/3//COMMIT_MSG@27
PS3, Line 27: defultdb
nit: defaultDb
http://gerrit.cloudera.org:8080/#/c/17604/3//COMMIT_MSG@28
PS3, Line 28: pure time
builtin?
http://gerrit.cloudera.org:8080/#/c/17604/3//COMMIT_MSG@30
PS3, Line 30:
I think you can add a summary that there is a read-write contention in FunctionName#analyze() when a FunctionName object is shared to two threads: 'isBuiltin_' can be incorrectly evaluated to false when 'db_' is temporarily modified by the other thread.
http://gerrit.cloudera.org:8080/#/c/17604/3/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java:
http://gerrit.cloudera.org:8080/#/c/17604/3/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@173
PS3, Line 173: getFunction() != null
nit: can we change this to isFnAnalyzed()? It's more readable for me.
http://gerrit.cloudera.org:8080/#/c/17604/3/fe/src/main/java/org/apache/impala/analysis/FunctionName.java
File fe/src/main/java/org/apache/impala/analysis/FunctionName.java:
http://gerrit.cloudera.org:8080/#/c/17604/3/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@64
PS3, Line 64: fnNamePath_ = (other.getFnNamePath() != null) ?
I think we don't need to clone this since it's private and never modified.
http://gerrit.cloudera.org:8080/#/c/17604/3/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@66
PS3, Line 66: db_ = other.getDb();
: fn_ = other.getFunction();
: isBuiltin_ = other.isBuiltin();
: isAnalyzed_ = other.isFnAnalyzed();
Is it safe to clone the analysis results? E.g. if two dbs both have a foo() function but using different implementation, will this cause troubles?
FunctionName doesn't have reset() or resetAnalysisState() methods. Although we reset the analysis results after cloning the QueryStmt in constructing an InlineViewRef, the reset doesn't go into this class.
If we'd like to keep them, could you add a reset() method in this class and use it in FunctionCallExpr#resetAnalysisState()?
http://gerrit.cloudera.org:8080/#/c/17604/3/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@72
PS3, Line 72: public FunctionName clone() { return new FunctionName(this); }
nit: Could you add an "@Override" annotation?
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 3
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Mon, 21 Jun 2021 07:11:26 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "liuyao (Code Review)" <ge...@cloudera.org>.
liuyao has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 4:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/17604/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17604/3//COMMIT_MSG@27
PS3, Line 27: me time.
> nit: defaultDb
Done
http://gerrit.cloudera.org:8080/#/c/17604/3//COMMIT_MSG@28
PS3, Line 28: getDefault
> builtin?
Done
http://gerrit.cloudera.org:8080/#/c/17604/3//COMMIT_MSG@30
PS3, Line 30: FunctionCallExpr::analyzeImpl, and just completed FunctionName::analyze;
> I think you can add a summary that there is a read-write contention in Func
Done
http://gerrit.cloudera.org:8080/#/c/17604/3/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java:
http://gerrit.cloudera.org:8080/#/c/17604/3/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@173
PS3, Line 173: Name_.clone();
> nit: can we change this to isFnAnalyzed()? It's more readable for me.
Yes, the readability here is not good. But I think it might be better to drop the shallow copy and modify the code that relies on the shallow copy.
http://gerrit.cloudera.org:8080/#/c/17604/3/fe/src/main/java/org/apache/impala/analysis/FunctionName.java
File fe/src/main/java/org/apache/impala/analysis/FunctionName.java:
http://gerrit.cloudera.org:8080/#/c/17604/3/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@64
PS3, Line 64: fnNamePath_ = (other.getFnNamePath() != null) ?
> I think we don't need to clone this since it's private and never modified.
I removed the "shallow copy" in the copy constructor of FunctionCallExpr, so I need to copy fnNamePath_
http://gerrit.cloudera.org:8080/#/c/17604/3/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@66
PS3, Line 66: db_ = other.getDb();
: fn_ = other.getFunction();
: isBuiltin_ = other.isBuiltin();
: isAnalyzed_ = other.isFnAnalyzed();
> Is it safe to clone the analysis results? E.g. if two dbs both have a foo()
I tried to do this, but an error occurred. Because FunctionName::fnNamePath_ is null.
use functional;
Select count(1) from alltypesagg;
I0621 19:03:29.787499 46677 jni-util.cc:286] 0649db5f6bc675d6:6b92f3b600000000] java.lang.NullPointerException
at org.apache.impala.analysis.FunctionCallExpr.isConstantImpl(FunctionCallExpr.java:372)
at org.apache.impala.analysis.Expr.isConstant(Expr.java:1460)
at org.apache.impala.analysis.Expr.computeNumDistinctValues(Expr.java:575)
at org.apache.impala.analysis.Expr.analyze(Expr.java:500)
at org.apache.impala.analysis.SelectStmt$SelectAnalyzer.analyzeSelectClause(SelectStmt.java:341)
at org.apache.impala.analysis.SelectStmt$SelectAnalyzer.analyze(SelectStmt.java:280)
at org.apache.impala.analysis.SelectStmt$SelectAnalyzer.access$100(SelectStmt.java:265)
at org.apache.impala.analysis.SelectStmt.analyze(SelectStmt.java:258)
at org.apache.impala.analysis.AnalysisContext.reAnalyze(AnalysisContext.java:575)
at org.apache.impala.analysis.AnalysisContext.analyze(AnalysisContext.java:549)
at org.apache.impala.analysis.AnalysisContext.analyzeAndAuthorize(AnalysisContext.java:445)
at org.apache.impala.service.Frontend.doCreateExecRequest(Frontend.java:1669)
at org.apache.impala.service.Frontend.getTExecRequest(Frontend.java:1635)
at org.apache.impala.service.Frontend.createExecRequest(Frontend.java:1605)
at org.apache.impala.service.JniFrontend.createExecRequest(JniFrontend.java:163)
I modified the copy constructor of FunctionCallExpr again. I clone QueryStmt in the constructor of InlineViewRef. At this time, db_ and fn_ are both null, and isBuiltin_ and isAnalyzed_ are both false, because only fnNamePath_ is stored in the stmtTableCache, and no other values are stored.
http://gerrit.cloudera.org:8080/#/c/17604/3/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@72
PS3, Line 72: @Override
> nit: Could you add an "@Override" annotation?
Done
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 4
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Mon, 21 Jun 2021 11:53:52 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "liuyao (Code Review)" <ge...@cloudera.org>.
liuyao has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17604/6/fe/src/main/java/org/apache/impala/analysis/FunctionName.java
File fe/src/main/java/org/apache/impala/analysis/FunctionName.java:
http://gerrit.cloudera.org:8080/#/c/17604/6/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@144
PS6, Line 144: if (!isFullyQualified()) {
: db_ = analyzer.getDefaultDb();
: if (preferBuiltinsDb && builtinDb.containsFunction(fn_)) {
: db_ = BuiltinsDb.NAME;
: }
: }
: Preconditions.checkNotNull(db_);
: isBuiltin_ = db_.equals(BuiltinsDb.NAME) &&
: builtinDb.containsFunction(fn_);
> I think a simpler solution is resolving the read-write contention here, i.e
This is indeed a simple and conservative method. However, the risk of shallow copy still exists, and this can be resolved later. If the database is not specified, check builtDb first, if not, set it to defaultDb. I'll modify it like this first, and then test it?
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 6
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Thu, 24 Jun 2021 02:50:43 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7255/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 6
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Wed, 23 Jun 2021 12:49:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 4: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/7241/
--
To view, visit http://gerrit.cloudera.org:8080/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 4
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Mon, 21 Jun 2021 17:47:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9763: builtin functions throw unknown exception
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception
......................................................................
Patch Set 10:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/8996/ : 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/17604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 10
Gerrit-Owner: liuyao <li...@sensorsdata.cn>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: liuyao <li...@sensorsdata.cn>
Gerrit-Comment-Date: Thu, 24 Jun 2021 08:00:13 +0000
Gerrit-HasComments: No