Skip to content
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
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,17 @@
import org.elasticsearch.xpack.esql.common.Failures;
import org.elasticsearch.xpack.esql.core.capabilities.Unresolvable;
import org.elasticsearch.xpack.esql.core.expression.Alias;
import org.elasticsearch.xpack.esql.core.expression.Attribute;
import org.elasticsearch.xpack.esql.core.expression.Expression;
import org.elasticsearch.xpack.esql.core.expression.MetadataAttribute;
import org.elasticsearch.xpack.esql.core.expression.TypeResolutions;
import org.elasticsearch.xpack.esql.core.expression.UnresolvedTimestamp;
import org.elasticsearch.xpack.esql.core.expression.function.Function;
import org.elasticsearch.xpack.esql.core.expression.predicate.operator.comparison.BinaryComparison;
import org.elasticsearch.xpack.esql.core.tree.Node;
import org.elasticsearch.xpack.esql.core.type.DataType;
import org.elasticsearch.xpack.esql.core.util.Holder;
import org.elasticsearch.xpack.esql.expression.function.TimestampAware;
import org.elasticsearch.xpack.esql.expression.function.UnsupportedAttribute;
import org.elasticsearch.xpack.esql.expression.function.fulltext.FullTextFunction;
import org.elasticsearch.xpack.esql.expression.predicate.operator.arithmetic.Neg;
Expand Down Expand Up @@ -69,6 +72,8 @@
*/
public class Verifier {

static final String UNMAPPED_TIMESTAMP_SUFFIX = "; the [unmapped_fields] setting does not apply to the implicit @timestamp reference";

/**
* Extra plan verification checks defined in plugins.
*/
Expand Down Expand Up @@ -104,9 +109,10 @@ Collection<Failure> verify(LogicalPlan plan, BitSet partialMetrics) {
Collection<Failure> verify(LogicalPlan plan, BitSet partialMetrics, UnmappedResolution unmappedResolution) {
assert partialMetrics != null;
Failures failures = new Failures();
boolean unmappedTimestampHandled = unmappedResolution != UnmappedResolution.FAIL && isTimestampUnmappedInAllIndices(plan, failures);

// quick verification for unresolved attributes
checkUnresolvedAttributes(plan, failures);
checkUnresolvedAttributes(plan, failures, unmappedTimestampHandled);

ConfigurationAware.verifyNoMarkerConfiguration(plan, failures);

Expand Down Expand Up @@ -158,7 +164,7 @@ Collection<Failure> verify(LogicalPlan plan, BitSet partialMetrics, UnmappedReso
return failures.failures();
}

private static void checkUnresolvedAttributes(LogicalPlan plan, Failures failures) {
private static void checkUnresolvedAttributes(LogicalPlan plan, Failures failures, boolean skipUnresolvedTimestamp) {
plan.forEachUp(p -> {
// if the children are unresolved, so will this node; counting it will only add noise
if (p.childrenResolved() == false) {
Expand Down Expand Up @@ -191,6 +197,9 @@ else if (p.resolved()) {
return;
}

if (skipUnresolvedTimestamp && ae instanceof UnresolvedTimestamp) {
return;
}
if (ae instanceof Unresolvable u) {
failures.add(fail(ae, u.unresolvedMessage()));
}
Expand Down Expand Up @@ -363,6 +372,38 @@ private static void checkLimitBy(LogicalPlan plan, Failures failures) {
}
}

/**
* The {@code unmapped_fields} setting does not apply to the implicit {@code @timestamp} reference ({@link TimestampAware} functions).
* Only emits the specific message when {@code @timestamp} is truly absent from all source index mappings;
* if the field was present but dropped/renamed by the query, the generic unresolved-attribute message is more appropriate.
* See https://github.com/elastic/elasticsearch/issues/142127
*/
private static boolean isTimestampUnmappedInAllIndices(LogicalPlan plan, Failures failures) {
if (plan.anyMatch(p -> p instanceof EsRelation r && r.indexMode() != IndexMode.LOOKUP && hasTimestamp(r))) {
return false;
}
plan.forEachDown(p -> {
if (p instanceof TimestampAware ta && ta.timestamp() instanceof UnresolvedTimestamp) {
failures.add(fail(p, "[{}] " + UnresolvedTimestamp.UNRESOLVED_SUFFIX + UNMAPPED_TIMESTAMP_SUFFIX, p.sourceText()));
}
p.forEachExpression(Expression.class, e -> {
if (e instanceof TimestampAware ta && ta.timestamp() instanceof UnresolvedTimestamp) {
failures.add(fail(e, "[{}] " + UnresolvedTimestamp.UNRESOLVED_SUFFIX + UNMAPPED_TIMESTAMP_SUFFIX, e.sourceText()));
}
});
});
return true;
}

private static boolean hasTimestamp(EsRelation relation) {
for (Attribute attr : relation.output()) {
Comment thread
GalLalouche marked this conversation as resolved.
if (MetadataAttribute.TIMESTAMP_FIELD.equals(attr.name())) {
return true;
}
}
return false;
}

/**
* {@code unmapped_fields="load"} does not yet support branching commands (FORK, LOOKUP JOIN, subqueries/views).
* See https://github.com/elastic/elasticsearch/issues/142033
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@
import org.elasticsearch.xpack.esql.core.expression.UnresolvedTimestamp;
import org.elasticsearch.xpack.esql.core.tree.Source;
import org.elasticsearch.xpack.esql.plan.IndexPattern;
import org.elasticsearch.xpack.esql.plan.logical.Aggregate;
import org.elasticsearch.xpack.esql.plan.logical.EsRelation;
import org.elasticsearch.xpack.esql.plan.logical.Filter;
import org.elasticsearch.xpack.esql.plan.logical.Limit;
import org.elasticsearch.xpack.esql.plan.logical.Project;

Expand Down Expand Up @@ -528,6 +530,173 @@ public void testLoadModeDisallowsSubqueryAndFork() {
verificationFailure(query, "FORK is not supported with unmapped_fields=\"load\"");
}

private static final String UNMAPPED_TIMESTAMP_SUFFIX = UnresolvedTimestamp.UNRESOLVED_SUFFIX + Verifier.UNMAPPED_TIMESTAMP_SUFFIX;

public void testTbucketWithUnmappedTimestamp() {
unmappedTimestampFailure("FROM test | STATS c = COUNT(*) BY tbucket(1 hour)", "[tbucket(1 hour)] ");
}

public void testTrangeWithUnmappedTimestamp() {
unmappedTimestampFailure("FROM test | WHERE trange(1 hour)", "[trange(1 hour)] ");
}

public void testTbucketAndTrangeWithUnmappedTimestamp() {
unmappedTimestampFailure(
"FROM test | WHERE trange(1 hour) | STATS c = COUNT(*) BY tbucket(1 hour)",
"[tbucket(1 hour)] ",
"[trange(1 hour)] "
);
}

public void testRateWithUnmappedTimestamp() {
unmappedTimestampFailure("FROM test | STATS rate(salary)", "[rate(salary)] ");
}

public void testIrateWithUnmappedTimestamp() {
unmappedTimestampFailure("FROM test | STATS irate(salary)", "[irate(salary)] ");
}

public void testDeltaWithUnmappedTimestamp() {
unmappedTimestampFailure("FROM test | STATS delta(salary)", "[delta(salary)] ");
}

public void testIdeltaWithUnmappedTimestamp() {
unmappedTimestampFailure("FROM test | STATS idelta(salary)", "[idelta(salary)] ");
}

public void testIncreaseWithUnmappedTimestamp() {
unmappedTimestampFailure("FROM test | STATS increase(salary)", "[increase(salary)] ");
}

public void testDerivWithUnmappedTimestamp() {
unmappedTimestampFailure("FROM test | STATS deriv(salary)", "[deriv(salary)] ");
}

public void testFirstOverTimeWithUnmappedTimestamp() {
unmappedTimestampFailure("FROM test | STATS first_over_time(salary)", "[first_over_time(salary)] ");
}

public void testLastOverTimeWithUnmappedTimestamp() {
unmappedTimestampFailure("FROM test | STATS last_over_time(salary)", "[last_over_time(salary)] ");
}

public void testRateAndTbucketWithUnmappedTimestamp() {
unmappedTimestampFailure("FROM test | STATS rate(salary) BY tbucket(1 hour)", "[rate(salary)] ", "[tbucket(1 hour)] ");
}

public void testTbucketWithUnmappedTimestampAfterWhere() {
unmappedTimestampFailure("FROM test | WHERE emp_no > 10 | STATS c = COUNT(*) BY tbucket(1 hour)", "[tbucket(1 hour)] ");
}

public void testTbucketWithUnmappedTimestampAfterEval() {
unmappedTimestampFailure("FROM test | EVAL x = salary + 1 | STATS c = COUNT(*) BY tbucket(1 hour)", "[tbucket(1 hour)] ");
}

public void testTbucketWithUnmappedTimestampMultipleGroupings() {
unmappedTimestampFailure("FROM test | STATS c = COUNT(*) BY tbucket(1 hour), emp_no", "[tbucket(1 hour)] ");
}

public void testTbucketWithUnmappedTimestampAfterRename() {
unmappedTimestampFailure("FROM test | RENAME emp_no AS e | STATS c = COUNT(*) BY tbucket(1 hour)", "[tbucket(1 hour)] ");
}

public void testTbucketWithUnmappedTimestampAfterDrop() {
unmappedTimestampFailure("FROM test | DROP emp_no | STATS c = COUNT(*) BY tbucket(1 hour)", "[tbucket(1 hour)] ");
}

public void testTrangeWithUnmappedTimestampCompoundWhere() {
unmappedTimestampFailure("FROM test | WHERE trange(1 hour) AND emp_no > 10", "[trange(1 hour)] ");
}

public void testTrangeWithUnmappedTimestampAfterEval() {
unmappedTimestampFailure("FROM test | EVAL x = salary + 1 | WHERE trange(1 hour)", "[trange(1 hour)] ");
}

public void testTbucketWithUnmappedTimestampInInlineStats() {
unmappedTimestampFailure("FROM test | INLINE STATS c = COUNT(*) BY tbucket(1 hour)", "[tbucket(1 hour)] ");
}

public void testTbucketWithUnmappedTimestampWithFork() {
var query = "FROM test | FORK (STATS c = COUNT(*) BY tbucket(1 hour)) (STATS d = COUNT(*) BY emp_no)";
for (var statement : List.of(setUnmappedNullify(query), setUnmappedLoad(query))) {
var e = expectThrows(VerificationException.class, () -> analyzeStatement(statement));
assertThat(e.getMessage(), containsString("[tbucket(1 hour)] "));
assertThat(e.getMessage(), not(containsString("FORK is not supported")));
}
}

public void testTbucketWithUnmappedTimestampWithLookupJoin() {
var query = """
FROM test
| EVAL language_code = languages
| LOOKUP JOIN languages_lookup ON language_code
| STATS c = COUNT(*) BY tbucket(1 hour)
""";
for (var statement : List.of(setUnmappedNullify(query), setUnmappedLoad(query))) {
var e = expectThrows(VerificationException.class, () -> analyzeStatement(statement));
assertThat(e.getMessage(), containsString("[tbucket(1 hour)] "));
assertThat(e.getMessage(), not(containsString("LOOKUP JOIN is not supported")));
}
}

public void testTbucketWithTimestampPresent() {
Comment thread
GalLalouche marked this conversation as resolved.
var query = "FROM sample_data | STATS c = COUNT(*) BY tbucket(1 hour)";
for (var statement : List.of(setUnmappedNullify(query), setUnmappedLoad(query))) {
var plan = analyzeStatement(statement);
var limit = as(plan, Limit.class);
var aggregate = as(limit.child(), Aggregate.class);
var relation = as(aggregate.child(), EsRelation.class);
assertThat(relation.indexPattern(), is("sample_data"));
assertTimestampInOutput(relation);
}
}

public void testTrangeWithTimestampPresent() {
var query = "FROM sample_data | WHERE trange(1 hour)";
for (var statement : List.of(setUnmappedNullify(query), setUnmappedLoad(query))) {
var plan = analyzeStatement(statement);
var limit = as(plan, Limit.class);
var filter = as(limit.child(), Filter.class);
var relation = as(filter.child(), EsRelation.class);
assertThat(relation.indexPattern(), is("sample_data"));
assertTimestampInOutput(relation);
}
}

public void testTbucketTimestampPresentButDroppedNullify() {
var e = expectThrows(
VerificationException.class,
() -> analyzeStatement(setUnmappedNullify("FROM sample_data | DROP @timestamp | STATS c = COUNT(*) BY tbucket(1 hour)"))
);
assertThat(e.getMessage(), containsString(UnresolvedTimestamp.UNRESOLVED_SUFFIX));
assertThat(e.getMessage(), not(containsString(Verifier.UNMAPPED_TIMESTAMP_SUFFIX)));
}

public void testTbucketTimestampPresentButRenamedNullify() {
var e = expectThrows(
VerificationException.class,
() -> analyzeStatement(setUnmappedNullify("FROM sample_data | RENAME @timestamp AS ts | STATS c = COUNT(*) BY tbucket(1 hour)"))
);
assertThat(e.getMessage(), containsString(UnresolvedTimestamp.UNRESOLVED_SUFFIX));
assertThat(e.getMessage(), not(containsString(Verifier.UNMAPPED_TIMESTAMP_SUFFIX)));
}

private static void assertTimestampInOutput(EsRelation relation) {
assertTrue(
"@timestamp field should be present in the EsRelation output",
relation.output().stream().anyMatch(a -> MetadataAttribute.TIMESTAMP_FIELD.equals(a.name()))
);
}

private void unmappedTimestampFailure(String query, String... expectedFailures) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like you're always appending the suffix to the expectedFailures. Why not just do it in this method instead of in all the other places?

for (var statement : List.of(setUnmappedNullify(query), setUnmappedLoad(query))) {
var e = expectThrows(VerificationException.class, () -> analyzeStatement(statement));
for (String expected : expectedFailures) {
assertThat(e.getMessage(), containsString(expected + UNMAPPED_TIMESTAMP_SUFFIX));
}
}
}

private void verificationFailure(String statement, String expectedFailure) {
var e = expectThrows(VerificationException.class, () -> analyzeStatement(statement));
assertThat(e.getMessage(), containsString(expectedFailure));
Expand Down
Loading