You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@phoenix.apache.org by comnetwork <gi...@git.apache.org> on 2018/07/26 05:38:06 UTC
[GitHub] phoenix pull request #314: PHOENIX-4820
GitHub user comnetwork opened a pull request:
https://github.com/apache/phoenix/pull/314
PHOENIX-4820
PHOENIX-4820
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/comnetwork/phoenix 4.x-HBase-1.3
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/phoenix/pull/314.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #314
----
commit ff779298e8c521c8a86d78ea7364d8eff8942c1a
Author: chenglei <ch...@...>
Date: 2018-07-26T05:32:13Z
PHOENIX-4820
----
---
[GitHub] phoenix pull request #314: PHOENIX-4820
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/314#discussion_r205982004
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java ---
@@ -559,8 +559,16 @@ protected QueryPlan compileSingleFlatQuery(StatementContext context, SelectState
groupBy = groupBy.compile(context, innerPlanTupleProjector);
context.setResolver(resolver); // recover resolver
RowProjector projector = ProjectionCompiler.compile(context, select, groupBy, asSubquery ? Collections.<PDatum>emptyList() : targetColumns, where);
- OrderBy orderBy = OrderByCompiler.compile(context, select, groupBy, limit, offset, projector,
- groupBy == GroupBy.EMPTY_GROUP_BY ? innerPlanTupleProjector : null, isInRowKeyOrder);
+ OrderBy orderBy = OrderByCompiler.compile(
+ context,
+ select,
+ groupBy,
+ limit,
+ offset,
+ projector,
+ groupBy == GroupBy.EMPTY_GROUP_BY ? innerPlanTupleProjector : null,
+ groupBy == GroupBy.EMPTY_GROUP_BY ? isInRowKeyOrder : true,
+ where);
--- End diff --
Would be ideal if the change could be isolated to OrderByCompiler being aware of its inner plan to determine correctly whether the OrderBy can be compiled out. Any ideas, @maryannxue?
---
[GitHub] phoenix pull request #314: PHOENIX-4820
Posted by maryannxue <gi...@git.apache.org>.
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/314#discussion_r206377190
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java ---
@@ -559,8 +559,16 @@ protected QueryPlan compileSingleFlatQuery(StatementContext context, SelectState
groupBy = groupBy.compile(context, innerPlanTupleProjector);
context.setResolver(resolver); // recover resolver
RowProjector projector = ProjectionCompiler.compile(context, select, groupBy, asSubquery ? Collections.<PDatum>emptyList() : targetColumns, where);
- OrderBy orderBy = OrderByCompiler.compile(context, select, groupBy, limit, offset, projector,
- groupBy == GroupBy.EMPTY_GROUP_BY ? innerPlanTupleProjector : null, isInRowKeyOrder);
+ OrderBy orderBy = OrderByCompiler.compile(
+ context,
+ select,
+ groupBy,
+ limit,
+ offset,
+ projector,
+ groupBy == GroupBy.EMPTY_GROUP_BY ? innerPlanTupleProjector : null,
+ groupBy == GroupBy.EMPTY_GROUP_BY ? isInRowKeyOrder : true,
+ where);
--- End diff --
Eliminating order-by based on the inner plan ordering is definitely the right and ultimate solution, and we should get rid of all “dirty fix” flags here. The compile result, in our case, the QueryPlan should have a definite ordering whether it’s from an order by, or a group-by, or a natural row order, or an order-by from the inner query. However, letting QueryPlan contain that information might not be a good idea. So we should see if we can infer the ordering from a QueryPlan.
---
[GitHub] phoenix pull request #314: PHOENIX-4820
Posted by comnetwork <gi...@git.apache.org>.
Github user comnetwork commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/314#discussion_r206013986
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/util/ExpressionUtil.java ---
@@ -68,4 +74,85 @@ public static boolean isPkPositionChanging(TableRef tableRef, List<Expression> p
return false;
}
+ public static boolean isColumnConstant(Expression columnExpression, Expression whereExpression) {
+ if(whereExpression == null) {
+ return false;
+ }
+ IsColumnConstantExpressionVisitor isColumnConstantExpressionVisitor =
+ new IsColumnConstantExpressionVisitor(columnExpression);
+ whereExpression.accept(isColumnConstantExpressionVisitor);
+ return isColumnConstantExpressionVisitor.isConstant();
+ }
+
+ private static class IsColumnConstantExpressionVisitor extends StatelessTraverseNoExpressionVisitor<Void> {
--- End diff --
I did not find a existing visitor which can statisfy the requirement , I would check more Expression besides ComparisonExpression.
---
[GitHub] phoenix pull request #314: PHOENIX-4820
Posted by comnetwork <gi...@git.apache.org>.
Github user comnetwork commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/314#discussion_r206014266
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java ---
@@ -559,8 +559,16 @@ protected QueryPlan compileSingleFlatQuery(StatementContext context, SelectState
groupBy = groupBy.compile(context, innerPlanTupleProjector);
context.setResolver(resolver); // recover resolver
RowProjector projector = ProjectionCompiler.compile(context, select, groupBy, asSubquery ? Collections.<PDatum>emptyList() : targetColumns, where);
- OrderBy orderBy = OrderByCompiler.compile(context, select, groupBy, limit, offset, projector,
- groupBy == GroupBy.EMPTY_GROUP_BY ? innerPlanTupleProjector : null, isInRowKeyOrder);
+ OrderBy orderBy = OrderByCompiler.compile(
+ context,
+ select,
+ groupBy,
+ limit,
+ offset,
+ projector,
+ groupBy == GroupBy.EMPTY_GROUP_BY ? innerPlanTupleProjector : null,
+ groupBy == GroupBy.EMPTY_GROUP_BY ? isInRowKeyOrder : true,
+ where);
--- End diff --
Yes, it is a nice suggestion, I would make a modification
---
[GitHub] phoenix pull request #314: PHOENIX-4820
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/314#discussion_r205981788
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/expression/Expression.java ---
@@ -88,4 +88,10 @@
* @return
*/
boolean requiresFinalEvaluation();
+
+ /**
+ *
+ * @return
+ */
+ boolean isConstantIfChildrenAllConstant();
--- End diff --
It's not clear that we need this. We already rollup determinism and isStateless for the expression tree. If determinism == Determinism.PER_STATEMENT or Determinism.ALWAYS and isStateless is true, then we know an expression is a constant. We have a utility for this in ExpressionUtil.isConstant(Expression).
---
[GitHub] phoenix pull request #314: PHOENIX-4820
Posted by comnetwork <gi...@git.apache.org>.
Github user comnetwork commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/314#discussion_r206012706
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/expression/Expression.java ---
@@ -88,4 +88,10 @@
* @return
*/
boolean requiresFinalEvaluation();
+
+ /**
+ *
+ * @return
+ */
+ boolean isConstantIfChildrenAllConstant();
--- End diff --
I think ExpressionUtil.isConstant(Expression) is not suitable for this case, just as the comments of jira said, what we want to check in this issue is if a expression is constant when all children of it are constants, just consider following sql:
select a.ak3 from
(select rand() ak1,length(pk2) ak2,length(pk3) ak3,length(v1) av1,length(v2) av2 from test order by pk2,pk3 limit 10) a
where a.ak1 = 0.0 and a.av2 = length(substr('abc',1,1))
group by a.ak3,CASE WHEN coalesce(a.ak1,1) > coalesce(a.av2,2) THEN coalesce(a.ak1,1) ELSE coalesce(a.av2,2) END,a.av1
order by a.ak3,a.av1
Obviously , because of rand(), the Determinism of expression a.ak1 is Determinism.PER_INVOCATION, so the determinism is Determinism.PER_INVOCATION and isStateless is false for expression "CASE WHEN coalesce(a.ak1,1) > coalesce(a.av2,2) THEN coalesce(a.ak1,1) ELSE coalesce(a.av2,2) END", but because the a.ak1 and a.av2 are both constants in where clause of outer query, we can regard "CASE WHEN coalesce(a.ak1,1) > coalesce(a.av2,2) THEN coalesce(a.ak1,1) ELSE coalesce(a.av2,2) END" as constant in IsConstantVisitor.
---
[GitHub] phoenix pull request #314: PHOENIX-4820
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/314#discussion_r205981903
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/util/ExpressionUtil.java ---
@@ -68,4 +74,85 @@ public static boolean isPkPositionChanging(TableRef tableRef, List<Expression> p
return false;
}
+ public static boolean isColumnConstant(Expression columnExpression, Expression whereExpression) {
+ if(whereExpression == null) {
+ return false;
+ }
+ IsColumnConstantExpressionVisitor isColumnConstantExpressionVisitor =
+ new IsColumnConstantExpressionVisitor(columnExpression);
+ whereExpression.accept(isColumnConstantExpressionVisitor);
+ return isColumnConstantExpressionVisitor.isConstant();
+ }
+
+ private static class IsColumnConstantExpressionVisitor extends StatelessTraverseNoExpressionVisitor<Void> {
--- End diff --
I'm hoping we don't need a new visitor here. There are probably other cases to check for besides ComparisonExpression. For example, InListExpression, maybe CoerceExpression, etc.
---
[GitHub] phoenix pull request #314: PHOENIX-4820
Posted by comnetwork <gi...@git.apache.org>.
Github user comnetwork commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/314#discussion_r206382301
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java ---
@@ -559,8 +559,16 @@ protected QueryPlan compileSingleFlatQuery(StatementContext context, SelectState
groupBy = groupBy.compile(context, innerPlanTupleProjector);
context.setResolver(resolver); // recover resolver
RowProjector projector = ProjectionCompiler.compile(context, select, groupBy, asSubquery ? Collections.<PDatum>emptyList() : targetColumns, where);
- OrderBy orderBy = OrderByCompiler.compile(context, select, groupBy, limit, offset, projector,
- groupBy == GroupBy.EMPTY_GROUP_BY ? innerPlanTupleProjector : null, isInRowKeyOrder);
+ OrderBy orderBy = OrderByCompiler.compile(
+ context,
+ select,
+ groupBy,
+ limit,
+ offset,
+ projector,
+ groupBy == GroupBy.EMPTY_GROUP_BY ? innerPlanTupleProjector : null,
+ groupBy == GroupBy.EMPTY_GROUP_BY ? isInRowKeyOrder : true,
+ where);
--- End diff --
@maryannxue , yes, you are right , Eliminating order-by based on the inner plan ordering is ultimate solution. Openning this JIRA is just to optimize the OrderBy for ClientAggregatePlan when there is a GroupBy, it is only to need to conside the GroupBy of the outer query, and innerQuery is not required to take into account. It is much simpler than purely optimizing OrderBy for ClientScanPlan, which need to make a huge modification and refactor with OrderByCompiler and OrderPreservingTracker. I have planned to purely optimize OrderBy for ClientScanPlan after this JIRA by openning a new JIRA.
---