Skip to content

Fix testInvalidJoinPatterns #129611

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 18, 2025
Merged

Fix testInvalidJoinPatterns #129611

merged 3 commits into from
Jun 18, 2025

Conversation

idegtiarenko
Copy link
Contributor

@idegtiarenko idegtiarenko commented Jun 18, 2025 •
edited
Loading

According to the report, the test was accidentally generating nested quotes such as:

dlhmhbrel:""""<.nu^'cy&!it46ay^lhm23kwbkyv!0qrz*-{now/M{yyyy.MM.dd|-15:00}}>::data"""

due to date math expressions.
Those are not necessary in this test case and could be excluded.

Closes: #129598

@idegtiarenko idegtiarenko requested a review from alex-spies June 18, 2025 08:24
@idegtiarenko idegtiarenko added >test-failure Triaged test failures from CI Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL v9.1.0 labels Jun 18, 2025
@elasticsearchmachine elasticsearchmachine added the needs:risk Requires assignment of a risk label (low, medium, blocker) label Jun 18, 2025
@elasticsearchmachine
Copy link
Collaborator

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

@idegtiarenko idegtiarenko mentioned this pull request Jun 18, 2025
Merged
@astefan astefan added >test Issues or PRs that are addressing/adding tests and removed >test-failure Triaged test failures from CI needs:risk Requires assignment of a risk label (low, medium, blocker) labels Jun 18, 2025
pawankartik-elastic
pawankartik-elastic previously approved these changes Jun 18, 2025
View reviewed changes
Copy link
Contributor

@pawankartik-elastic pawankartik-elastic left a comment

Choose a reason for hiding this comment

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

Yikes, thanks for flagging this!

@pawankartik-elastic
Copy link
Contributor

pawankartik-elastic commented Jun 18, 2025 •
edited
Loading

I think the right way to fix this is to not quote if the expression is already quoted due to datemath; rather than excluding them. Wdyt? If you find this acceptable, I'll push the fix to this branch.
Edit: Ref Alex's comment.

alex-spies
alex-spies reviewed Jun 18, 2025
View reviewed changes
x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java Outdated
@@ -3205,7 +3205,7 @@ public void testInvalidJoinPatterns() {
{
var fromPatterns = randomIndexPattern();
// Generate a syntactically invalid (partial quoted) pattern.
var joinPattern = randomIdentifier() + ":" + quote(randomIndexPatterns(without(CROSS_CLUSTER)));
var joinPattern = randomIdentifier() + ":" + quote(randomIndexPatterns(without(CROSS_CLUSTER), without(DATE_MATH)));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the problem is that we use randomIndexPatterns (plural!). Date math should be fine in this test - why do we specifically exclude it?

Shouldn't the fix just be to use randomIndexPattern (singular)? That also seems to be consistent with the error message on #129598:

token recognition error at: ','"

Copy link
Contributor

@pawankartik-elastic pawankartik-elastic Jun 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, yes. This actually makes sense. I don't know why I used randomIndexPatterns() (likely wanted to use randomIndexPattern()) and was wondering what the issue with the nested quoting was since the original tests did actually use nested quoting. Even my code comment says "pattern" specifically. Should I push the changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Update: sorry, actually multiple index patterns would also be fine to test - although I'm not sure this is the original intention.

The problem seems to be the double quoting, because randomIndexPatterns has a maybeQuote in there, but we always quote the output - potentially another time.

My comment that I don't see how this relates to date math still holds IMO, but I may not be seeing something - randomization over many cases can be funny :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Another bug that may cause an issue in the future: randomIndexPatterns() uses maybeQuote(). Shouldn't it be quote() considering we now prohibit partial quoting? Or else, we'll end up generating: "a",b,c.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybeQuote only quotes if the patterns don't contain a single ", so this is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't the fix just be to use randomIndexPattern (singular)

Makes sense! Updated to singular and this seems to not fail after couple of thousands of iterations.

@pawankartik-elastic pawankartik-elastic dismissed their stale review June 18, 2025 09:09

Ref the discussion.

@alex-spies
Copy link
Contributor

@idegtiarenko , Pawan and I dissected the error message a bit offline. I still believe it's the double quoting.

Notice the 4 quotes:
""""<.nu^'cy&!it46ay^lhm23kwbkyv!0qrz*-{now/M{yyyy.MM.dd|-15:00}}>::data""",<.e*okgu6-{now/M{yyyy.MM}}>,-<.28s7a4f-{now/M{yyyy.MM}}>::failures,.fr'0^06!d.476x8$lj"

What I think the lexer sees:
"""
"<.nu^'cy&!it46ay^lhm23kwbkyv!0qrz*-{now/M{yyyy.MM.dd|-15:00}}>::data
"""
,<.e*okgu6-{now/M{yyyy.MM}}>,-<.28s7a4f-{now/M{yyyy.MM}}>::failures,.fr'0^06!d.476x8$lj"
The parser doesn't expect any token that starts with a comma here.

But we double quoted mistakenly:
"
"""<.nu^'cy&!it46ay^lhm23kwbkyv!0qrz*-{now/M{yyyy.MM.dd|-15:00}}>::data""",<.e*okgu6-{now/M{yyyy.MM}}>,-<.28s7a4f-{now/M{yyyy.MM}}>::failures,.fr'0^06!d.476x8$lj
"

alex-spies
alex-spies approved these changes Jun 18, 2025
View reviewed changes
Copy link
Contributor

@alex-spies alex-spies left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @idegtiarenko !

@idegtiarenko
Merge branch 'main' into fix_129598
dd3e109 
# Conflicts:
#	muted-tests.yml
@idegtiarenko idegtiarenko merged commit 1e347d5 into elastic:main Jun 18, 2025
27 checks passed
@idegtiarenko idegtiarenko deleted the fix_129598 branch June 18, 2025 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alex-spies alex-spies alex-spies approved these changes

@pawankartik-elastic pawankartik-elastic pawankartik-elastic approved these changes

Assignees
No one assigned
Labels
:Analytics/ES|QL AKA ESQL Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test Issues or PRs that are addressing/adding tests v9.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[CI] StatementParserTests testInvalidJoinPatterns failing
5 participants
@idegtiarenko @elasticsearchmachine @pawankartik-elastic @alex-spies @astefan