Skip to content

Conversation

quux00
Copy link
Contributor

@quux00 quux00 commented Nov 14, 2024

Follow-on PR for functionality added in #116348

@quux00 quux00 added >test Issues or PRs that are addressing/adding tests Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL v9.0.0 v8.17.0 labels Nov 14, 2024
@elasticsearchmachine
Copy link
Collaborator

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

@quux00 quux00 changed the title Added ESQL skip_unavailable testing for non-matching index expressions under RCS2 ESQL: CCS skip_unavailable testing for non-matching index expressions under RCS2 Nov 14, 2024
@quux00 quux00 requested a review from jakelandis November 14, 2024 22:10
Copy link
Contributor

@jakelandis jakelandis left a comment

Choose a reason for hiding this comment

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

LGTM and thanks for the comments as they serve as documentation for how this is intended to work for various cases (which i will never remember and is too low level for reference documentation)

assertThat(e.getMessage(), containsString("on indices [employees_nomatch]"));
assertThat(e.getMessage(), containsString("security_exception"));

// TODO: in follow on PR, add support for throwing a VerificationException for this scenario - no exception is currently thrown
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can you remove "in a follow on PR", unless you plan to address immediately ? else, if in a year+ looking at the message appears that we forgot to do something important instead of a standard TODO which represents an area of improvement, but maybe not completely necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two commented out "TODO" sections in this test. One will be handled immediately (PR is currently in progress). The other is slated to be worked on as part of #114495. That one is probably slated for post ESQL CCS GA timeframe, Jan/Feb 2025.

@quux00 quux00 merged commit 286c4bb into elastic:main Nov 19, 2024
16 checks passed
quux00 added a commit to quux00/elasticsearch that referenced this pull request Nov 20, 2024
quux00 added a commit to quux00/elasticsearch that referenced this pull request Nov 20, 2024
quux00 added a commit that referenced this pull request Nov 20, 2024
rjernst pushed a commit to rjernst/elasticsearch that referenced this pull request Nov 20, 2024
alexey-ivanov-es pushed a commit to alexey-ivanov-es/elasticsearch that referenced this pull request Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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 v8.17.0 v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants