-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Esql dimension aware attributes #131463
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 dimension aware attributes #131463
Changes from all commits
05a4561
5f8e350
0c0b5bd
9d79b2f
2f9e7d2
c11fa12
28a3aea
001047d
3397960
038f743
d726c75
37849ed
cf48733
f246efa
3703533
47df3b4
5250dce
c7dfe13
ec5dfd4
2f638e7
1a16702
e7b7e7c
1b1afe5
89c2054
3b4dc17
9ab5c0f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -117,4 +117,9 @@ protected NodeInfo<ReferenceAttribute> info() { | |
protected String label() { | ||
return "r"; | ||
} | ||
|
||
@Override | ||
public boolean isDimension() { | ||
return false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thought: it might get tricky when dealing with renames; if you have a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My understanding from @kkrik-es is that, at least for the initial version, that's an acceptable restriction. I think ReferenceAttributes are used for the output of EVAL and similar commands, and at least for the initial version of this we do not want to treat those as dimensions. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -107,6 +107,11 @@ protected String label() { | |
return UNRESOLVED_PREFIX; | ||
} | ||
|
||
@Override | ||
public boolean isDimension() { | ||
return false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thought: maybe this should throw UOE, because we can't determine this on an unresolved attr. |
||
} | ||
|
||
@Override | ||
public String nodeString() { | ||
return toString(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
package org.elasticsearch.xpack.esql.core.type; | ||
|
||
import org.elasticsearch.TransportVersions; | ||
import org.elasticsearch.action.fieldcaps.IndexFieldCapabilities; | ||
import org.elasticsearch.common.io.stream.StreamInput; | ||
import org.elasticsearch.common.io.stream.StreamOutput; | ||
import org.elasticsearch.common.io.stream.Writeable; | ||
|
@@ -31,10 +32,36 @@ public class EsField implements Writeable { | |
* roles within the ESQL query processing pipeline. | ||
*/ | ||
public enum TimeSeriesFieldType implements Writeable { | ||
UNKNOWN(0), | ||
NONE(1), | ||
METRIC(2), | ||
DIMENSION(3); | ||
UNKNOWN(0) { | ||
@Override | ||
public TimeSeriesFieldType merge(TimeSeriesFieldType other) { | ||
return other; | ||
} | ||
}, | ||
NONE(1) { | ||
@Override | ||
public TimeSeriesFieldType merge(TimeSeriesFieldType other) { | ||
return other; | ||
} | ||
}, | ||
METRIC(2) { | ||
@Override | ||
public TimeSeriesFieldType merge(TimeSeriesFieldType other) { | ||
if (other != DIMENSION) { | ||
return METRIC; | ||
} | ||
throw new IllegalStateException("Time Series Metadata conflict. Cannot merge [" + other + "] with [METRIC]."); | ||
} | ||
}, | ||
DIMENSION(3) { | ||
@Override | ||
public TimeSeriesFieldType merge(TimeSeriesFieldType other) { | ||
if (other != METRIC) { | ||
return DIMENSION; | ||
} | ||
throw new IllegalStateException("Time Series Metadata conflict. Cannot merge [" + other + "] with [DIMENSION]."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To confirm, we do not allow mixing dimensions and metrics, but we do allow NONE with dimensions. I think the GAUGE type is optional - should we treat it similarly to NONE? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My main concern here is allowing for a migration from a label to a dimension. Labels would have neither a metric nor a dimension type, and would show up as NONE here, but if a later mapping marks them as dimensions, we should pick that up. |
||
} | ||
}; | ||
|
||
private final int id; | ||
|
||
|
@@ -57,6 +84,19 @@ public static TimeSeriesFieldType readFromStream(StreamInput in) throws IOExcept | |
default -> throw new IOException("Unexpected value for TimeSeriesFieldType: " + id); | ||
}; | ||
} | ||
|
||
public static TimeSeriesFieldType fromIndexFieldCapabilities(IndexFieldCapabilities capabilities) { | ||
if (capabilities.isDimension()) { | ||
assert capabilities.metricType() == null; | ||
return DIMENSION; | ||
} | ||
if (capabilities.metricType() != null) { | ||
return METRIC; | ||
} | ||
return NONE; | ||
} | ||
|
||
public abstract TimeSeriesFieldType merge(TimeSeriesFieldType other); | ||
} | ||
|
||
private static Map<String, Reader<? extends EsField>> readers = Map.ofEntries( | ||
|
@@ -83,13 +123,8 @@ public static Reader<? extends EsField> getReader(String name) { | |
private final Map<String, EsField> properties; | ||
private final String name; | ||
private final boolean isAlias; | ||
// Because the subclasses all reimplement serialization, this needs to be writeable from subclass constructors | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Comment was outdated - I ended up not reimplementing the serialization in the sub classes, but forgot to delete this in the previous PR) |
||
private final TimeSeriesFieldType timeSeriesFieldType; | ||
|
||
public EsField(String name, DataType esDataType, Map<String, EsField> properties, boolean aggregatable) { | ||
this(name, esDataType, properties, aggregatable, false, TimeSeriesFieldType.UNKNOWN); | ||
} | ||
|
||
public EsField( | ||
String name, | ||
DataType esDataType, | ||
|
@@ -100,10 +135,6 @@ public EsField( | |
this(name, esDataType, properties, aggregatable, false, timeSeriesFieldType); | ||
} | ||
|
||
public EsField(String name, DataType esDataType, Map<String, EsField> properties, boolean aggregatable, boolean isAlias) { | ||
this(name, esDataType, properties, aggregatable, isAlias, TimeSeriesFieldType.UNKNOWN); | ||
} | ||
|
||
public EsField( | ||
String name, | ||
DataType esDataType, | ||
|
@@ -247,6 +278,10 @@ public Exact getExactInfo() { | |
return Exact.EXACT_FIELD; | ||
} | ||
|
||
public TimeSeriesFieldType getTimeSeriesFieldType() { | ||
return timeSeriesFieldType; | ||
} | ||
|
||
@Override | ||
public String toString() { | ||
return name + "@" + esDataType.typeName() + "=" + properties; | ||
|
@@ -265,12 +300,13 @@ public boolean equals(Object o) { | |
&& isAlias == field.isAlias | ||
&& esDataType == field.esDataType | ||
&& Objects.equals(name, field.name) | ||
&& Objects.equals(properties, field.properties); | ||
&& Objects.equals(properties, field.properties) | ||
&& Objects.equals(timeSeriesFieldType, field.timeSeriesFieldType); | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return Objects.hash(esDataType, aggregatable, properties, name, isAlias); | ||
return Objects.hash(esDataType, aggregatable, properties, name, isAlias, timeSeriesFieldType); | ||
} | ||
|
||
public static final class Exact { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should actually be ternary, resp. return
Boolean
(capitalB
) so that attributes can returnnull
to say "I don't know if I refer to a dimension" rather than claiming not to.Otherwise 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be the behavior associated with the null version? The intention here is that we want to only allow filtering by dimensions, and enable automatic grouping by all dimensions. If a field returns null here, we would then need to decide if "null" behaved as "this is a dimension" or "this is not a dimension" for these cases.
Unless you're proposing that we fail (or null-with-warning) any query where there is ambiguity about the dimensions? I'm not sure I agree with that.