Skip to content

Commit

Permalink
Reapply "ESQL: Remove parent from FieldAttribute (#112881)" (#115006) (
Browse files Browse the repository at this point in the history
…#115007)

This reverts commit 17ecb66 and
reapplies #112881 once the
previous, non-backported transport version bump is dealt with.
  • Loading branch information
alex-spies authored Oct 17, 2024
1 parent 441eea7 commit 73ca4f5
Show file tree
Hide file tree
Showing 30 changed files with 323 additions and 134 deletions.
5 changes: 5 additions & 0 deletions docs/changelog/112881.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 112881
summary: "ESQL: Remove parent from `FieldAttribute`"
area: ES|QL
type: enhancement
issues: []
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ static TransportVersion def(int id) {
public static final TransportVersion CONVERT_FAILURE_STORE_OPTIONS_TO_SELECTOR_OPTIONS_INTERNALLY = def(8_772_00_0);
public static final TransportVersion REMOVE_MIN_COMPATIBLE_SHARD_NODE = def(8_773_00_0);
public static final TransportVersion REVERT_REMOVE_MIN_COMPATIBLE_SHARD_NODE = def(8_774_00_0);
public static final TransportVersion ESQL_FIELD_ATTRIBUTE_PARENT_SIMPLIFIED = def(8_775_00_0);

/*
* STOP! READ THIS FIRST! No, really,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.xpack.esql.core.tree.NodeInfo;
import org.elasticsearch.xpack.esql.core.tree.Source;
import org.elasticsearch.xpack.esql.core.type.DataType;
Expand Down Expand Up @@ -42,11 +43,11 @@ public Alias(Source source, String name, Expression child) {
this(source, name, child, null);
}

public Alias(Source source, String name, Expression child, NameId id) {
public Alias(Source source, String name, Expression child, @Nullable NameId id) {
this(source, name, child, id, false);
}

public Alias(Source source, String name, Expression child, NameId id, boolean synthetic) {
public Alias(Source source, String name, Expression child, @Nullable NameId id, boolean synthetic) {
super(source, name, singletonList(child), id, synthetic);
this.child = child;
}
Expand All @@ -55,7 +56,7 @@ public Alias(Source source, String name, Expression child, NameId id, boolean sy
/**
* Old constructor from when this had a qualifier string. Still needed to not break serialization.
*/
private Alias(Source source, String name, String qualifier, Expression child, NameId id, boolean synthetic) {
private Alias(Source source, String name, String qualifier, Expression child, @Nullable NameId id, boolean synthetic) {
this(source, name, child, id, synthetic);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
package org.elasticsearch.xpack.esql.core.expression;

import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.xpack.esql.core.tree.Source;
import org.elasticsearch.xpack.esql.core.type.DataType;

Expand Down Expand Up @@ -41,15 +42,15 @@ public static List<NamedWriteableRegistry.Entry> getNamedWriteables() {
// can the attr be null - typically used in JOINs
private final Nullability nullability;

public Attribute(Source source, String name, NameId id) {
public Attribute(Source source, String name, @Nullable NameId id) {
this(source, name, Nullability.TRUE, id);
}

public Attribute(Source source, String name, Nullability nullability, NameId id) {
public Attribute(Source source, String name, Nullability nullability, @Nullable NameId id) {
this(source, name, nullability, id, false);
}

public Attribute(Source source, String name, Nullability nullability, NameId id, boolean synthetic) {
public Attribute(Source source, String name, Nullability nullability, @Nullable NameId id, boolean synthetic) {
super(source, name, emptyList(), id, synthetic);
this.nullability = nullability;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,25 @@
*/
package org.elasticsearch.xpack.esql.core.expression;

import org.elasticsearch.TransportVersions;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.xpack.esql.core.tree.NodeInfo;
import org.elasticsearch.xpack.esql.core.tree.Source;
import org.elasticsearch.xpack.esql.core.type.DataType;
import org.elasticsearch.xpack.esql.core.type.EsField;
import org.elasticsearch.xpack.esql.core.util.PlanStreamInput;
import org.elasticsearch.xpack.esql.core.util.PlanStreamOutput;
import org.elasticsearch.xpack.esql.core.util.StringUtils;

import java.io.IOException;
import java.util.Objects;

import static org.elasticsearch.xpack.esql.core.util.PlanStreamInput.readCachedStringWithVersionCheck;
import static org.elasticsearch.xpack.esql.core.util.PlanStreamOutput.writeCachedStringWithVersionCheck;

/**
* Attribute for an ES field.
* To differentiate between the different type of fields this class offers:
Expand All @@ -37,32 +41,31 @@ public class FieldAttribute extends TypedAttribute {
FieldAttribute::readFrom
);

private final FieldAttribute parent;
private final String path;
private final String parentName;
private final EsField field;

public FieldAttribute(Source source, String name, EsField field) {
this(source, null, name, field);
}

public FieldAttribute(Source source, FieldAttribute parent, String name, EsField field) {
this(source, parent, name, field, Nullability.TRUE, null, false);
public FieldAttribute(Source source, @Nullable String parentName, String name, EsField field) {
this(source, parentName, name, field, Nullability.TRUE, null, false);
}

public FieldAttribute(Source source, FieldAttribute parent, String name, EsField field, boolean synthetic) {
this(source, parent, name, field, Nullability.TRUE, null, synthetic);
public FieldAttribute(Source source, @Nullable String parentName, String name, EsField field, boolean synthetic) {
this(source, parentName, name, field, Nullability.TRUE, null, synthetic);
}

public FieldAttribute(
Source source,
FieldAttribute parent,
@Nullable String parentName,
String name,
EsField field,
Nullability nullability,
NameId id,
@Nullable NameId id,
boolean synthetic
) {
this(source, parent, name, field.getDataType(), field, nullability, id, synthetic);
this(source, parentName, name, field.getDataType(), field, nullability, id, synthetic);
}

/**
Expand All @@ -71,17 +74,16 @@ public FieldAttribute(
*/
FieldAttribute(
Source source,
FieldAttribute parent,
@Nullable String parentName,
String name,
DataType type,
EsField field,
Nullability nullability,
NameId id,
@Nullable NameId id,
boolean synthetic
) {
super(source, name, type, nullability, id, synthetic);
this.path = parent != null ? parent.name() : StringUtils.EMPTY;
this.parent = parent;
this.parentName = parentName;
this.field = field;
}

Expand All @@ -91,16 +93,16 @@ public FieldAttribute(
*/
private FieldAttribute(
Source source,
FieldAttribute parent,
@Nullable String parentName,
String name,
DataType type,
EsField field,
String qualifier,
@Nullable String qualifier,
Nullability nullability,
NameId id,
@Nullable NameId id,
boolean synthetic
) {
this(source, parent, name, type, field, nullability, id, synthetic);
this(source, parentName, name, type, field, nullability, id, synthetic);
}

private FieldAttribute(StreamInput in) throws IOException {
Expand All @@ -114,8 +116,8 @@ private FieldAttribute(StreamInput in) throws IOException {
*/
this(
Source.readFrom((StreamInput & PlanStreamInput) in),
in.readOptionalWriteable(FieldAttribute::readFrom),
((PlanStreamInput) in).readCachedString(),
readParentName(in),
readCachedStringWithVersionCheck(in),
DataType.readFrom(in),
EsField.readFrom(in),
in.readOptionalString(),
Expand All @@ -129,8 +131,8 @@ private FieldAttribute(StreamInput in) throws IOException {
public void writeTo(StreamOutput out) throws IOException {
if (((PlanStreamOutput) out).writeAttributeCacheHeader(this)) {
Source.EMPTY.writeTo(out);
out.writeOptionalWriteable(parent);
((PlanStreamOutput) out).writeCachedString(name());
writeParentName(out);
writeCachedStringWithVersionCheck(out, name());
dataType().writeTo(out);
field.writeTo(out);
// We used to write the qualifier here. We can still do if needed in the future.
Expand All @@ -145,22 +147,49 @@ public static FieldAttribute readFrom(StreamInput in) throws IOException {
return ((PlanStreamInput) in).readAttributeWithCache(FieldAttribute::new);
}

private void writeParentName(StreamOutput out) throws IOException {
if (out.getTransportVersion().onOrAfter(TransportVersions.ESQL_FIELD_ATTRIBUTE_PARENT_SIMPLIFIED)) {
((PlanStreamOutput) out).writeOptionalCachedString(parentName);
} else {
// Previous versions only used the parent field attribute to retrieve the parent's name, so we can use just any
// fake FieldAttribute here as long as the name is correct.
FieldAttribute fakeParent = parentName() == null ? null : new FieldAttribute(Source.EMPTY, parentName(), field());
out.writeOptionalWriteable(fakeParent);
}
}

private static String readParentName(StreamInput in) throws IOException {
if (in.getTransportVersion().onOrAfter(TransportVersions.ESQL_FIELD_ATTRIBUTE_PARENT_SIMPLIFIED)) {
return ((PlanStreamInput) in).readOptionalCachedString();
}

FieldAttribute parent = in.readOptionalWriteable(FieldAttribute::readFrom);
return parent == null ? null : parent.name();
}

@Override
public String getWriteableName() {
return ENTRY.name;
}

@Override
protected NodeInfo<FieldAttribute> info() {
return NodeInfo.create(this, FieldAttribute::new, parent, name(), dataType(), field, (String) null, nullable(), id(), synthetic());
}

public FieldAttribute parent() {
return parent;
return NodeInfo.create(
this,
FieldAttribute::new,
parentName,
name(),
dataType(),
field,
(String) null,
nullable(),
id(),
synthetic()
);
}

public String path() {
return path;
public String parentName() {
return parentName;
}

/**
Expand All @@ -174,7 +203,7 @@ public String fieldName() {
if ((synthetic() || name().startsWith(SYNTHETIC_ATTRIBUTE_NAME_PREFIX)) == false) {
return name();
}
return Strings.hasText(path) ? path + "." + field.getName() : field.getName();
return Strings.hasText(parentName) ? parentName + "." + field.getName() : field.getName();
}

public EsField.Exact getExactInfo() {
Expand All @@ -190,13 +219,13 @@ public FieldAttribute exactAttribute() {
}

private FieldAttribute innerField(EsField type) {
return new FieldAttribute(source(), this, name() + "." + type.getName(), type, nullable(), id(), synthetic());
return new FieldAttribute(source(), name(), name() + "." + type.getName(), type, nullable(), id(), synthetic());
}

@Override
protected Attribute clone(Source source, String name, DataType type, Nullability nullability, NameId id, boolean synthetic) {
// Ignore `type`, this must be the same as the field's type.
return new FieldAttribute(source, parent, name, field, nullability, id, synthetic);
return new FieldAttribute(source, parentName, name, field, nullability, id, synthetic);
}

@Override
Expand All @@ -206,13 +235,13 @@ public Attribute withDataType(DataType type) {

@Override
public int hashCode() {
return Objects.hash(super.hashCode(), path, field);
return Objects.hash(super.hashCode(), parentName, field);
}

@Override
public boolean equals(Object obj) {
return super.equals(obj)
&& Objects.equals(path, ((FieldAttribute) obj).path)
&& Objects.equals(parentName, ((FieldAttribute) obj).parentName)
&& Objects.equals(field, ((FieldAttribute) obj).field);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.core.Tuple;
import org.elasticsearch.index.mapper.IdFieldMapper;
import org.elasticsearch.index.mapper.IgnoredFieldMapper;
Expand Down Expand Up @@ -59,7 +60,7 @@ public MetadataAttribute(
String name,
DataType dataType,
Nullability nullability,
NameId id,
@Nullable NameId id,
boolean synthetic,
boolean searchable
) {
Expand All @@ -79,9 +80,9 @@ private MetadataAttribute(
Source source,
String name,
DataType dataType,
String qualifier,
@Nullable String qualifier,
Nullability nullability,
NameId id,
@Nullable NameId id,
boolean synthetic,
boolean searchable
) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import org.elasticsearch.common.io.stream.NamedWriteable;
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.xpack.esql.core.tree.Source;

import java.util.ArrayList;
Expand All @@ -32,11 +33,11 @@ public static List<NamedWriteableRegistry.Entry> getNamedWriteables() {
private final NameId id;
private final boolean synthetic;

public NamedExpression(Source source, String name, List<Expression> children, NameId id) {
public NamedExpression(Source source, String name, List<Expression> children, @Nullable NameId id) {
this(source, name, children, id, false);
}

public NamedExpression(Source source, String name, List<Expression> children, NameId id, boolean synthetic) {
public NamedExpression(Source source, String name, List<Expression> children, @Nullable NameId id, boolean synthetic) {
super(source, children);
this.name = name;
this.id = id == null ? new NameId() : id;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.xpack.esql.core.tree.NodeInfo;
import org.elasticsearch.xpack.esql.core.tree.Source;
import org.elasticsearch.xpack.esql.core.type.DataType;
Expand All @@ -31,7 +32,14 @@ public ReferenceAttribute(Source source, String name, DataType dataType) {
this(source, name, dataType, Nullability.FALSE, null, false);
}

public ReferenceAttribute(Source source, String name, DataType dataType, Nullability nullability, NameId id, boolean synthetic) {
public ReferenceAttribute(
Source source,
String name,
DataType dataType,
Nullability nullability,
@Nullable NameId id,
boolean synthetic
) {
super(source, name, dataType, nullability, id, synthetic);
}

Expand Down
Loading

0 comments on commit 73ca4f5

Please sign in to comment.