ESQL: Remove qualifier from attrs #110581
Conversation
Qualifier is unused.
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
elasticsearchmachine
added
the
Team:Analytics
There was a problem hiding this comment.
Thanks Alex, LGTM
In the analyzer, the qualifier is always set to null
This alone justifies the removal IMHO.
There is a chance that we'll need similar concepts when we implement joins, but my gut feeling says that we'll adopt completely different solutions (as we do for LOOKUP and ENRICH).
And in general, I'd rather add a piece of information only when I need it, to avoid biases in the design of future features.
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/AnalyzerRules.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/AnalyzerRules.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Alias.java
Outdated
Show resolved
Hide resolved
...gin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java
Outdated
Show resolved
Hide resolved
|
Discussed this offline with @costin . It's not unlikely that we'll require qualifiers again in some capacity, esp. to improve the language in cases when join-like commands like However, removing qualifiers for now is still advantageous because until qualifiers come back, we have clarity on their usage (none), and if/when they come back, we'll see in the diff exactly where and how they are required for new language features. For instance, it seems reasonable that only I'll try to merge this as soon as CI is happy. |
|
Thanks for the reviews @luigidellaquila , @astefan and @bpintea ! |
* 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
In ES|QL's analyzer, we resolve names to field attributes, like some_field in FROM some_idx | KEEP some_field. The Attribute class, being copied from (S)QL, has a name and a qualified name, e.g. some_field and qualifier.some_field. However, we never use the qualifier, so remove it.
In ES|QL's analyzer, we resolve names to field attributes, like some_field in FROM some_idx | KEEP some_field. The Attribute class, being copied from (S)QL, has a name and a qualified name, e.g. some_field and qualifier.some_field. However, we never use the qualifier, so remove it.
In ES|QL's analyzer, we resolve names to field attributes, like
some_fieldinFROM some_idx | KEEP some_field.The
Attributetype, being copied from (S)QL, has a name and a qualified name, e.g.some_fieldandsome_idx.some_field.However, we never use the qualifier (
some_idxin above's example). In the analyzer, the qualifier is always set tonulland during optimization, we often do not take the qualified name into account, because we know the name alone matters.This is due to ES|QL's treatment of names, which is different from SQL's: at each point in the query, the (intermediate) result's columns must have unique names.
Let's remove
Attribute.qualifier. As it is right now, it is confusing because one doesn't know if it may or may not contain relevant info, and especially theAttribute.clusterprivate member looks like it does something for cross cluster queries, but it really doesn't.