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

[CI] StatementParserTests testInvalidJoinPatterns failing #125536

Closed
elasticsearchmachine opened this issue Mar 24, 2025 · 5 comments · Fixed by #125731
Closed

[CI] StatementParserTests testInvalidJoinPatterns failing #125536

elasticsearchmachine opened this issue Mar 24, 2025 · 5 comments · Fixed by #125731
Assignees
Labels
:Analytics/ES|QL AKA ESQL low-risk An open issue or test failure that is a low risk to future releases Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test-failure Triaged test failures from CI v8.19.0 v9.1.0

Comments

@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Mar 24, 2025

Build Scans:

Reproduction Line:

./gradlew ":x-pack:plugin:esql:test" --tests "org.elasticsearch.xpack.esql.parser.StatementParserTests.testInvalidJoinPatterns" -Dtests.seed=40CE02365A6C686B -Dtests.locale=ln-AO -Dtests.timezone=America/Monterrey -Druntime.java=24

Applicable branches:
main

Reproduces locally?:
N/A

Failure History:
See dashboard

Failure Message:

java.lang.AssertionError: 
Expected: a string containing "mismatched input '::' expecting {<EOF>, '|', ',', 'metadata'}"
     but: was "line 1:162: mismatched input '-11:00}}>::data' expecting {DEV_CHANGE_POINT, 'enrich', 'dissect', 'eval', 'grok', 'limit', 'sort', 'stats', 'where', DEV_INLINESTATS, DEV_FORK, 'lookup', DEV_JOIN_LEFT, DEV_JOIN_RIGHT, DEV_LOOKUP, 'mv_expand', 'drop', 'keep', DEV_INSIST, DEV_RRF, 'rename'}"

Issue Reasons:

  • [main] 13 failures in test testInvalidJoinPatterns (26.5% fail rate in 49 executions)
  • [main] 2 failures in step part-3 (8.7% fail rate in 23 executions)
  • [main] 2 failures in pipeline elasticsearch-pull-request (8.7% fail rate in 23 executions)

Note:
This issue was created using new test triage automation. Please report issues or feedback to es-delivery.

@elasticsearchmachine
Copy link
Collaborator Author

This has been muted on branch main

Mute Reasons:

  • [main] 3 failures in test testInvalidJoinPatterns (12.0% fail rate in 25 executions)
  • [main] 2 failures in step part-3 (10.0% fail rate in 20 executions)
  • [main] 2 failures in pipeline elasticsearch-pull-request (10.0% fail rate in 20 executions)

Build Scans:

@elasticsearchmachine elasticsearchmachine added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) needs:risk Requires assignment of a risk label (low, medium, blocker) labels Mar 24, 2025
@elasticsearchmachine
Copy link
Collaborator Author

Pinging @elastic/es-analytical-engine (Team:Analytics)

@idegtiarenko
Copy link
Contributor

idegtiarenko commented Mar 25, 2025

Caused by #120660

@jbaiera I identified following issues with the test, could you please take a look on them:

// Selectors are not supported on the left of the join query if used with cluster ids.
var fromPatterns = randomIndexPatterns(CROSS_CLUSTER);
// We do different validation based on the quotation of the pattern
// Autogenerated patterns will not mix cluster ids with selectors. Unquote it to ensure stable tests
fromPatterns = unquoteIndexPattern(fromPatterns) + "::data";
var joinPattern = randomIndexPattern(without(CROSS_CLUSTER), without(WILDCARD_PATTERN), without(INDEX_SELECTOR));
expectError(
"FROM " + fromPatterns + " | LOOKUP JOIN " + joinPattern + " ON " + randomIdentifier(),
"mismatched input '::' expecting {<EOF>, '|', ',', 'metadata'}"
);

  1. Date math expressions (such as index-{now/M-1M{yyyy.MM.dd}}) must be quoted. If the test relies on unquoted identifiers, than DATE_MATH feature must be excluded
  2. randomIndexPatterns usage looks unexpected. It results in index-1,index-2,index-3::data. Why it is not generating a single pattern with selector regardless quotes?
  3. I am a bit puzzled by the error message mismatched input '::' expecting {<EOF>, '|', ',', 'metadata'}. It looks like generic error message when new grammar is disabled (eg in release or non dev tests only) is it the case?

