You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "wangsheng (Code Review)" <ge...@cloudera.org> on 2022/03/19 07:30:10 UTC
[Impala-ASF-CR] IMPALA-7942: Add query hints for cardinalities and selectivities
wangsheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/18023 )
Change subject: IMPALA-7942: Add query hints for cardinalities and selectivities
......................................................................
Patch Set 8:
(1 comment)
> Code modification to facilitate above general version of grammar to
> handle predicates and selectivity hints.
>
> Since we put the hint at the class Predicate, it looks like we can
> get the hint for any simple or complex predicate at the right
> place.
>
> [15:00:23 qchen@qifan-10229: Impala.07202021] git diff
> diff --git a/fe/src/main/cup/sql-parser.cup b/fe/src/main/cup/sql-parser.cup
> index bbd1e53d0..bef0b3c1d 100644
> --- a/fe/src/main/cup/sql-parser.cup
> +++ b/fe/src/main/cup/sql-parser.cup
> @@ -307,7 +307,7 @@ terminal
> KW_RANGE, KW_RCFILE, KW_RECOVER, KW_REFERENCES, KW_REFRESH,
> KW_REGEXP, KW_RELY,
> KW_RENAME, KW_REPEATABLE, KW_REPLACE, KW_REPLICATION, KW_RESTRICT,
> KW_RETURNS,
> KW_REVOKE, KW_RIGHT, KW_RLIKE, KW_ROLE, KW_ROLES, KW_ROLLUP,
> KW_ROW, KW_ROWS, KW_SCHEMA,
> - KW_SCHEMAS, KW_SELECT, KW_SEMI, KW_SEQUENCEFILE,
> KW_SERDEPROPERTIES, KW_SERIALIZE_FN,
> + KW_SCHEMAS, KW_SELECT, KW_SELECTIVITY, KW_SEMI,
> KW_SEQUENCEFILE, KW_SERDEPROPERTIES, KW_SERIALIZE_FN,
> KW_SET, KW_SHOW, KW_SMALLINT, KW_SETS, KW_SORT, KW_SPEC, KW_STORED,
> KW_STRAIGHT_JOIN,
> KW_STRING, KW_STRUCT, KW_SYMBOL, KW_SYSTEM_TIME, KW_SYSTEM_VERSION,
> KW_TABLE, KW_TABLES, KW_TABLESAMPLE, KW_TBLPROPERTIES,
> @@ -431,7 +431,7 @@ nonterminal TimeTravelSpec opt_asof;
> nonterminal Subquery subquery;
> nonterminal JoinOperator join_operator;
> nonterminal opt_inner, opt_outer;
> -nonterminal PlanHint plan_hint;
> +nonterminal PlanHint plan_hint, selectivity_hint;
> nonterminal List<PlanHint> plan_hints, opt_plan_hints,
> plan_hint_list;
> nonterminal TypeDef type_def;
> nonterminal Type type;
> @@ -629,6 +629,8 @@ precedence left KW_INTO;
>
> precedence left KW_OVER;
>
> +precedence left COMMENTED_PLAN_HINT_START;
> +
> start with stmt;
>
> stmt ::=
> @@ -3720,6 +3722,15 @@ function_params ::=
> {: RESULT = new FunctionParams(false, true, exprs); :}
> ;
>
> +selectivity_hint ::=
> + COMMENTED_PLAN_HINT_START KW_SELECTIVITY LPAREN
> DECIMAL_LITERAL:value RPAREN
> + COMMENTED_PLAN_HINT_END
> + {:
> + RESULT = new PlanHint("selectivity",
> + new ArrayList(Arrays.asList(value.toString())));
> + :}
> + ;
> +
> predicate ::=
> expr:e KW_IS KW_NULL
> {: RESULT = new IsNullPredicate(e, false); :}
> @@ -3746,6 +3757,8 @@ predicate ::=
> :}
> | expr:e1 KW_LOGICAL_OR expr:e2
> {: RESULT = new CompoundVerticalBarExpr(e1, e2); :}
> + | predicate:p selectivity_hint:h
> + {: RESULT = p; :}
> ;
>
> comparison_predicate ::=
> diff --git a/fe/src/main/jflex/sql-scanner.flex b/fe/src/main/jflex/sql-scanner.flex
> index e60676a9c..bc3a91513 100644
> --- a/fe/src/main/jflex/sql-scanner.flex
> +++ b/fe/src/main/jflex/sql-scanner.flex
> @@ -236,6 +236,7 @@ import org.apache.impala.thrift.TReservedWordsVersion;
> keywordMap.put("schemas", SqlParserSymbols.KW_SCHEMAS);
> keywordMap.put("select", SqlParserSymbols.KW_SELECT);
> keywordMap.put("semi", SqlParserSymbols.KW_SEMI);
> + keywordMap.put("selectivity", SqlParserSymbols.KW_SELECTIVITY);
> keywordMap.put("sequencefile", SqlParserSymbols.KW_SEQUENCEFILE);
> keywordMap.put("serdeproperties", SqlParserSymbols.KW_SERDEPROPERTIES);
> keywordMap.put("serialize_fn", SqlParserSymbols.KW_SERIALIZE_FN);
Sorry for the delay, thanks for testing so carefully! From you modification above, I found some problems. You didn't assgin selectivity value to 'Predicate' in sql-parse.cup like this:
if (hint != null) {
p.setSelectivityHint(Double.valueOf(hint.getArgs().get(0)));
}
So even you can execute 'explain select * from functional.alltypestiny where int_col = 1 /* +SELECTIVITY(0.5) */' successfully, but '0.5' do not passed to 'BinaryPredicate', which means this selectivity is invalid. If you set to 0.1/0.4/0.8, the cardinality is always 4. I already try your code in my env.
Besides, we can not modify like this directly yet:
| predicate:p selectivity_hint:h
{:
if (hint != null) {
p.setSelectivityHint(Double.valueOf(hint.getArgs().get(0)));
}
RESULT = p;
:}
Since 'predicate' is an 'Expr', instead of 'Predicate'. 'setSelectivityHint' method is belong to 'Predicate'. Here is import statement:
nonterminal Expr predicate, bool_test_expr;
And this is why I define a 'predicate_with_hint' in this patch.
http://gerrit.cloudera.org:8080/#/c/18023/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18023/3//COMMIT_MSG@49
PS3, Line 49: ' with two 'BinaryPredicate', if
: set hint
> "If we only handle a subset of complex expressions, this will confused user
Hi Qifan,
"Not sure the argument holds :-) as currently the patch allows selectivity hints to be applied to a subset of predicates anyway."
About this, here is my opinion: this patch indeed 'allows selectivity hints to be applied to a subset of predicates anyway', but if we use wrong invalid hint, query will throw exception or print a warning wsg. If we set a hint to 'a=1 and b=2', the original hint will missed due to conjunct without any tips, this is what I'm worried.
"If there is a hint for the whole predicate, can we wrap the whole thing in CompoundPredicate()".
For this idea, you mean rewrite exprs refer to a compound predicate hint? I'm not sure this is suitable, but maybe too complex for this patch?
Besides, I'm not sure, if we can implement '(xxx) /* SELECTIVITY(0.1) */' in sql-parser. To be honest, I'm didn't take a deep research on java cup...
--
To view, visit http://gerrit.cloudera.org:8080/18023
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2776b9bbd878b8a21d9c866b400140a454f59e1b
Gerrit-Change-Number: 18023
Gerrit-PatchSet: 8
Gerrit-Owner: wangsheng <sk...@163.com>
Gerrit-Reviewer: Amogh Margoor <am...@gmail.com>
Gerrit-Reviewer: Fucun Chu <ch...@hotmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Sat, 19 Mar 2022 07:30:10 +0000
Gerrit-HasComments: Yes