Skip to content

Commit

Permalink
Allow expressions based on DISTINCT values in ORDER BY
Browse files Browse the repository at this point in the history
  • Loading branch information
katzyn committed Jul 13, 2018
1 parent 5da46ca commit e647b73
Show file tree
Hide file tree
Showing 4 changed files with 128 additions and 1 deletion.
55 changes: 54 additions & 1 deletion h2/src/main/org/h2/command/dml/Query.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,14 @@
import org.h2.engine.Session;
import org.h2.engine.Mode.ModeEnum;
import org.h2.expression.Alias;
import org.h2.expression.Comparison;
import org.h2.expression.ConditionAndOr;
import org.h2.expression.ConditionNot;
import org.h2.expression.Expression;
import org.h2.expression.ExpressionColumn;
import org.h2.expression.ExpressionVisitor;
import org.h2.expression.Function;
import org.h2.expression.Operation;
import org.h2.expression.Parameter;
import org.h2.expression.ValueExpression;
import org.h2.message.DbException;
Expand Down Expand Up @@ -480,7 +485,9 @@ static void initOrder(Session session,
if (!isAlias) {
if (mustBeInResult) {
if (session.getDatabase().getMode().getEnum() != ModeEnum.MySQL) {
throw DbException.get(ErrorCode.ORDER_BY_NOT_IN_RESULT, e.getSQL());
if (!checkOrderOther(session, e, expressionSQL)) {
throw DbException.get(ErrorCode.ORDER_BY_NOT_IN_RESULT, e.getSQL());
}
}
}
expressions.add(e);
Expand All @@ -492,6 +499,52 @@ static void initOrder(Session session,
}
}

private static boolean checkOrderOther(Session session, Expression expr, ArrayList<String> expressionSQL) {
if (expr.isConstant()) {

This comment has been minimized.

Copy link
@grandinj

grandinj Jul 13, 2018

Contributor

a comment as to the purpose of this method would be nice

return true;
}
if (expressionSQL != null) {
String exprSQL = expr.getSQL();
for (String sql: expressionSQL) {
if (session.getDatabase().equalsIdentifiers(exprSQL, sql)) {
return true;
}
}
}
if (expr instanceof Function) {
Function function = (Function) expr;
if (!function.isDeterministic()) {
return false;
}
for (Expression e : function.getArgs()) {
if (!checkOrderOther(session, e, expressionSQL)) {
return false;
}
}
return true;
}
if (expr instanceof Operation) {
Operation operation = (Operation) expr;
Expression right = operation.getExpression(false);
return checkOrderOther(session, operation.getExpression(true), expressionSQL)
&& (right == null || checkOrderOther(session, right, expressionSQL));
}
if (expr instanceof ConditionAndOr) {
ConditionAndOr condition = (ConditionAndOr) expr;
return checkOrderOther(session, condition.getExpression(true), expressionSQL)
&& checkOrderOther(session, condition.getExpression(false), expressionSQL);
}
if (expr instanceof ConditionNot) {
return checkOrderOther(session, ((ConditionNot) expr).getCondition(), expressionSQL);
}
if (expr instanceof Comparison) {
Comparison condition = (Comparison) expr;
return checkOrderOther(session, condition.getExpression(true), expressionSQL)
&& checkOrderOther(session, condition.getExpression(false), expressionSQL);
}
return false;
}

/**
* Create a {@link SortOrder} object given the list of {@link SelectOrderBy}
* objects. The expression list is extended if necessary.
Expand Down
9 changes: 9 additions & 0 deletions h2/src/main/org/h2/expression/ConditionNot.java
Original file line number Diff line number Diff line change
Expand Up @@ -98,4 +98,13 @@ public int getCost() {
return condition.getCost();
}

/**
* Get the sub-expression of this condition.
*
* @return the sub-expression
*/
public Expression getCondition() {

This comment has been minimized.

Copy link
@grandinj

grandinj Jul 13, 2018

Contributor

rather call this getSubCondition or getSubExpression

return condition;
}

}
11 changes: 11 additions & 0 deletions h2/src/main/org/h2/expression/Operation.java
Original file line number Diff line number Diff line change
Expand Up @@ -407,4 +407,15 @@ public int getCost() {
return left.getCost() + 1 + (right == null ? 0 : right.getCost());
}

/**
* Get the left or the right sub-expression of this condition.
*
* @param getLeft true to get the left sub-expression, false to get the
* right sub-expression.
* @return the sub-expression
*/
public Expression getExpression(boolean getLeft) {

This comment has been minimized.

Copy link
@grandinj

grandinj Jul 13, 2018

Contributor

rather use getLHS and getRHS, otherwise very hard to read the caller code

This comment has been minimized.

Copy link
@katzyn

katzyn Jul 13, 2018

Author Contributor

This method was copied from ConditionAndOr as is. So it should be changed too.

I did not understand what you suggest. Add a couple of methods instead of shared one or what?

This comment has been minimized.

Copy link
@grandinj

grandinj Jul 13, 2018

Contributor

yes, use two methods rather than one is what I meant

return getLeft ? this.left : right;
}

}
54 changes: 54 additions & 0 deletions h2/src/test/org/h2/test/scripts/distinct.sql
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,60 @@ CREATE TABLE TEST2(ID2 BIGINT);
INSERT INTO TEST2 VALUES (1), (2);
> update count: 2

SELECT DISTINCT NAME FROM TEST ORDER BY NAME;
> NAME
> ----
> B
> a
> c
> rows (ordered): 3

SELECT DISTINCT NAME FROM TEST ORDER BY LOWER(NAME);
> NAME
> ----
> a
> B
> c
> rows (ordered): 3

SELECT DISTINCT ID FROM TEST ORDER BY ID;
> ID
> --
> 1
> 2
> 3
> rows (ordered): 3

SELECT DISTINCT ID FROM TEST ORDER BY -ID - 1;
> ID
> --
> 3
> 2
> 1
> rows (ordered): 3

SELECT DISTINCT ID FROM TEST ORDER BY (-ID + 10) > 0 AND NOT (ID = 0), ID;
> ID
> --
> 1
> 2
> 3
> rows (ordered): 3

SELECT DISTINCT NAME, ID + 1 FROM TEST ORDER BY UPPER(NAME) || (ID + 1);
> NAME ID + 1
> ---- ------
> a 2
> B 3
> c 4
> rows (ordered): 3

SELECT DISTINCT ID FROM TEST ORDER BY NAME;
> exception ORDER_BY_NOT_IN_RESULT

SELECT DISTINCT ID FROM TEST ORDER BY CURRENT_TIMESTAMP;
> exception ORDER_BY_NOT_IN_RESULT

SET MODE MySQL;
> ok

Expand Down

0 comments on commit e647b73

Please sign in to comment.