Skip to content

SQL: [Tests] Fix replaceChildren of Pivot #49004

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

Closed
wants to merge 1 commit into from
Closed

SQL: [Tests] Fix replaceChildren of Pivot #49004

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
5 changes: 5 additions & 0 deletions .../plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/UnresolvedAttribute.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,11 @@ public String unresolvedMessage() {
return unresolvedMsg;
}

@Override
Copy link
Member

Choose a reason for hiding this comment

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

A simple fix yet incorrect since an unresolved attribute should be first resolved before being manipulated.
It's way none of the with methods are implemented - touching an unresolved attribute is incorrect period.

public Attribute withQualifier(String qualifier) {
return this;
}

public static String errorMessage(String name, List<String> potentialMatches) {
String msg = "Unknown column [" + name + "]";
if (!CollectionUtils.isEmpty(potentialMatches)) {
Expand Down
22 changes: 7 additions & 15 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/Pivot.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,6 @@ public Pivot(Source source, LogicalPlan child, Expression column, List<NamedExpr
this.aggregates = aggregates;
}

private static Expression withQualifierNull(Expression e) {
if (e instanceof Attribute) {
Attribute fa = (Attribute) e;
return fa.withQualifier(null);
}
return e;
}

@Override
protected NodeInfo<Pivot> info() {
Copy link
Member

Choose a reason for hiding this comment

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

I haven't reviewed the previous PR so there's not much to do at this point.
I find the transformation inside info an ugly hack - from a simple copy it became transformer that overrides qualifiers.
What if the relationship has qualifiers in it?

return NodeInfo.create(this, Pivot::new, child(), column, values, aggregates);
Expand All @@ -65,9 +57,9 @@ protected Pivot replaceChild(LogicalPlan newChild) {
// to be changed to null like the attributes of EsRelation
// otherwise they don't equal and aren't removed
// when calculating the groupingSet
newColumn = column.transformUp(Pivot::withQualifierNull);
newColumn = column.transformUp(a -> a.withQualifier(null), Attribute.class);
newAggregates = aggregates.stream().map((NamedExpression aggregate) ->
(NamedExpression) aggregate.transformUp(Pivot::withQualifierNull)
(NamedExpression) aggregate.transformUp(a -> a.withQualifier(null), Attribute.class)
).collect(Collectors.toUnmodifiableList());
}

Expand All @@ -85,7 +77,7 @@ public List<NamedExpression> values() {
public List<NamedExpression> aggregates() {
return aggregates;
}

public AttributeSet groupingSet() {
if (groupingSet == null) {
AttributeSet columnSet = Expressions.references(singletonList(column));
Expand Down Expand Up @@ -125,7 +117,7 @@ public AttributeSet valuesOutput() {
}
return valueOutput;
}

@Override
public List<Attribute> output() {
if (output == null) {
Expand All @@ -146,17 +138,17 @@ public boolean expressionsResolved() {
public int hashCode() {
return Objects.hash(column, values, aggregates, child());
}

@Override
public boolean equals(Object obj) {
if (this == obj) {
return true;
}

if (obj == null || getClass() != obj.getClass()) {
return false;
}

Pivot other = (Pivot) obj;
return Objects.equals(column, other.column)
&& Objects.equals(values, other.values)
Expand Down