ES|QL: Validate TOP_SNIPPETS query argument is foldable#142763
ES|QL: Validate TOP_SNIPPETS query argument is foldable#142763carlosdelest merged 29 commits intoelastic:mainfrom
Conversation
…on time Implements compile-time validation for the TOP_SNIPPETS function to ensure that its query parameter is foldable (constant). This follows the same pattern as other full-text functions like Match. Changes: - TopSnippets.java: Implements PostOptimizationVerificationAware interface and adds postOptimizationVerification() method using Foldables.resolveTypeQuery() - TopSnippetsValidationTests.java: New unit tests for validation logic - TopSnippetsTests.java: Updated assertion style per code review feedback - LogicalPlanOptimizerTests.java: Added post-optimization validation tests - top-snippets.csv-spec: Added integration tests for foldable query validation - docs/changelog/142462.yaml: Added changelog entry Fixes elastic#142462
|
Hi @mridula-s109, I've created a changelog YAML for you. |
# Conflicts: # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/TopSnippetsValidationTests.java
|
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
|
Hi @mridula-s109, I've updated the changelog YAML for you. |
carlosdelest
left a comment
There was a problem hiding this comment.
LGTM 👍
Some minor questions about tests
x-pack/plugin/esql/qa/testFixtures/src/main/resources/top-snippets.csv-spec
Outdated
Show resolved
Hide resolved
...rc/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/TopSnippets.java
Show resolved
Hide resolved
| - match: { values.0.2: "Alice Smith" } | ||
|
|
||
| --- | ||
| TOP_SNIPPETS with foldable query: |
There was a problem hiding this comment.
Don't we need a capability for mixed clusters? Will this work when issued against an older cluster? 🤔
x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java
Outdated
Show resolved
Hide resolved
|
Thanks for the comments @carlosdelest and @ioanatia, i am looking into them, will rerequest review post addressing them. |
# Conflicts: # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/TopSnippetsTests.java
…efore checking for Literals
| as(registeredDomain.child(), EsRelation.class); | ||
| } | ||
|
|
||
| public void testTopSnippetsQueryMustBeFoldable() { |
There was a problem hiding this comment.
I initially suggested to @mridula-s109 to add a similar test in VerifierTests, because we already had some tests that were checking for constant arguments.
If the validation is actually done post-optimization, it makes sense to have the test here.
* ES|QL: Validate TOP_SNIPPETS query argument is foldable at verification time Implements compile-time validation for the TOP_SNIPPETS function to ensure that its query parameter is foldable (constant). This follows the same pattern as other full-text functions like Match. Changes: - TopSnippets.java: Implements PostOptimizationVerificationAware interface and adds postOptimizationVerification() method using Foldables.resolveTypeQuery() - TopSnippetsValidationTests.java: New unit tests for validation logic - TopSnippetsTests.java: Updated assertion style per code review feedback - LogicalPlanOptimizerTests.java: Added post-optimization validation tests - top-snippets.csv-spec: Added integration tests for foldable query validation - docs/changelog/142462.yaml: Added changelog entry Fixes elastic#142462 * Update docs/changelog/142763.yaml * [CI] Auto commit changes from spotless * [CI] Auto commit changes from spotless * Update 142763.yaml * Delete docs/changelog/142462.yaml * Removed validation test # Conflicts: # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/TopSnippetsValidationTests.java * Removed the constantQuery Test * Moved validation to verification level * tests added * modified the test cases * Update TopSnippets.java * [CI] Auto commit changes from spotless * fixed bug * [CI] Auto commit changes from spotless * Update docs/changelog/142763.yaml * Add foldable validation to TOP_SNIPPETS query parameter * [CI] Auto commit changes from spotless * Fix YAML tests * Fix tests * Remove protected access * Use postOptimizationVerification to let optimizer to fold constants before checking for Literals * Move test to optimization tests as folding check happens there --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co> Co-authored-by: carlosdelest <carlos.delgado@elastic.co> Co-authored-by: Carlos Delgado <6339205+carlosdelest@users.noreply.github.com>
* ES|QL: Validate TOP_SNIPPETS query argument is foldable at verification time Implements compile-time validation for the TOP_SNIPPETS function to ensure that its query parameter is foldable (constant). This follows the same pattern as other full-text functions like Match. Changes: - TopSnippets.java: Implements PostOptimizationVerificationAware interface and adds postOptimizationVerification() method using Foldables.resolveTypeQuery() - TopSnippetsValidationTests.java: New unit tests for validation logic - TopSnippetsTests.java: Updated assertion style per code review feedback - LogicalPlanOptimizerTests.java: Added post-optimization validation tests - top-snippets.csv-spec: Added integration tests for foldable query validation - docs/changelog/142462.yaml: Added changelog entry Fixes elastic#142462 * Update docs/changelog/142763.yaml * [CI] Auto commit changes from spotless * [CI] Auto commit changes from spotless * Update 142763.yaml * Delete docs/changelog/142462.yaml * Removed validation test # Conflicts: # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/TopSnippetsValidationTests.java * Removed the constantQuery Test * Moved validation to verification level * tests added * modified the test cases * Update TopSnippets.java * [CI] Auto commit changes from spotless * fixed bug * [CI] Auto commit changes from spotless * Update docs/changelog/142763.yaml * Add foldable validation to TOP_SNIPPETS query parameter * [CI] Auto commit changes from spotless * Fix YAML tests * Fix tests * Remove protected access * Use postOptimizationVerification to let optimizer to fold constants before checking for Literals * Move test to optimization tests as folding check happens there --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co> Co-authored-by: carlosdelest <carlos.delgado@elastic.co> Co-authored-by: Carlos Delgado <6339205+carlosdelest@users.noreply.github.com>
Implements compile-time validation for the TOP_SNIPPETS function to ensure that its query parameter is foldable (constant). This follows the same pattern as other full-text functions like Kql/FullTextFunction.
Changes:
TopSnippets.java: Added foldable query validation in resolveType() using Foldables.resolveTypeQuery(), following the Kql/FullTextFunction pattern
TopSnippetsTests.java: Updated assertion style per code review feedback
VerifierTests.java: Added analyzer-level verification tests for query foldability
top-snippets.csv-spec: Added integration tests for foldable query validation
230_folding.yml: Added YAML REST integration tests for folding
docs/changelog/142462.yaml: Added changelog entry
Fixes #142462