Skip to content

SQL: Refactor usage of NamedExpression#49570

Closed
costin wants to merge 1 commit intoelastic:masterfrom
costin:refactor_named_expression_2
Closed

SQL: Refactor usage of NamedExpression#49570
costin wants to merge 1 commit intoelastic:masterfrom
costin:refactor_named_expression_2

Conversation

@costin
Copy link
Copy Markdown
Member

@costin costin commented Nov 25, 2019

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

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.
@elasticmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-search (:Search/SQL)

@costin
Copy link
Copy Markdown
Member Author

costin commented Nov 25, 2019

Note this PR is not yet merged unto master - however all its tests pass in its current form.

@costin
Copy link
Copy Markdown
Member Author

costin commented Nov 28, 2019

Closed by #49693

@costin costin closed this Nov 28, 2019
@costin costin deleted the refactor_named_expression_2 branch December 10, 2019 11:39
codebird added a commit to codebird/elasticsearch that referenced this pull request Feb 5, 2020
matriv pushed a commit that referenced this pull request Feb 7, 2020
…#42121)

The related issue regarding aggregation queries where some literals
are also selected together with aggregate function has been fixed
with #49570. Add integration tests to verify the behavior.

Relates to: #41411
matriv pushed a commit that referenced this pull request Feb 7, 2020
…#42121)

The related issue regarding aggregation queries where some literals
are also selected together with aggregate function has been fixed
with #49570. Add integration tests to verify the behavior.

Relates to: #41411

(cherry picked from commit 9f414a8)
matriv pushed a commit that referenced this pull request Feb 7, 2020
…#42121)

The related issue regarding aggregation queries where some literals
are also selected together with aggregate function has been fixed
with #49570. Add integration tests to verify the behavior.

Relates to: #41411

(cherry picked from commit 9f414a8)
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
Added an integration test to validate behaviour of string scalars on top
of aggregate functions. The behaviour was fixed with #49570.

Relates to: #41597
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
…) (#55310)

Added an integration test to validate behaviour of string scalars on top
of aggregate functions. The behaviour was fixed with #49570.

Relates to: #41597

(cherry picked from commit 35f9641)
matriv added a commit that referenced this pull request Apr 16, 2020
…) (#55309)

Added an integration test to validate behaviour of string scalars on top
of aggregate functions. The behaviour was fixed with #49570.

Relates to: #41597

(cherry picked from commit 35f9641)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants