Skip to content

ESQL: Remove temporary workarounds for resolved bugs #127499

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

Merged

Conversation

kanoshiou
Copy link
Contributor

As far as I have tested, #127167 appears to be a variant of #126026, and #125870 seems to be a variant of #126392. Since these issues have been resolved, the temporary workaround can now be safely removed.

Closes #127167
Closes #125870

@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label external-contributor Pull request authored by a developer outside the Elasticsearch team v9.1.0 labels Apr 29, 2025
@PeteGillinElastic PeteGillinElastic added :Analytics/ES|QL AKA ESQL and removed needs:triage Requires assignment of a team area label labels May 1, 2025
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label May 1, 2025
@elasticsearchmachine
Copy link
Collaborator

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

@luigidellaquila luigidellaquila self-assigned this May 2, 2025
@luigidellaquila luigidellaquila self-requested a review May 2, 2025 07:58
@luigidellaquila
Copy link
Contributor

buildkite test this

@luigidellaquila luigidellaquila added the >test Issues or PRs that are addressing/adding tests label May 2, 2025
@luigidellaquila
Copy link
Contributor

Thanks @kanoshiou!
I see some failures in the CI that seem completely unrelated to the changes introduced by this PR.
I'm updating the branch to see if they get fixed

@luigidellaquila
Copy link
Contributor

buildkite test this

@kanoshiou
Copy link
Contributor Author

Thank you @luigidellaquila !

I noticed a failed test in the last CI run related to this PR.

./gradlew ":x-pack:plugin:esql:qa:server:multi-clusters:v9.1.0#newToOld" -Dtests.class="org.elasticsearch.xpack.esql.ccq.MultiClusterSpecIT" -Dtests.method="test {eval.EvalAfterGroupingUsingSameName5}" -Dtests.seed=BB2926D5EC19AEEC -Dtests.bwc=true -Dtests.locale=ln-Latn-CD -Dtests.timezone=Indian/Kerguelen -Druntime.java=24
MultiClusterSpecIT > test {eval.EvalAfterGroupingUsingSameName5} FAILED
    org.elasticsearch.client.ResponseException: method [POST], host [http://[::1]:62902], URI [/_query?pretty=true&error_trace=true], status line [HTTP/1.1 400 Bad Request]
    ---
    error:
      root_cause:
      - type: "parsing_exception"
        reason: "line 7:15: invalid index pattern [*:firewall_logs,firewall_logs,*:threat_list,threat_list,*:airports,airports],\
          \ remote clusters are not supported in LOOKUP JOIN"
        stack_trace: "org.elasticsearch.xpack.esql.parser.ParsingException: line 7:15:\
          \ invalid index pattern [*:firewall_logs,firewall_logs,*:threat_list,threat_list,*:airports,airports],\
          \ remote clusters are not supported in LOOKUP JOIN\n\tat org.elasticsearch.xpack.esql.parser.LogicalPlanBuilder.lambda$visitJoinCommand$24(LogicalPlanBuilder.java:621)\n\

I’m not sure how to resolve it, so any guidance would be greatly appreciated!

@luigidellaquila
Copy link
Contributor

Thanks for checking.
Adding required_capability: join_lookup_v12 should solve the problem

@kanoshiou
Copy link
Contributor Author

Thanks, @luigidellaquila! I've updated the branch.

@luigidellaquila
Copy link
Contributor

buildkite test this

@luigidellaquila
Copy link
Contributor

The CI is not happy about the changelog file apparently.
The error is not clear, but my guess is that it could be the type: test.
In this case I think we don't need a changelog entry at all actually

@kanoshiou
Copy link
Contributor Author

kanoshiou commented May 2, 2025

[$.type: does not have a value in the enumeration [breaking, breaking-java, bug, deprecation, enhancement, feature, known-issue, new-aggregation, regression, security, upgrade]]

I noticed that you added the test label, so I updated the type to test😂.
Should I change the type value or just delete the changelog?

@luigidellaquila
Copy link
Contributor

I'd just remove the delete the file

@kanoshiou
Copy link
Contributor Author

I have removed the changelog.

@luigidellaquila
Copy link
Contributor

buildkite test this

Copy link
Contributor

@luigidellaquila luigidellaquila left a comment

Choose a reason for hiding this comment

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

Thanks @kanoshiou! LGTM

@luigidellaquila luigidellaquila merged commit 7269b75 into elastic:main May 5, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL external-contributor Pull request authored by a developer outside the Elasticsearch team 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
4 participants