Skip to content

SQL: Small code improvements of Pipes & Processors #40909

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

Merged
merged 1 commit into from
Apr 8, 2019
Merged

SQL: Small code improvements of Pipes & Processors #40909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 3 additions & 9 deletions ...ck/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/pipeline/Pipe.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@
import org.elasticsearch.xpack.sql.expression.Attribute;
import org.elasticsearch.xpack.sql.expression.Expression;
import org.elasticsearch.xpack.sql.expression.gen.processor.Processor;
import org.elasticsearch.xpack.sql.tree.Source;
import org.elasticsearch.xpack.sql.tree.Node;
import org.elasticsearch.xpack.sql.tree.Source;

import java.util.ArrayList;
import java.util.List;
Expand Down Expand Up @@ -53,13 +53,7 @@ public void collectFields(SqlSourceBuilder sourceBuilder) {

@Override
public boolean supportedByAggsOnlyQuery() {
for (Pipe pipe : children()) {
if (pipe.supportedByAggsOnlyQuery()) {
return true;
}
}

return false;
return children().stream().anyMatch(Pipe::supportedByAggsOnlyQuery);
Copy link
Member

Choose a reason for hiding this comment

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

Not a fan of streams.

}

public abstract Processor asProcessor();
Expand All @@ -83,4 +77,4 @@ public Pipe resolveAttributes(AttributeResolver resolver) {
public interface AttributeResolver {
FieldExtraction resolve(Attribute attribute);
}
}
}
19 changes: 1 addition & 18 deletions ...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 @@ -8,13 +8,11 @@

import org.elasticsearch.xpack.sql.expression.Expression;
import org.elasticsearch.xpack.sql.expression.Expressions;
import org.elasticsearch.xpack.sql.expression.Nullability;
import org.elasticsearch.xpack.sql.expression.gen.pipeline.Pipe;
import org.elasticsearch.xpack.sql.expression.gen.script.ParamsBuilder;
import org.elasticsearch.xpack.sql.expression.gen.script.ScriptTemplate;
import org.elasticsearch.xpack.sql.tree.Source;
import org.elasticsearch.xpack.sql.tree.NodeInfo;
import org.elasticsearch.xpack.sql.type.DataType;
import org.elasticsearch.xpack.sql.tree.Source;

import java.util.Arrays;
import java.util.List;
Expand Down Expand Up @@ -47,21 +45,6 @@ protected TypeResolution resolveType() {
return TypeResolution.TYPE_RESOLVED;
}

@Override
public DataType dataType() {
return dataType;
}

@Override
public boolean foldable() {
return Expressions.foldable(children());
}

@Override
public Nullability nullable() {
return Nullability.UNKNOWN;
}

