You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by comnetwork <gi...@git.apache.org> on 2018/12/15 07:50:27 UTC
[GitHub] phoenix pull request #417: PHOENIX-4820
GitHub user comnetwork opened a pull request:
https://github.com/apache/phoenix/pull/417
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/417.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 #417
----
commit 25066184a7fbfbebceedb287eaa1c45914504207
Author: chenglei <ch...@...>
Date: 2018-12-15T07:45:17Z
PHOENIX 4820
----
---
[GitHub] phoenix issue #417: PHOENIX-4820
Posted by comnetwork <gi...@git.apache.org>.
Github user comnetwork commented on the issue:
https://github.com/apache/phoenix/pull/417
@twdsilva ,thank you for the review, yes , I think the approach of your diff file is more reasonable than my added method Expression.isConstantIfChildrenAllConstant and improves the inappropriate modification introduced by PHOENIX-2169 at the same time
---
[GitHub] phoenix pull request #417: PHOENIX-4820
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/417#discussion_r244065508
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/util/ExpressionUtil.java ---
@@ -68,4 +76,144 @@ public static boolean isPkPositionChanging(TableRef tableRef, List<Expression> p
return false;
}
+ /**
+ * check the whereExpression to see if the columnExpression is constant.
+ * eg. for "where a =3 and b > 9", a is constant,but b is not.
+ * @param columnExpression
+ * @param whereExpression
+ * @return
+ */
+ public static boolean isColumnExpressionConstant(ColumnExpression 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> {
+ private final Expression columnExpression ;
+ private Expression firstRhsConstantExpression = null;
+ private int rhsConstantCount = 0;
+ private boolean isNullExpressionVisited = false;
+
+ public IsColumnConstantExpressionVisitor(Expression columnExpression) {
+ this.columnExpression = columnExpression;
+ }
+ /**
+ * only conside and,for "where a = 3 or b = 9", neither a or b is constant.
--- End diff --
nit: only consider
---
[GitHub] phoenix pull request #417: PHOENIX-4820
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/417#discussion_r244065522
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/util/ExpressionUtil.java ---
@@ -68,4 +76,144 @@ public static boolean isPkPositionChanging(TableRef tableRef, List<Expression> p
return false;
}
+ /**
+ * check the whereExpression to see if the columnExpression is constant.
+ * eg. for "where a =3 and b > 9", a is constant,but b is not.
+ * @param columnExpression
+ * @param whereExpression
+ * @return
+ */
+ public static boolean isColumnExpressionConstant(ColumnExpression 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> {
+ private final Expression columnExpression ;
+ private Expression firstRhsConstantExpression = null;
+ private int rhsConstantCount = 0;
+ private boolean isNullExpressionVisited = false;
+
+ public IsColumnConstantExpressionVisitor(Expression columnExpression) {
+ this.columnExpression = columnExpression;
+ }
+ /**
+ * only conside and,for "where a = 3 or b = 9", neither a or b is constant.
+ */
+ @Override
+ public Iterator<Expression> visitEnter(AndExpression andExpression) {
+ if(rhsConstantCount > 1) {
+ return null;
+ }
+ return andExpression.getChildren().iterator();
+ }
+ /**
+ * <pre>
+ * We just conside {@link ComparisonExpression} because:
--- End diff --
nit: only consider
---
[GitHub] phoenix pull request #417: 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/417#discussion_r243724103
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/OrderByCompiler.java ---
@@ -88,7 +88,8 @@ public static OrderBy compile(StatementContext context,
Integer offset,
RowProjector rowProjector,
TupleProjector tupleProjector,
- boolean isInRowKeyOrder) throws SQLException {
+ boolean isInRowKeyOrder,
+ Expression whereExpression) throws SQLException {
--- End diff --
We need the whereExpression because in OrderPreservingTracker.IsConstantVisitor, we need whereExpression to help us to check the columnExpression is constant or not. Just take the same sql as example:
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
We need to check if "CASE WHEN coalesce(a.ak1,1) > coalesce(a.av2,2) THEN coalesce(a.ak1,1) ELSE coalesce(a.av2,2) END,a.av1" is constant in OrderPreservingTracker.hasEqualityConstraints method, so we move forward to check if the a.ak1 and a.av2 are constant in OrderPreservingTracker.IsConstantVisitor, and we must visit the where clause "where a.ak1 = 0.0 and a.av2 = length(substr('abc',1,1))" to check the a.ak1 and a.av2.
BTW. There is no whereExpression in current code of OrderByCompiler because for PHOENIX-3451 which was completed by James and me before, RowKeyColumnExpression is only supported to check in OrderPreservingTracker.hasEqualityConstraints, and two FIXME tags were left in this method at at time:
private boolean hasEqualityConstraints(int startPos, int endPos) {
ScanRanges ranges = context.getScanRanges();
// If a GROUP BY is being done, then the rows are ordered according to the GROUP BY key,
// not by the original row key order of the table (see PHOENIX-3451).
// We check each GROUP BY expression to see if it only references columns that are
// matched by equality constraints, in which case the expression itself would be constant.
// FIXME: this only recognizes row key columns that are held constant, not all columns.
// FIXME: we should optimize out any GROUP BY or ORDER BY expression which is deemed to
// be held constant based on the WHERE clause.
if (!groupBy.isEmpty()) {
In this patch, I added whereExpression to try to fix this problem to support non-pk columns also.
---
[GitHub] phoenix pull request #417: PHOENIX-4820
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/417#discussion_r243703560
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/expression/Expression.java ---
@@ -88,4 +88,10 @@
* @return
*/
boolean requiresFinalEvaluation();
+
--- End diff --
As James said before its not clear to me why you need this new function, can just use ExpressionUtil.isConstant() ?
Your implementation returns true by default and is false only for AggregateFunction, RandomFunction and SequenceValueExpression for which ExpressionUtil.isConstant() should also return false, right?
---
[GitHub] phoenix pull request #417: PHOENIX-4820
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/417#discussion_r243703928
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/OrderByCompiler.java ---
@@ -88,7 +88,8 @@ public static OrderBy compile(StatementContext context,
Integer offset,
RowProjector rowProjector,
TupleProjector tupleProjector,
- boolean isInRowKeyOrder) throws SQLException {
+ boolean isInRowKeyOrder,
+ Expression whereExpression) throws SQLException {
--- End diff --
Can you explain why the whereExpression is needed?
---
[GitHub] phoenix pull request #417: 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/417#discussion_r243723430
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/expression/Expression.java ---
@@ -88,4 +88,10 @@
* @return
*/
boolean requiresFinalEvaluation();
+
--- End diff --
ExpressionUtil.isConstant(Expression) is not suitable for OrderPreservingTracker.IsConstantVisitor, because ExpressionUtil.isConstant(Expression) depends on Expression.isStateless() and Expression.getDeterminism(). Expression isStateless() is to check if the expression is depend on the server state, even for RowKeyColumnExpression , isStateless() is false. What we want to check 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 for expression "CASE WHEN coalesce(a.ak1,1) > coalesce(a.av2,2) THEN coalesce(a.ak1,1) ELSE coalesce(a.av2,2) END", the determinism is Determinism.PER_INVOCATION and isStateless is false , 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.
BTW. The value returned by ExpressionUtil.isConstant() is complicated and runtime related because of Expression.getDeterminism(). In following BaseCompoundExpression.init method, determinism is the combine of its children.
` private void init(List<Expression> children) {
this.children = ImmutableList.copyOf(children);
boolean isStateless = true;
boolean isNullable = false;
boolean requiresFinalEvaluation = false;
this.determinism = Determinism.ALWAYS;
for (int i = 0; i < children.size(); i++) {
Expression child = children.get(i);
isNullable |= child.isNullable();
isStateless &= child.isStateless();
this.determinism = this.determinism.combine(child.getDeterminism());
requiresFinalEvaluation |= child.requiresFinalEvaluation();
}
this.isStateless = isStateless;
this.isNullable = isNullable;
this.requiresFinalEvaluation = requiresFinalEvaluation;
}`
---
[GitHub] phoenix pull request #417: PHOENIX-4820
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/417#discussion_r244065574
--- Diff: phoenix-core/src/test/java/org/apache/phoenix/compile/QueryCompilerTest.java ---
@@ -2951,6 +2951,129 @@ public void testOrderPreservingGroupBy() throws Exception {
}
}
}
+
--- End diff --
Nice tests!
---
[GitHub] phoenix pull request #417: PHOENIX-4820
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/417#discussion_r244065455
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/OrderByCompiler.java ---
@@ -88,7 +88,8 @@ public static OrderBy compile(StatementContext context,
Integer offset,
RowProjector rowProjector,
TupleProjector tupleProjector,
- boolean isInRowKeyOrder) throws SQLException {
+ boolean isInRowKeyOrder,
+ Expression whereExpression) throws SQLException {
--- End diff --
Thanks for the explanation.
---