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 <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Fri, 26 Mar 2021 01:45:24 +0000 Gerrit-HasComments: Yes
