Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Esql support #233
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
base: main
Are you sure you want to change the base?
Esql support #233
Changes from 7 commits
a307db2
9c35f22
6f99055
086a592
e30e0f9
7746c14
76303d8
1fb29f7
5d47f2f
c291e24
22e72e9
af6e24a
4ce6fa4
4ed69ff
0725f98
cfb36f3
a92a71e
d4f559d
65eb675
789f467
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 feels cumbersome to me.
Could we align with the proposal in the filter PR to provide an ESQL query with
esql_query
instead of requring the configuration of multiple separate parameters?In this case, since the input plugin does require a JSON-encoded object for its
query
parameter when using the Query DSL, we could auto-detect that a givenquery
parameter is ESQL (unlike the ES filter, which uses a QueryString query as itsquery
parameter)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.
When we had a discussion with @jsvd about this, we had a similar idea to deprecate this
response_type
and replace withquery_type
in the future. And through the experience as I do see, introducing new param is not a difficult, deprecation -> obseletion -> removal is a long headache process.From this point of view, I would support adding minimal change but I am open to apply changes if anyone has strong opinion.
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.
I've left a separate note on how to do it.
I don't personally care much about removing the
response_type
right away, but if a user starts using ESQL I'd like them to not start new usages of a config that we'd like to deprecate.Since this is effectively a rename, we can easily use the
with_deprecated_alias
helper fromNormalizeConfigSupport
.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.
Migrating to
query_type
with auto-detection of ESQL queries would be pretty straight-forward with theNormalizeConfigSupport
mixin: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.
I was thinking to add the deprecation right after this ES|QL change.
One agreement we need to decide is naming. I personally do not like
hits
,aggregations
along withesql
. They indicate different contexts. I had optionsdsl_search
,dsl_aggregation
andesql
.Let me please know your opinion: I can either apply with change if we quickly come with agreement or create an issue follow up right after this PR.
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.
review note: moved to private area