// Selectors are not supported on the left of the join query if used with cluster ids.
var fromPatterns = randomIndexPatterns(CROSS_CLUSTER, without(INDEX_SELECTOR));
// We do different validation based on the quotation of the pattern
// Autogenerated patterns will not mix cluster ids with selectors. Unquote, modify, and requote it to ensure stable tests
fromPatterns = "\"" + unquoteIndexPattern(fromPatterns) + "::data\"";
var joinPattern = randomIndexPattern(without(CROSS_CLUSTER), without(WILDCARD_PATTERN), without(INDEX_SELECTOR));
expectError(
"FROM " + fromPatterns + " | LOOKUP JOIN " + joinPattern + " ON " + randomIdentifier(),
"Selectors are not yet supported on remote cluster patterns"
);

  1. Similar as above, why do we force quoting?

// Selectors are not yet supported in join patterns on the right.
var joinPattern = randomIndexPattern(without(CROSS_CLUSTER), without(WILDCARD_PATTERN), INDEX_SELECTOR);
expectError(
"FROM " + randomIndexPatterns() + " | LOOKUP JOIN " + joinPattern + " ON " + randomIdentifier(),
"extraneous input ':' expecting {QUOTED_STRING, UNQUOTED_SOURCE}"
);

  1. Left pattern is verified first. It must be without(CROSS_CLUSTER) in order to pass validation.

@idegtiarenko idegtiarenko added the low-risk An open issue or test failure that is a low risk to future releases label Mar 25, 2025
@elasticsearchmachine elasticsearchmachine removed the needs:risk Requires assignment of a risk label (low, medium, blocker) label Mar 25, 2025
@jbaiera
Copy link
Member

jbaiera commented Mar 26, 2025

Will get a fix put together for these issues soon. Thanks for the direct guidance on how to get these back toward a good state.

To answer a couple of questions raised:

randomIndexPatterns usage looks unexpected. It results in index-1,index-2,index-3::data. Why it is not generating a single pattern with selector regardless quotes?

Hmm, I would expect the highlighted case to have cluster ids generated and added to the patterns, unless you're simply giving a hypothetical example of the resulting values.

Cluster ids and index selectors are exclusive to one another in the random index pattern creation logic. We needed to keep them exclusive of each other because supported features will randomly toggle on if not explicitly enabled or disabled. This could cause other tests to fail in the case that both features toggled on for a random pattern.

In this highlighted test case we have to circumvent the random pattern logic to force an invalid pattern that does combine these features together to make sure they throw the exceptions expected.

I am a bit puzzled by the error message mismatched input '::' expecting {, '|', ',', 'metadata'}. It looks like generic error message when new grammar is disabled (eg in release or non dev tests only) is it the case?

This is an error message that is returned from the Antlr generated code because the selectors and cluster ids are not compatible with each other in the esql syntax. Unfortunately, further validation is required for quoted fields because they circumvent the syntax rules as an escaped pattern:

Similar as above, why do we force quoting?

We force quoting on this test because the error message is different if you supply cluster:index::data vs "cluster:index::data". The former is disallowed by the language syntax until CCS and selectors are compatible with each other. The latter is allowed by the language syntax because it is escaped, but needs to be validated in the IdentifierBuilder.

@jbaiera
Copy link
Member

jbaiera commented Mar 26, 2025

Opened #125731 for this

omricohenn pushed a commit to omricohenn/elasticsearch that referenced this issue Mar 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL low-risk An open issue or test failure that is a low risk to future releases Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test-failure Triaged test failures from CI v8.19.0 v9.1.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants