Skip to content

Commit

Permalink
SQL: Bug fix for the optional "start" parameter usage inside LOCATE f…
Browse files Browse the repository at this point in the history 
…unction (#32576)

The incorrect NodeInfo is created when the optional parameter is not used, leading to the incorrect constructor being used. Simplified LocateFunctionProcessorDefinition by using one constructor instead of two.
Fixes #32554
  • Loading branch information
@astefan
astefan authored Aug 9, 2018
1 parent 6750e15 commit 7b618f3
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 74 deletions.
18 changes: 4 additions & 14 deletions ...l/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/Locate.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,20 +62,10 @@ protected TypeResolution resolveType() {

@Override
protected ProcessorDefinition makeProcessorDefinition() {
LocateFunctionProcessorDefinition processorDefinition;
if (start == null) {
processorDefinition = new LocateFunctionProcessorDefinition(location(), this,
ProcessorDefinitions.toProcessorDefinition(pattern),
ProcessorDefinitions.toProcessorDefinition(source));
}
else {
processorDefinition = new LocateFunctionProcessorDefinition(location(), this,
ProcessorDefinitions.toProcessorDefinition(pattern),
ProcessorDefinitions.toProcessorDefinition(source),
ProcessorDefinitions.toProcessorDefinition(start));
}

return processorDefinition;
return new LocateFunctionProcessorDefinition(location(), this,
ProcessorDefinitions.toProcessorDefinition(pattern),
ProcessorDefinitions.toProcessorDefinition(source),
start == null ? null : ProcessorDefinitions.toProcessorDefinition(start));
}

@Override
Expand Down
13 changes: 1 addition & 12 deletions ...search/xpack/sql/expression/function/scalar/string/LocateFunctionProcessorDefinition.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,20 +21,12 @@ public class LocateFunctionProcessorDefinition extends ProcessorDefinition {

public LocateFunctionProcessorDefinition(Location location, Expression expression, ProcessorDefinition pattern,
ProcessorDefinition source, ProcessorDefinition start) {
super(location, expression, Arrays.asList(pattern, source, start));
super(location, expression, start == null ? Arrays.asList(pattern, source) : Arrays.asList(pattern, source, start));
this.pattern = pattern;
this.source = source;
this.start = start;
}

public LocateFunctionProcessorDefinition(Location location, Expression expression, ProcessorDefinition pattern,
ProcessorDefinition source) {
super(location, expression, Arrays.asList(pattern, source));
this.pattern = pattern;
this.source = source;
this.start = null;
}

@Override
public final ProcessorDefinition replaceChildren(List<ProcessorDefinition> newChildren) {
int childrenSize = newChildren.size();
Expand Down Expand Up @@ -68,9 +60,6 @@ public boolean resolved() {

protected ProcessorDefinition replaceChildren(ProcessorDefinition newPattern, ProcessorDefinition newSource,
ProcessorDefinition newStart) {
if (newStart == null) {
return new LocateFunctionProcessorDefinition(location(), expression(), newPattern, newSource);
}
return new LocateFunctionProcessorDefinition(location(), expression(), newPattern, newSource, newStart);
}

Expand Down
69 changes: 21 additions & 48 deletions ...h/xpack/sql/expression/function/scalar/string/LocateFunctionProcessorDefinitionTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,50 +38,34 @@ public static LocateFunctionProcessorDefinition randomLocateFunctionProcessorDef
return (LocateFunctionProcessorDefinition) (new Locate(randomLocation(),
randomStringLiteral(),
randomStringLiteral(),
frequently() ? randomIntLiteral() : null)
randomFrom(true, false) ? randomIntLiteral() : null)
.makeProcessorDefinition());
}

@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/32554")
@Override
public void testTransform() {
// test transforming only the properties (location, expression),
// skipping the children (the two parameters of the binary function) which are tested separately
LocateFunctionProcessorDefinition b1 = randomInstance();
Expression newExpression = randomValueOtherThan(b1.expression(), () -> randomLocateFunctionExpression());
LocateFunctionProcessorDefinition newB;
if (b1.start() == null) {
newB = new LocateFunctionProcessorDefinition(
b1.location(),
newExpression,
b1.pattern(),
b1.source());
} else {
newB = new LocateFunctionProcessorDefinition(
b1.location(),
newExpression,
b1.pattern(),
b1.source(),
b1.start());
}
LocateFunctionProcessorDefinition newB = new LocateFunctionProcessorDefinition(
b1.location(),
newExpression,
b1.pattern(),
b1.source(),
b1.start());

assertEquals(newB, b1.transformPropertiesOnly(v -> Objects.equals(v, b1.expression()) ? newExpression : v, Expression.class));

LocateFunctionProcessorDefinition b2 = randomInstance();
Location newLoc = randomValueOtherThan(b2.location(), () -> randomLocation());
if (b2.start() == null) {
newB = new LocateFunctionProcessorDefinition(
newLoc,
b2.expression(),
b2.pattern(),
b2.source());
} else {
newB = new LocateFunctionProcessorDefinition(
newLoc,
b2.expression(),
b2.pattern(),
b2.source(),
b2.start());
}
newB = new LocateFunctionProcessorDefinition(
newLoc,
b2.expression(),
b2.pattern(),
b2.source(),
b2.start());

assertEquals(newB,
b2.transformPropertiesOnly(v -> Objects.equals(v, b2.location()) ? newLoc : v, Location.class));
}
Expand All @@ -93,15 +77,9 @@ public void testReplaceChildren() {
ProcessorDefinition newSource = toProcessorDefinition((Expression) randomValueOtherThan(b.source(), () -> randomStringLiteral()));
ProcessorDefinition newStart;

LocateFunctionProcessorDefinition newB;
if (b.start() == null) {
newB = new LocateFunctionProcessorDefinition(b.location(), b.expression(), b.pattern(), b.source());
newStart = null;
}
else {
newB = new LocateFunctionProcessorDefinition(b.location(), b.expression(), b.pattern(), b.source(), b.start());
newStart = toProcessorDefinition((Expression) randomValueOtherThan(b.start(), () -> randomIntLiteral()));
}
LocateFunctionProcessorDefinition newB = new LocateFunctionProcessorDefinition(
b.location(), b.expression(), b.pattern(), b.source(), b.start());
newStart = toProcessorDefinition((Expression) randomValueOtherThan(b.start(), () -> randomIntLiteral()));
LocateFunctionProcessorDefinition transformed = null;

// generate all the combinations of possible children modifications and test all of them
Expand Down Expand Up @@ -132,7 +110,8 @@ protected LocateFunctionProcessorDefinition mutate(LocateFunctionProcessorDefini
comb.get(0) ? toProcessorDefinition((Expression) randomValueOtherThan(f.pattern(),
() -> randomStringLiteral())) : f.pattern(),
comb.get(1) ? toProcessorDefinition((Expression) randomValueOtherThan(f.source(),
() -> randomStringLiteral())) : f.source()));
() -> randomStringLiteral())) : f.source(),
null));
}
}
} else {
Expand All @@ -155,13 +134,7 @@ protected LocateFunctionProcessorDefinition mutate(LocateFunctionProcessorDefini

@Override
protected LocateFunctionProcessorDefinition copy(LocateFunctionProcessorDefinition instance) {
return instance.start() == null ?
new LocateFunctionProcessorDefinition(instance.location(),
instance.expression(),
instance.pattern(),
instance.source())
:
new LocateFunctionProcessorDefinition(instance.location(),
return new LocateFunctionProcessorDefinition(instance.location(),
instance.expression(),
instance.pattern(),
instance.source(),
Expand Down

0 comments on commit 7b618f3

Please sign in to comment.