Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQL: Improve the optimization of null conditionals #71192

Merged
merged 2 commits into from
Apr 3, 2021
Merged

SQL: Improve the optimization of null conditionals #71192

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e08210c
SQL: Improve the optimization of null conditionals
costin Apr 1, 2021
24d1410
Update tests
costin Apr 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion ...ql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/NullIf.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,12 @@
*/
public class NullIf extends ConditionalFunction {

private final Expression left, right;

public NullIf(Source source, Expression left, Expression right) {
super(source, Arrays.asList(left, right));
this.left = left;
this.right = right;
}

@Override
Expand All @@ -40,9 +44,25 @@ public Expression replaceChildren(List<Expression> newChildren) {
return new NullIf(source(), newChildren.get(0), newChildren.get(1));
}

public Expression left() {
return left;
}

public Expression right() {
return right;
}

@Override
public boolean foldable() {
return left.semanticEquals(right) || super.foldable();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't also be foldable if left.foldable() and left.fold() == null, but then we need to call left.fold() so I'm not sure we should do that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's covered by the optimizer rule below: https://github.com/elastic/elasticsearch/pull/71192/files#diff-f89f100e88759827c9f9cbceed83a4efb475521a3e42d489584917807df11190R702

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but then we need to call left.fold() so I'm not sure we should do that.

@matriv fmi, why would that be problematic?, if .foldable()?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conceptually you call foldable() beforehand, to check if it's safe to fold() something, so it's weird to call fold() on a child inside the check of foldable().

}

@Override
public Object fold() {
return NullIfProcessor.apply(children().get(0).fold(), children().get(1).fold());
if (left.semanticEquals(right)) {
return null;
}
return NullIfProcessor.apply(left.fold(), right.fold());
}

@Override
Expand Down
46 changes: 32 additions & 14 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,10 @@
import org.elasticsearch.xpack.sql.expression.predicate.conditional.ArbitraryConditionalFunction;
import org.elasticsearch.xpack.sql.expression.predicate.conditional.Case;
import org.elasticsearch.xpack.sql.expression.predicate.conditional.Coalesce;
import org.elasticsearch.xpack.sql.expression.predicate.conditional.ConditionalFunction;
import org.elasticsearch.xpack.sql.expression.predicate.conditional.IfConditional;
import org.elasticsearch.xpack.sql.expression.predicate.conditional.Iif;
import org.elasticsearch.xpack.sql.expression.predicate.conditional.NullIf;
import org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic.Mul;
import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.In;
import org.elasticsearch.xpack.sql.plan.logical.LocalRelation;
Expand Down Expand Up @@ -694,27 +696,43 @@ static class SimplifyConditional extends OptimizerExpressionRule {

@Override
protected Expression rule(Expression e) {
if (e instanceof ArbitraryConditionalFunction) {
ArbitraryConditionalFunction c = (ArbitraryConditionalFunction) e;
if (e instanceof ConditionalFunction) {
List<Expression> children = e.children();
// optimize nullIf
if (e instanceof NullIf) {
NullIf nullIf = (NullIf) e;
if (Expressions.isNull(nullIf.left()) || Expressions.isNull(nullIf.right())) {
return nullIf.left();
}
}
if (e instanceof ArbitraryConditionalFunction) {
ArbitraryConditionalFunction c = (ArbitraryConditionalFunction) e;

// there's no need for a conditional with only one child
if (c instanceof Coalesce) {
if (children.size() == 1) {
return c.children().get(0);
}
}

// exclude any nulls found
List<Expression> newChildren = new ArrayList<>();
for (Expression child : c.children()) {
if (Expressions.isNull(child) == false) {
newChildren.add(child);
// exclude any nulls found
List<Expression> newChildren = new ArrayList<>();
for (Expression child : children) {
if (Expressions.isNull(child) == false) {
newChildren.add(child);

// For Coalesce find the first non-null foldable child (if any) and break early
if (e instanceof Coalesce && child.foldable()) {
break;
// For Coalesce find the first non-null foldable child (if any) and break early
if (e instanceof Coalesce && child.foldable()) {
break;
}
}
}
}

if (newChildren.size() < c.children().size()) {
return c.replaceChildren(newChildren);
if (newChildren.size() < children.size()) {
return c.replaceChildren(newChildren);
}
}
}

return e;
}
}
Expand Down
33 changes: 30 additions & 3 deletions x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@
import static java.util.Collections.emptyMap;
import static java.util.Collections.singletonList;
import static org.elasticsearch.xpack.ql.TestUtils.equalsOf;
import static org.elasticsearch.xpack.ql.TestUtils.fieldAttribute;
import static org.elasticsearch.xpack.ql.TestUtils.greaterThanOf;
import static org.elasticsearch.xpack.ql.TestUtils.greaterThanOrEqualOf;
import static org.elasticsearch.xpack.ql.TestUtils.lessThanOf;
Expand Down Expand Up @@ -307,6 +308,16 @@ public void testMathFolding() {
assertEquals(Math.E, foldFunction(new E(EMPTY)));
}

public void testNullIfFolding() {
assertEquals(null, foldFunction(new NullIf(EMPTY, ONE, ONE)));
assertEquals(1, foldFunction(new NullIf(EMPTY, ONE, TWO)));
assertEquals(2, foldFunction(new NullIf(EMPTY, TWO, ONE)));

FieldAttribute fa = fieldAttribute();
// works even if the expressions are not foldable
assertEquals(null, foldFunction(new NullIf(EMPTY, fa, fa)));
}

private static Object foldFunction(Function f) {
return ((Literal) new ConstantFolding().rule(f)).value();
}
Expand Down Expand Up @@ -439,9 +450,19 @@ public void testSimplifyCoalesceNulls() {
}

public void testSimplifyCoalesceRandomNulls() {
Expression e = new SimplifyConditional().rule(new Coalesce(EMPTY, randomListOfNulls()));
assertEquals(Coalesce.class, e.getClass());
assertEquals(0, e.children().size());
List<Expression> nulls = randomListOfNulls();
Expression e = new SimplifyConditional().rule(new Coalesce(EMPTY, nulls));
if (nulls.size() == 1) {
assertEquals(NULL, e);
} else {
assertEquals(Coalesce.class, e.getClass());
assertEquals(0, e.children().size());
}
}

public void testSimplifyCoalesceWithOnlyOneChild() {
Expression fa = fieldAttribute();
assertSame(fa, new SimplifyConditional().rule(new Coalesce(EMPTY, singletonList(fa))));
}

public void testSimplifyCoalesceRandomNullsWithValue() {
Expand Down Expand Up @@ -494,6 +515,12 @@ public void testFoldNullNotAppliedOnNullIf() {
assertEquals(orig, f);
}

public void testSimplifyNullIf() {
assertEquals(ONE, new SimplifyConditional().rule(new NullIf(EMPTY, ONE, NULL)));
assertEquals(NULL, new SimplifyConditional().rule(new NullIf(EMPTY, NULL, ONE)));
assertEquals(NULL, new SimplifyConditional().rule(new NullIf(EMPTY, NULL, NULL)));
}

public void testSimplifyGreatestNulls() {
Expression e = new SimplifyConditional().rule(new Greatest(EMPTY, asList(NULL, NULL)));
assertEquals(Greatest.class, e.getClass());
Expand Down