You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Aman Sinha (Code Review)" <ge...@cloudera.org> on 2020/09/03 01:35:20 UTC
[Impala-ASF-CR] IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
Aman Sinha has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16407
Change subject: IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
......................................................................
IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
This change allows unwrapping a builtin cast function such as
casttobigint(col) similar to a CAST(col as bigint). Unwrapping
is useful to access the SlotRef of the column and this in turn
is needed to compute predicate selectivity correctly. Without
unwrapping, the cast function uses default 10 % selectivity
for a predicate such as 'casttobigint(l_quantity) is NOT NULL'
which is not accurate.
Note that Impala does not allow a user query to directly call the
builtin cast function..rather they have to use the explicit CAST
syntax. However, since the frontend jar can be used by an external
frontend module as a library, the builtin function can be called
and this patch makes the behavior consistent.
Testing:
- Ran PlannerTest
- Manual testing by commenting out the code in
FunctionCallExpr.analyzeImpl() that throws an AnalysisException
if builtin cast function is called. I haven't added a new test
for this reason.
Cardinality before this change:
explain select * from date_dim d1, date_dim d2
where d1.d_week_seq = d2.d_week_seq - 52
and casttobigint(d1.d_week_seq) is not null
and casttobigint(d2.d_week_seq) is not null
SCAN HDFS [tpcds.date_dim d1]
HDFS partitions=1/1 files=1 size=9.84MB
predicates: casttobigint(d1.d_week_seq) IS NOT NULL
runtime filters: RF000 -> d1.d_week_seq
row-size=255B cardinality=7.30K
Cardinality after this change:
SCAN HDFS [tpcds.date_dim d1]
HDFS partitions=1/1 files=1 size=9.84MB
predicates: casttobigint(d1.d_week_seq) IS NOT NULL
runtime filters: RF000 -> d1.d_week_seq
row-size=255B cardinality=73.05K
Change-Id: Idf82b2de78c6a7051ea036062f177d69e2558940
---
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
2 files changed, 8 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/16407/1
--
To view, visit http://gerrit.cloudera.org:8080/16407
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Idf82b2de78c6a7051ea036062f177d69e2558940
Gerrit-Change-Number: 16407
Gerrit-PatchSet: 1
Gerrit-Owner: Aman Sinha <am...@cloudera.com>
[Impala-ASF-CR] IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/16407 )
Change subject: IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16407/3/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:
http://gerrit.cloudera.org:8080/#/c/16407/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@329
PS3, Line 329: public static final com.google.common.base.Predicate<Expr> IS_IS_NULL_PREDICATE =
> These were not part of my patch though. I am inclined to leave it as-is sin
Done
http://gerrit.cloudera.org:8080/#/c/16407/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@339
PS3, Line 339: IS_DISTINCT_FROM_OR_NOT_DISTINCT_PREDICATE =
> Maybe should add a comment here why we have this variant.
Done
--
To view, visit http://gerrit.cloudera.org:8080/16407
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idf82b2de78c6a7051ea036062f177d69e2558940
Gerrit-Change-Number: 16407
Gerrit-PatchSet: 3
Gerrit-Owner: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Wed, 24 Mar 2021 17:14:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
Posted by "Aman Sinha (Code Review)" <ge...@cloudera.org>.
Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/16407 )
Change subject: IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
......................................................................
Patch Set 4:
(3 comments)
Thanks Quanlong for the review ! Pls see my response below. Let me know if you have questions/suggestions.
http://gerrit.cloudera.org:8080/#/c/16407/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16407/3//COMMIT_MSG@18
PS3, Line 18: .
> nit: ,
Done
http://gerrit.cloudera.org:8080/#/c/16407/3/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:
http://gerrit.cloudera.org:8080/#/c/16407/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@1625
PS3, Line 1625: isBuiltinCastFunction
> Should we consider other builtin functions that supposed to keep the same c
Hmm..I suppose there could be other scalar builtin functions to consider for example upper(), lower() although that is already under-estimated in Impala (see below). My intent in this patch was to just bring parity for the builtin cast function because that is a special one. For other predicates such as
[localhost:21050] tpch> Explain select * from tpch.nation where UPPER (n_name) IS NOT NULL
00:SCAN HDFS [tpch.nation] |
| HDFS partitions=1/1 files=1 size=2.15KB |
| predicates: upper(n_name) IS NOT NULL |
| row-size=109B cardinality=3
Here, the correct cardinality should have been 25 since UPPER does not change the nullability. But this is an existing issue and we could look at other such examples. If you know of any prior jiras around this, let me know. Otherwise I will create one.
http://gerrit.cloudera.org:8080/#/c/16407/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@1624
PS3, Line 1624: (this instanceof FunctionCallExpr
: && ((FunctionCallExpr) this).isBuiltinCastFunction())
> Should we consider 'implicitOnly' in this branch? Is 'implicitOnly' used by
Since a builtin cast function is of type FunctionCallExpr, it does not have any attribute of implicit or explicit (unlike a CastExpr), so in that sense it would be somewhat meaningless to check the implicitOnly flag. The external FE does not directly use or populate the implicitOnly flag. But since it calls analyze which calls computeSelectivity(), it ends up here.
--
To view, visit http://gerrit.cloudera.org:8080/16407
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idf82b2de78c6a7051ea036062f177d69e2558940
Gerrit-Change-Number: 16407
Gerrit-PatchSet: 4
Gerrit-Owner: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Fri, 26 Mar 2021 01:45:24 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16407 )
Change subject: IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/7124/ : 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/16407
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idf82b2de78c6a7051ea036062f177d69e2558940
Gerrit-Change-Number: 16407
Gerrit-PatchSet: 2
Gerrit-Owner: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Sep 2020 03:30:22 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/16407 )
Change subject: IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
......................................................................
Patch Set 4: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16407/3/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:
http://gerrit.cloudera.org:8080/#/c/16407/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@1624
PS3, Line 1624: (this instanceof FunctionCallExpr
: && ((FunctionCallExpr) this).isBuiltinCastFunction())
> Since a builtin cast function is of type FunctionCallExpr, it does not have
Ack
http://gerrit.cloudera.org:8080/#/c/16407/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@1625
PS3, Line 1625: isBuiltinCastFunction
> Hmm..I suppose there could be other scalar builtin functions to consider f
Thanks for the explanation! I'm not aware of any jiras yet.
--
To view, visit http://gerrit.cloudera.org:8080/16407
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idf82b2de78c6a7051ea036062f177d69e2558940
Gerrit-Change-Number: 16407
Gerrit-PatchSet: 4
Gerrit-Owner: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Fri, 26 Mar 2021 07:16:18 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
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/16407 )
Change subject: IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
......................................................................
IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
This change allows unwrapping a builtin cast function such as
casttobigint(col) similar to a CAST(col as bigint). Unwrapping
is useful to access the SlotRef of the column and this in turn
is needed to compute predicate selectivity correctly. Without
unwrapping, the cast function uses default 10 % selectivity
for a predicate such as 'casttobigint(l_quantity) is NOT NULL'
which is not accurate.
Note that Impala does not allow a user query to directly call the
builtin cast function. Rather, they have to use the explicit CAST
syntax. However, since the frontend jar can be used by an external
frontend module as a library, the builtin function can be called
and this patch makes the behavior consistent.
Testing:
- Ran PlannerTest
- Manual testing by commenting out the code in
FunctionCallExpr.analyzeImpl() that throws an AnalysisException
if builtin cast function is called. I haven't added a new test
for this reason.
Cardinality before this change:
explain select * from date_dim d1, date_dim d2
where d1.d_week_seq = d2.d_week_seq - 52
and casttobigint(d1.d_week_seq) is not null
and casttobigint(d2.d_week_seq) is not null
SCAN HDFS [tpcds.date_dim d1]
HDFS partitions=1/1 files=1 size=9.84MB
predicates: casttobigint(d1.d_week_seq) IS NOT NULL
runtime filters: RF000 -> d1.d_week_seq
row-size=255B cardinality=7.30K
Cardinality after this change:
SCAN HDFS [tpcds.date_dim d1]
HDFS partitions=1/1 files=1 size=9.84MB
predicates: casttobigint(d1.d_week_seq) IS NOT NULL
runtime filters: RF000 -> d1.d_week_seq
row-size=255B cardinality=73.05K
Change-Id: Idf82b2de78c6a7051ea036062f177d69e2558940
Reviewed-on: http://gerrit.cloudera.org:8080/16407
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/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
2 files changed, 7 insertions(+), 4 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/16407
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Idf82b2de78c6a7051ea036062f177d69e2558940
Gerrit-Change-Number: 16407
Gerrit-PatchSet: 6
Gerrit-Owner: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
[Impala-ASF-CR] IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16407 )
Change subject: IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/8447/ : 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/16407
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idf82b2de78c6a7051ea036062f177d69e2558940
Gerrit-Change-Number: 16407
Gerrit-PatchSet: 4
Gerrit-Owner: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Fri, 26 Mar 2021 01:30:37 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16407 )
Change subject: IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/16407
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idf82b2de78c6a7051ea036062f177d69e2558940
Gerrit-Change-Number: 16407
Gerrit-PatchSet: 5
Gerrit-Owner: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Sat, 27 Mar 2021 09:05:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16407 )
Change subject: IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
......................................................................
Patch Set 5: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/7006/
--
To view, visit http://gerrit.cloudera.org:8080/16407
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idf82b2de78c6a7051ea036062f177d69e2558940
Gerrit-Change-Number: 16407
Gerrit-PatchSet: 5
Gerrit-Owner: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Fri, 26 Mar 2021 18:25:30 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16407 )
Change subject: IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7011/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/16407
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idf82b2de78c6a7051ea036062f177d69e2558940
Gerrit-Change-Number: 16407
Gerrit-PatchSet: 5
Gerrit-Owner: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Sat, 27 Mar 2021 03:27:59 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
Posted by "Aman Sinha (Code Review)" <ge...@cloudera.org>.
Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/16407 )
Change subject: IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16407/3/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:
http://gerrit.cloudera.org:8080/#/c/16407/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@1625
PS3, Line 1625: isBuiltinCastFunction
> Thanks for the explanation! I'm not aware of any jiras yet.
Created https://issues.apache.org/jira/browse/IMPALA-10615
--
To view, visit http://gerrit.cloudera.org:8080/16407
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idf82b2de78c6a7051ea036062f177d69e2558940
Gerrit-Change-Number: 16407
Gerrit-PatchSet: 4
Gerrit-Owner: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Fri, 26 Mar 2021 17:07:44 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
Posted by "Aman Sinha (Code Review)" <ge...@cloudera.org>.
Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/16407 )
Change subject: IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16407/3/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:
http://gerrit.cloudera.org:8080/#/c/16407/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@329
PS3, Line 329: public static final com.google.common.base.Predicate<Expr> IS_IS_NULL_PREDICATE =
> Change name to IS_ISNULL_PREDICATE for consistency.
These were not part of my patch though. I am inclined to leave it as-is since it will change the caller as well. Let me know if you feel otherwise.
--
To view, visit http://gerrit.cloudera.org:8080/16407
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idf82b2de78c6a7051ea036062f177d69e2558940
Gerrit-Change-Number: 16407
Gerrit-PatchSet: 3
Gerrit-Owner: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Tue, 23 Mar 2021 19:41:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16407 )
Change subject: IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/16407
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idf82b2de78c6a7051ea036062f177d69e2558940
Gerrit-Change-Number: 16407
Gerrit-PatchSet: 5
Gerrit-Owner: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Fri, 26 Mar 2021 17:09:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
Posted by "Aman Sinha (Code Review)" <ge...@cloudera.org>.
Hello Quanlong Huang, Kurt Deschler, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16407
to look at the new patch set (#4).
Change subject: IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
......................................................................
IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
This change allows unwrapping a builtin cast function such as
casttobigint(col) similar to a CAST(col as bigint). Unwrapping
is useful to access the SlotRef of the column and this in turn
is needed to compute predicate selectivity correctly. Without
unwrapping, the cast function uses default 10 % selectivity
for a predicate such as 'casttobigint(l_quantity) is NOT NULL'
which is not accurate.
Note that Impala does not allow a user query to directly call the
builtin cast function. Rather, they have to use the explicit CAST
syntax. However, since the frontend jar can be used by an external
frontend module as a library, the builtin function can be called
and this patch makes the behavior consistent.
Testing:
- Ran PlannerTest
- Manual testing by commenting out the code in
FunctionCallExpr.analyzeImpl() that throws an AnalysisException
if builtin cast function is called. I haven't added a new test
for this reason.
Cardinality before this change:
explain select * from date_dim d1, date_dim d2
where d1.d_week_seq = d2.d_week_seq - 52
and casttobigint(d1.d_week_seq) is not null
and casttobigint(d2.d_week_seq) is not null
SCAN HDFS [tpcds.date_dim d1]
HDFS partitions=1/1 files=1 size=9.84MB
predicates: casttobigint(d1.d_week_seq) IS NOT NULL
runtime filters: RF000 -> d1.d_week_seq
row-size=255B cardinality=7.30K
Cardinality after this change:
SCAN HDFS [tpcds.date_dim d1]
HDFS partitions=1/1 files=1 size=9.84MB
predicates: casttobigint(d1.d_week_seq) IS NOT NULL
runtime filters: RF000 -> d1.d_week_seq
row-size=255B cardinality=73.05K
Change-Id: Idf82b2de78c6a7051ea036062f177d69e2558940
---
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
2 files changed, 7 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/16407/4
--
To view, visit http://gerrit.cloudera.org:8080/16407
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idf82b2de78c6a7051ea036062f177d69e2558940
Gerrit-Change-Number: 16407
Gerrit-PatchSet: 4
Gerrit-Owner: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
[Impala-ASF-CR] IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16407 )
Change subject: IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/7081/ : 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/16407
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idf82b2de78c6a7051ea036062f177d69e2558940
Gerrit-Change-Number: 16407
Gerrit-PatchSet: 1
Gerrit-Owner: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 03 Sep 2020 01:56:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/16407 )
Change subject: IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/16407/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16407/3//COMMIT_MSG@18
PS3, Line 18: ..
nit: ,
http://gerrit.cloudera.org:8080/#/c/16407/3/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:
http://gerrit.cloudera.org:8080/#/c/16407/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@1624
PS3, Line 1624: (this instanceof FunctionCallExpr
: && ((FunctionCallExpr) this).isBuiltinCastFunction())
Should we consider 'implicitOnly' in this branch? Is 'implicitOnly' used by the external frontend?
http://gerrit.cloudera.org:8080/#/c/16407/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@1625
PS3, Line 1625: isBuiltinCastFunction
Should we consider other builtin functions that supposed to keep the same cardinality?
--
To view, visit http://gerrit.cloudera.org:8080/16407
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idf82b2de78c6a7051ea036062f177d69e2558940
Gerrit-Change-Number: 16407
Gerrit-PatchSet: 3
Gerrit-Owner: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Wed, 24 Mar 2021 04:08:32 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16407 )
Change subject: IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7006/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/16407
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idf82b2de78c6a7051ea036062f177d69e2558940
Gerrit-Change-Number: 16407
Gerrit-PatchSet: 5
Gerrit-Owner: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Fri, 26 Mar 2021 17:09:53 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/16407 )
Change subject: IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16407/3/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:
http://gerrit.cloudera.org:8080/#/c/16407/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@329
PS3, Line 329: public static final com.google.common.base.Predicate<Expr> IS_IS_NULL_PREDICATE =
Change name to IS_ISNULL_PREDICATE for consistency.
http://gerrit.cloudera.org:8080/#/c/16407/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@339
PS3, Line 339: IS_DISTINCT_FROM_OR_NOT_DISTINCT_PREDICATE =
Maybe should add a comment here why we have this variant.
--
To view, visit http://gerrit.cloudera.org:8080/16407
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idf82b2de78c6a7051ea036062f177d69e2558940
Gerrit-Change-Number: 16407
Gerrit-PatchSet: 3
Gerrit-Owner: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Comment-Date: Tue, 16 Mar 2021 17:43:02 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
Posted by "Aman Sinha (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16407
to look at the new patch set (#2).
Change subject: IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
......................................................................
IMPALA-10116: Allow unwrapping a builtin cast function similar to CastExpr
This change allows unwrapping a builtin cast function such as
casttobigint(col) similar to a CAST(col as bigint). Unwrapping
is useful to access the SlotRef of the column and this in turn
is needed to compute predicate selectivity correctly. Without
unwrapping, the cast function uses default 10 % selectivity
for a predicate such as 'casttobigint(l_quantity) is NOT NULL'
which is not accurate.
Note that Impala does not allow a user query to directly call the
builtin cast function..rather they have to use the explicit CAST
syntax. However, since the frontend jar can be used by an external
frontend module as a library, the builtin function can be called
and this patch makes the behavior consistent.
Testing:
- Ran PlannerTest
- Manual testing by commenting out the code in
FunctionCallExpr.analyzeImpl() that throws an AnalysisException
if builtin cast function is called. I haven't added a new test
for this reason.
Cardinality before this change:
explain select * from date_dim d1, date_dim d2
where d1.d_week_seq = d2.d_week_seq - 52
and casttobigint(d1.d_week_seq) is not null
and casttobigint(d2.d_week_seq) is not null
SCAN HDFS [tpcds.date_dim d1]
HDFS partitions=1/1 files=1 size=9.84MB
predicates: casttobigint(d1.d_week_seq) IS NOT NULL
runtime filters: RF000 -> d1.d_week_seq
row-size=255B cardinality=7.30K
Cardinality after this change:
SCAN HDFS [tpcds.date_dim d1]
HDFS partitions=1/1 files=1 size=9.84MB
predicates: casttobigint(d1.d_week_seq) IS NOT NULL
runtime filters: RF000 -> d1.d_week_seq
row-size=255B cardinality=73.05K
Change-Id: Idf82b2de78c6a7051ea036062f177d69e2558940
---
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
2 files changed, 7 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/16407/2
--
To view, visit http://gerrit.cloudera.org:8080/16407
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idf82b2de78c6a7051ea036062f177d69e2558940
Gerrit-Change-Number: 16407
Gerrit-PatchSet: 2
Gerrit-Owner: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>