[GitHub] phoenix pull request #314: PHOENIX-4820
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.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. ---
[GitHub] phoenix pull request #314: PHOENIX-4820
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.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
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.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
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 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 {
--- 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
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
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
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 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 {
--- 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
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.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
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 Date: 2018-07-26T05:32:13Z PHOENIX-4820 ---
