SQL: Refactor usage of NamedExpression#49570
Closed
costin wants to merge 1 commit intoelastic:masterfrom
Closed
Conversation
To recap, Attributes form the properties of a derived table an each LogicalPlan has Attributes as output since each one can be part of a query and its result sent to the user. This change essentially removes the name id comparison so any changes applied to existing functions should work as long as the functions are semantically equivalent. This change enforces the hashCode and equals which has the side-effect of using hashCode as identifiers for each expression. By removing any property from an Attribute, the various components need to look the original source for comparison which, while annoying, should prevent nodes from getting out of sync due to optimizations. Remove the usage of NamedExpression as basis for all Expressions. Instead, restrict their use only for named context, such as projections by using Aliasing instead. Remove different types of Attributes and allow only FieldAttribute, UnresolvedAttribute and ReferenceAttribute. To avoid issues with rewrites, resolve the references inside the QueryContainer so the information always stays on the source. Rename ExpressionId to NameId Side-effect, simplify the rules as the state for InnerAggs doesn't have to be contained anymore. The first commit milestone from refactoring of NamedExpressions. Essentially there are only 3 types of NamedExpressions: Alias - user define (implicit or explicit) name FieldAttribute - field from Elasticsearch ReferenceAttribute - a reference to another source acting as an Attribute.
Collaborator
|
Pinging @elastic/es-search (:Search/SQL) |
Member
Author
|
Note this PR is not yet merged unto master - however all its tests pass in its current form. |
Member
Author
|
Closed by #49693 |
This was referenced Feb 4, 2020
codebird
added a commit
to codebird/elasticsearch
that referenced
this pull request
Feb 5, 2020
This was referenced Feb 13, 2020
Closed
This was referenced Mar 2, 2020
matriv
added a commit
to matriv/elasticsearch
that referenced
this pull request
Apr 16, 2020
Added an integ test to validate behaviour of string scalars on top of aggregate functions. The behaviour was fixed with elastic#49570. Relates to: elastic#41597
matriv
added a commit
that referenced
this pull request
Apr 16, 2020
matriv
added a commit
to matriv/elasticsearch
that referenced
this pull request
Apr 16, 2020
…tic#55304) Added an integration test to validate behaviour of string scalars on top of aggregate functions. The behaviour was fixed with elastic#49570. Relates to: elastic#41597 (cherry picked from commit 35f9641)
matriv
added a commit
that referenced
this pull request
Apr 16, 2020
matriv
added a commit
that referenced
this pull request
Apr 16, 2020
This file contains hidden or 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
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.
To recap, Attributes form the properties of a derived table an each
LogicalPlan has Attributes as output since each one can be part of a
query and its result sent to the user.
This change essentially removes the name id comparison so any changes
applied to existing functions should work as long as the functions are
semantically equivalent.
This change enforces the hashCode and equals which has the side-effect
of using hashCode as identifiers for each expression.
By removing any property from an Attribute, the various components need
to look the original source for comparison which, while annoying, should
prevent nodes from getting out of sync due to optimizations.
Remove the usage of NamedExpression as basis for all Expressions.
Instead, restrict their use only for named context, such as projections
by using Aliasing instead.
Remove different types of Attributes and allow only FieldAttribute,
UnresolvedAttribute and ReferenceAttribute. To avoid issues with
rewrites, resolve the references inside the QueryContainer so the
information always stays on the source.
Rename ExpressionId to NameId
Side-effect, simplify the rules as the state for InnerAggs doesn't have
to be contained anymore.
The first commit milestone from refactoring of NamedExpressions.
Essentially there are only 3 types of NamedExpressions:
Alias - user define (implicit or explicit) name
FieldAttribute - field from Elasticsearch
ReferenceAttribute - a reference to another source acting as an
Attribute.
Relates to #46954