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.

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

ESQL: Remove parent from FieldAttribute #112881

Merged
merged 18 commits into from
Oct 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 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 @@ -175,6 +175,7 @@ static TransportVersion def(int id) {
public static final TransportVersion ML_INFERENCE_ATTACH_TO_EXISTSING_DEPLOYMENT = def(8_771_00_0);
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 ESQL_FIELD_ATTRIBUTE_PARENT_SIMPLIFIED = def(8_774_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,
alex-spies marked this conversation as resolved.
Show resolved Hide resolved
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