-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Allow non-exact query types on the root JSON field. #41290
Allow non-exact query types on the root JSON field. #41290
Conversation
Pinging @elastic/es-search |
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 for the root field, I wonder if you plan to add the support for the keyed field as well, I am fine if we do this in a follow up but I think we can support these queries for both (the root and the keyed field) ?
throw new UnsupportedOperationException("[fuzzy] queries are not currently supported on [" + | ||
CONTENT_TYPE + "] fields."); | ||
throw new UnsupportedOperationException("[fuzzy] queries are not currently supported on keyed " + | ||
"[" + CONTENT_TYPE + "] fields."); |
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 don't know what is your plan on this but it should be possible to allow this type of query by adding the field prefix in the fuzzy query and change the prefixLength to be prefixLength + field.length + 1
?
throw new UnsupportedOperationException("[regexp] queries are not currently supported on [" + | ||
CONTENT_TYPE + "] fields."); | ||
throw new UnsupportedOperationException("[regexp] queries are not currently supported on keyed " + | ||
"[" + CONTENT_TYPE + "] fields."); |
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.
Same here, just adding the field prefix should work ?
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.
Do you think it'd make sense to automatically adjust maxDeterminizedStates
as well?
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.
let's keep the value as is, the field name shouldn't consume a lot of states and the current limit is already high (10,000
).
throw new UnsupportedOperationException("[wildcard] queries are not currently supported on [" + | ||
CONTENT_TYPE + "] fields."); | ||
throw new UnsupportedOperationException("[wildcard] queries are not currently supported on keyed " + | ||
"[" + CONTENT_TYPE + "] fields."); |
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.
Same here ?
I was planning some future work to support these queries on the keyed field. However maybe it's not as difficult as I thought and I could just add it to this PR. The only slight difficulty I saw was that the keys are allowed to contain characters such as |
Thanks, I missed the escaping part. I am fine either way but feel free to merge this pr and we can have a follow up to discuss the keyed field part. |
@elasticmachine run elasticsearch-ci/2 |
Thanks @jimczi, I've decided to merge this as-is. |
In an earlier iteration of the design, it made sense to disallow these query types on the root JSON field. It should now it be fine to allow them.
In an earlier iteration of the design, it made sense to disallow these query types on the root JSON field. It should now it be fine to allow them.
In an earlier iteration of the design, it made sense to disallow these query types on the root JSON field. It should now it be fine to allow them.
In an earlier iteration of the design, it made sense to disallow these query
types on the root JSON field. It should now it be fine to allow them.