-
Notifications
You must be signed in to change notification settings - Fork 25.6k
ES|QL: Disallow brackets in unquoted index patterns #130427
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
ES|QL: Disallow brackets in unquoted index patterns #130427
Conversation
Hi @luigidellaquila, I've created a changelog YAML for you. |
…indexes' into esql/no_parentheses_in_indexes
Pinging @elastic/es-analytical-engine (Team:Analytics) |
I believe we keep enrich policy definition in cluster state. Should we prohibit creation of new ones with brackets? What should we do with ones previously created with brackets in names? Please let me know if this concerns are separate from this change |
Enrich policies allow brackets, so I don't think we can just prohibit it. Yaml tests apparently have problems creating policies with brackets, but I'll add a Java integration test for it. |
@@ -0,0 +1,102 @@ | |||
--- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has to be a separate file and with a single test, just because the policy delete doesn't seem to work with brackets
- do:
enrich.delete_policy:
name: <name with brackets here>
Apparently the bracket collides with some regex evaluation that happens during the process.
I couldn't find a way to work around this problem, but it seems unrelated to ES|QL, so I guess we can handle it separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No way, I had to skip the test.
If I don't delete the enrich policy, it will make other tests fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@idegtiarenko I added a test that uses an enrich policy with brackets (with quotes), and it works as expected. |
can't delete enrich policy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but you need to be paranoid with grammar changes and add more tests. I've put in some examples which should be extended to other commands that use index names patterns.
@@ -0,0 +1,5 @@ | |||
pr: 130427 | |||
summary: Disallow brackets in unquoted index pattersn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo
UnresolvedRelation right = as(lookup.right(), UnresolvedRelation.class); | ||
assertThat(right.indexPattern().indexPattern(), is("foo)")); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, add more unit tests here. The above ones are the bare minimum imo, but it's always good to be paranoid with grammar changes as the devil is in the edge cases :-)).
from te()st
from \"te()st\"
from concat(foo, bar)
from ((((()))))
from (((abc)))
from *()*
from *test()*
from *:test()
from *:()
from *:test)
from remote1:test(),remote2:test
and use these also for other places where index names are used (enrich
, lookup join
, fork
etc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks
No 9.1 version backport? Just curious |
We absolutely need to backport it to 9.1 as well, thanks for pointing it out. |
Actually, I think we want it only in 9.2, 9.1, and 8.19. |
💔 Backport failed
You can use sqren/backport to manually backport by running |
|
||
|
||
--- | ||
"Enrich in fork": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test is causing bwc test failures since FORK is not available in 8.19
To avoid collisions with subquery syntax, we disallow the usage of
(
and)
in unquoted index patterns and enrich policy names.Fixes: #130378