Discrepancy in result from _validate/query API and actual query validity #2416
Conversation
Signed-off-by: Andriy Redko <[email protected]>
|
@AmiStrn have a solution for you :-) |
|
Can one of the admins verify this patch? |
| @@ -451,6 +451,11 @@ public String getWriteableName() { | |||
| return MappedFieldType.Relation.INTERSECTS; | |||
| } | |||
|
|
|||
| // For validation, always assume that there is an intersection | |||
| if (shardContext.validate()) { | |||
There was a problem hiding this comment.
Are we validating the range is in valid DateTimeFormat (can be parsed as such) before bypassing the validation to the other part of the query with INTERSECT? I could be wrong, but seems like the dateMathParser below does some validation for the date format here. In case of invalid range, the query should be marked invalid. Couldn't find a test case for this scenario or maybe I missed? Just want to make sure we are not ignoring the case where timestamp is invalid.
| @@ -451,6 +451,11 @@ public String getWriteableName() { | |||
| return MappedFieldType.Relation.INTERSECTS; | |||
| } | |||
|
|
|||
| // For validation, always assume that there is an intersection | |||
| if (shardContext.validate()) { | |||
There was a problem hiding this comment.
Are we validating the range is in valid DateTimeFormat (can be parsed as such) before bypassing the validation to the other part of the query with INTERSECT? I could be wrong, but seems like the dateMathParser below does some validation for the date format here. In case of invalid range, the query should be marked invalid. Couldn't find a test case for this scenario or maybe I missed? Just want to make sure we are not ignoring the case where timestamp is invalid.
There was a problem hiding this comment.
@setiah thanks for looking, yeah, it does not validate but probably should, updated the pull request accordingly, thanks!
…ion to trigger first Signed-off-by: Andriy Redko <[email protected]>
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.x 1.x
# Navigate to the new working tree
cd .worktrees/backport-1.x
# Create a new branch
git switch --create backport/backport-2416-to-1.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 5c0f9bc499c5c4a744d2f29fb5bd9eab4aabc004
# Push it to GitHub
git push --set-upstream origin backport/backport-2416-to-1.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.xThen, create a pull request where the |
…ity (opensearch-project#2416) * Discrepancy in result from _validate/query API and actual query validity Signed-off-by: Andriy Redko <[email protected]> * Moved the validate() check later into the flow to allow range validation to trigger first Signed-off-by: Andriy Redko <[email protected]>
…ity (opensearch-project#2416) * Discrepancy in result from _validate/query API and actual query validity Signed-off-by: Andriy Redko <[email protected]> * Moved the validate() check later into the flow to allow range validation to trigger first Signed-off-by: Andriy Redko <[email protected]>
…ity (#2416) (#2466) * Discrepancy in result from _validate/query API and actual query validity Signed-off-by: Andriy Redko <[email protected]> * Moved the validate() check later into the flow to allow range validation to trigger first Signed-off-by: Andriy Redko <[email protected]>
Signed-off-by: Andriy Redko [email protected]
Description
It turned out, the validation is subject to what is stored in the index. At least for
dateproperties there is an optimization which basically says if the index contains any document with the property value in required range or not. If none - the query is rewritten asMatchNoneQueryand that query is being validated instead of original one (in this case, the validation passes successfully for basically any type of queries or constraints). However, as the new data gets in, the validation and query may start to fail at any moment.The suggested solution for validation flow is to not apply the optimization for
dateproperties but always assume there is a match. As such, the query search/shard contexts have been enriched with new propertyvalidate(): it is set totrueonly when query validation request is served.Note: Another variation of this issue is when query contains properties which do not exist in the index mapping at all. In this case, the validate/search behaviour is consistent: the query is always valid and always returns no results.
Issues Resolved
Fixes #2036
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.