@Override
public Object fold() {
return NullIfProcessor.apply(children().get(0).fold(), children().get(1).fold());
Expand Down
8 changes: 6 additions & 2 deletions ...in/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/NullIfProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,12 @@ public static Object apply(Object leftValue, Object rightValue) {

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
NullIfProcessor that = (NullIfProcessor) o;
return Objects.equals(leftProcessor, that.leftProcessor) &&
Objects.equals(rightProcessor, that.rightProcessor);
Expand Down
48 changes: 9 additions & 39 deletions ...ain/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/InPipe.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,26 +5,20 @@
*/
package org.elasticsearch.xpack.sql.expression.predicate.operator.comparison;

import org.elasticsearch.xpack.sql.capabilities.Resolvables;
import org.elasticsearch.xpack.sql.execution.search.FieldExtraction;
import org.elasticsearch.xpack.sql.execution.search.SqlSourceBuilder;
import org.elasticsearch.xpack.sql.expression.Expression;
import org.elasticsearch.xpack.sql.expression.gen.pipeline.MultiPipe;
import org.elasticsearch.xpack.sql.expression.gen.pipeline.Pipe;
import org.elasticsearch.xpack.sql.tree.Source;
import org.elasticsearch.xpack.sql.expression.gen.processor.Processor;
import org.elasticsearch.xpack.sql.tree.NodeInfo;
import org.elasticsearch.xpack.sql.tree.Source;

import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
import java.util.stream.Collectors;

public class InPipe extends Pipe {

private List<Pipe> pipes;
public class InPipe extends MultiPipe {

public InPipe(Source source, Expression expression, List<Pipe> pipes) {
super(source, expression, pipes);
this.pipes = pipes;
}

@Override
Expand All @@ -37,36 +31,12 @@ public final Pipe replaceChildren(List<Pipe> newChildren) {

@Override
protected NodeInfo<InPipe> info() {
return NodeInfo.create(this, InPipe::new, expression(), pipes);
}

@Override
public boolean supportedByAggsOnlyQuery() {
return pipes.stream().allMatch(FieldExtraction::supportedByAggsOnlyQuery);
}

@Override
public final Pipe resolveAttributes(AttributeResolver resolver) {
List<Pipe> newPipes = new ArrayList<>(pipes.size());
for (Pipe p : pipes) {
newPipes.add(p.resolveAttributes(resolver));
}
return replaceChildren(newPipes);
}

@Override
public boolean resolved() {
return Resolvables.resolved(pipes);
}

@Override
public final void collectFields(SqlSourceBuilder sourceBuilder) {
pipes.forEach(p -> p.collectFields(sourceBuilder));
return NodeInfo.create(this, InPipe::new, expression(), children());
}

@Override
public int hashCode() {
return Objects.hash(pipes);
return Objects.hash(children());
}

@Override
Expand All @@ -80,11 +50,11 @@ public boolean equals(Object obj) {
}

InPipe other = (InPipe) obj;
return Objects.equals(pipes, other.pipes);
return Objects.equals(children(), other.children());
}

@Override
public InProcessor asProcessor() {
return new InProcessor(pipes.stream().map(Pipe::asProcessor).collect(Collectors.toList()));
public InProcessor asProcessor(List<Processor> processors) {
return new InProcessor(processors);
}
}
35 changes: 26 additions & 9 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 @@ -43,7 +43,9 @@
import org.elasticsearch.xpack.sql.expression.function.scalar.string.Repeat;
import org.elasticsearch.xpack.sql.expression.predicate.BinaryOperator;
import org.elasticsearch.xpack.sql.expression.predicate.Range;
import org.elasticsearch.xpack.sql.expression.predicate.conditional.ArbitraryConditionalFunction;
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.Greatest;
import org.elasticsearch.xpack.sql.expression.predicate.conditional.IfNull;
import org.elasticsearch.xpack.sql.expression.predicate.conditional.Least;
Expand Down Expand Up @@ -97,6 +99,7 @@
import org.elasticsearch.xpack.sql.util.CollectionUtils;
import org.elasticsearch.xpack.sql.util.StringUtils;

import java.lang.reflect.Constructor;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
Expand Down Expand Up @@ -445,18 +448,32 @@ public void testNullFoldingDoesNotApplyOnLogicalExpressions() {
assertEquals(and, rule.rule(and));
}

public void testNullFoldingDoesNotApplyOnConditionals() {
@SuppressWarnings("unchecked")
public void testNullFoldingDoesNotApplyOnConditionals() throws Exception {
FoldNull rule = new FoldNull();

Coalesce coalesce = new Coalesce(EMPTY, Arrays.asList(Literal.NULL, ONE, TWO));
assertEquals(coalesce, rule.rule(coalesce));
coalesce = new Coalesce(EMPTY, Arrays.asList(Literal.NULL, NULL, NULL));
assertEquals(coalesce, rule.rule(coalesce));
Class<ConditionalFunction> clazz =
(Class<ConditionalFunction>) randomFrom(IfNull.class, NullIf.class);
Constructor<ConditionalFunction> ctor = clazz.getConstructor(Source.class, Expression.class, Expression.class);
ConditionalFunction conditionalFunction = ctor.newInstance(EMPTY, Literal.NULL, ONE);
assertEquals(conditionalFunction, rule.rule(conditionalFunction));
conditionalFunction = ctor.newInstance(EMPTY, ONE, Literal.NULL);
assertEquals(conditionalFunction, rule.rule(conditionalFunction));
conditionalFunction = ctor.newInstance(EMPTY, Literal.NULL, Literal.NULL);
assertEquals(conditionalFunction, rule.rule(conditionalFunction));
}

@SuppressWarnings("unchecked")
public void testNullFoldingDoesNotApplyOnArbitraryConditionals() throws Exception {
FoldNull rule = new FoldNull();

Greatest greatest = new Greatest(EMPTY, Arrays.asList(Literal.NULL, ONE, TWO));
assertEquals(greatest, rule.rule(greatest));
greatest = new Greatest(EMPTY, Arrays.asList(Literal.NULL, ONE, TWO));
assertEquals(greatest, rule.rule(greatest));
Class<ArbitraryConditionalFunction> clazz =
(Class<ArbitraryConditionalFunction>) randomFrom(Coalesce.class, Greatest.class, Least.class);
Constructor<ArbitraryConditionalFunction> ctor = clazz.getConstructor(Source.class, List.class);
ArbitraryConditionalFunction conditionalFunction = ctor.newInstance(EMPTY, Arrays.asList(Literal.NULL, ONE, TWO));
assertEquals(conditionalFunction, rule.rule(conditionalFunction));
conditionalFunction = ctor.newInstance(EMPTY, Arrays.asList(Literal.NULL, NULL, NULL));
assertEquals(conditionalFunction, rule.rule(conditionalFunction));
}

public void testSimplifyCoalesceNulls() {
Expand Down