ESQL: Fix for overzealous validation in case of invalid mapped fields #111475
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is more conceptually correct: union typed attributes must not be used in a query, unless they're immediately cast to a single type or dropped. Implementation is haaacky and needs improvement, but it gets the job done.
alex-spies
changed the title
alex-spies
commented
Jul 31, 2024
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java
Outdated
Show resolved
Hide resolved
|
Hi @alex-spies, I've created a changelog YAML for you. |
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
elasticsearchmachine
added
the
Team:Analytics
|
Hi @alex-spies, I've updated the changelog YAML for you. |
alex-spies
commented
Aug 2, 2024
...in/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/TypeResolutions.java
Outdated
Show resolved
Hide resolved
alex-spies
added
the
auto-backport
…aking-unsupported-in-renames
astefan
approved these changes
Aug 8, 2024
x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec
Outdated
Show resolved
Hide resolved
…aking-unsupported-in-renames
bpintea
reviewed
Aug 9, 2024
| @@ -2018,6 +2018,19 @@ M | |||
| null | |||
| ; | |||
|
|
|||
| shadowingInternalWithGroup2#[skip:-8.14.1,reason:implemented in 8.14] | |||
There was a problem hiding this comment.
I guess this is not related, but good to add a shadowing test within the groups in particular.
There was a problem hiding this comment.
This could've tripped due to the logic in the resolution of STATS, which didn't look like it correctly tracks the order of the groups. Turns out it did, because AttributeMap uses a LinkedHashMap internally; but better to have something to guard against regressions.
💔 Backport failed
You can use sqren/backport to manually backport by running |
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
alex-spies
added a commit
to alex-spies/elasticsearch
that referenced
this pull request
Aug 9, 2024
…elastic#111475) Fix validation of fields mapped to different types in different indices and align with validation of fields of unsupported type. * Allow using multi-typed fields in KEEP and DROP, just like unsupported fields. * Explicitly invalidate using both these field kinds in RENAME. * Map both kinds of fields to UnsupportedAttribute to enforce consistency. * Consider convert functions containing valid multi-typed fields as resolved to avoid weird workarounds when resolving STATS. * Add a bunch of tests. (cherry picked from commit 585480f) # Conflicts: # x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Stats.java
elasticsearchmachine
pushed a commit
that referenced
this pull request
Aug 9, 2024
…#111475) (#111735) Fix validation of fields mapped to different types in different indices and align with validation of fields of unsupported type. * Allow using multi-typed fields in KEEP and DROP, just like unsupported fields. * Explicitly invalidate using both these field kinds in RENAME. * Map both kinds of fields to UnsupportedAttribute to enforce consistency. * Consider convert functions containing valid multi-typed fields as resolved to avoid weird workarounds when resolving STATS. * Add a bunch of tests. (cherry picked from commit 585480f) # Conflicts: # x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Stats.java
weizijun
added a commit
to weizijun/elasticsearch
that referenced
this pull request
Aug 9, 2024
* upstream/main: (22 commits) Prune changelogs after 8.15.0 release Bump versions after 8.15.0 release EIS integration (elastic#111154) Skip LOOKUP/INLINESTATS cases unless on snapshot (elastic#111755) Always enforce strict role validation (elastic#111056) Mute org.elasticsearch.xpack.esql.analysis.VerifierTests testUnsupportedAndMultiTypedFields elastic#111753 [ML] Force time shift integration test (elastic#111620) ESQL: Add tests for sort, where with unsupported type (elastic#111737) [ML] Force time shift documentation (elastic#111668) Fix remote cluster credential secure settings reload (elastic#111535) ESQL: Fix for overzealous validation in case of invalid mapped fields (elastic#111475) Pass allow security manager flag in gradle test policy setup plugin (elastic#111725) Rename streamContent/Separator to bulkContent/Separator (elastic#111716) Mute org.elasticsearch.tdigest.ComparisonTests testSparseGaussianDistribution elastic#111721 Remove 8.14 from branches.json Only emit product origin in deprecation log if present (elastic#111683) Forward port release notes for v8.15.0 (elastic#111714) [ES|QL] Combine Disjunctive CIDRMatch (elastic#111501) ESQL: Remove qualifier from attrs (elastic#110581) Force using the last centroid during merging (elastic#111644) ... # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/InferenceNamedWriteablesProvider.java
cbuescher
pushed a commit
to cbuescher/elasticsearch
that referenced
this pull request
Sep 4, 2024
…elastic#111475) Fix validation of fields mapped to different types in different indices and align with validation of fields of unsupported type. * Allow using multi-typed fields in KEEP and DROP, just like unsupported fields. * Explicitly invalidate using both these field kinds in RENAME. * Map both kinds of fields to UnsupportedAttribute to enforce consistency. * Consider convert functions containing valid multi-typed fields as resolved to avoid weird workarounds when resolving STATS. * Add a bunch of tests.
davidkyle
pushed a commit
to davidkyle/elasticsearch
that referenced
this pull request
Sep 5, 2024
…elastic#111475) Fix validation of fields mapped to different types in different indices and align with validation of fields of unsupported type. * Allow using multi-typed fields in KEEP and DROP, just like unsupported fields. * Explicitly invalidate using both these field kinds in RENAME. * Map both kinds of fields to UnsupportedAttribute to enforce consistency. * Consider convert functions containing valid multi-typed fields as resolved to avoid weird workarounds when resolving STATS. * Add a bunch of tests.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fix #111452
#107545 postpones some of the validation for fields that occur in multiple indices with different types. Such fields must not be explicitly used in the query.
The validation now happens after the main reference resolution. This is problematic for renames in particular. Consider a field
athat has the same type in all indices:is perfectly valid. But after reference resolution, the
RENAMEbecomes aPROJECTthat has to list all fields that it will output. If there is afieldthat iskeywordinindex1butipinindex2, we have the followingPerforming the validation now gives wrong results, because it sees
fieldand flags it as invalid;fieldwas not used explicitly, but ended up in the plan due to replacingRENAMEby aPROJECT.This PR reverts to the original approach: during the initial resolution inResolveRefs, we immediately validate usage of a field attribute referring to a multi-typed field. Usage is considered valid if the attribute is either used inside a conversion function, likeTO_IP, or if the attribute is being dropped.Updated with another approach: This PR now aligns how we treat multi-typed fields with how we treat unsupported field types. The latter are already being passed through and are even allowed to be mentioned explicitly in
KEEP,RENAMEDROP. To achieve this, rather than mappingFieldAttributes containing anInvalidMappedFieldtoUnresolvedAttribute- which the verifier finds and counts as invalid query - we map such field attributes toUnsupportedAttribute, which is what fields with unsupported types are also being mapped to.To avoid weird workarounds when resolving
STATS, type resolution for convert functions becomes more lenient, so that multi-typed field attributes (FieldAttributecontaining anInvalidMappedField) are considered resolved as long as all of the mapped types are supported by the convert function; e.g.TO_IP(client_ip)in caseclient_ipis mapped as bothkeywordandip.