Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Reference relevant issue in ReplaceSumWithStats #74396

Merged
merged 3 commits into from
Jun 23, 2021
Merged

Reference relevant issue in ReplaceSumWithStats #74396

merged 3 commits into from
Jun 23, 2021

Conversation

Luegg
Copy link
Contributor

@Luegg Luegg commented Jun 22, 2021 •
edited
Loading

Since the SQL SUM function behaves as expected, #45251 can be closed. As soon as #71582 is resolved, we can go back to using the sum aggregation instead of stats.

Closes elastic#45251
6a50d7f 
Since the SQL `SUM` function behaves as expected, elastic#45251 can be closed.
As soon as elastic#71582 is resolved, we can go back to using the
`sum` aggregation instead of `stats`.
@Luegg Luegg added :Analytics/SQL SQL querying v8.0.0 Team:QL (Deprecated) Meta label for query languages team labels Jun 22, 2021
@Luegg Luegg requested review from costin, astefan, bpintea and matriv June 22, 2021 07:12
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-ql (Team:QL)

bpintea
bpintea approved these changes Jun 22, 2021
View reviewed changes
Copy link
Contributor

@bpintea bpintea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
IMO the description should be rephrased to mention #65796 as the closing PR for #45251 issue, though, not this PR.

@Luegg
Copy link
Contributor Author

Luegg commented Jun 22, 2021

Good point, I removed the "closes" remark and will close #45251 from #65796 as soon as this PR is merged.

@matriv
Copy link
Contributor

matriv commented Jun 22, 2021

@Luegg Thx, please also change the reference in OptimizerTests: https://github.com/elastic/elasticsearch/pull/65796/files#diff-9999ff46c731a755d2ead6a1946409b94fe52bf0f36a2d0bfd834063993a40aeR1047 though

@Luegg
Copy link
Contributor Author

Luegg commented Jun 22, 2021

thanks @matriv, there were even more references.

matriv
matriv approved these changes Jun 22, 2021
View reviewed changes
Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

astefan
astefan approved these changes Jun 22, 2021
View reviewed changes
Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

costin
costin approved these changes Jun 22, 2021
View reviewed changes
Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Luegg Luegg merged commit 48e5a2f into elastic:master Jun 23, 2021
@Luegg Luegg deleted the updateSumWithStatsWorkaround branch June 23, 2021 06:10
@Luegg Luegg mentioned this pull request Jun 23, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matriv matriv matriv approved these changes

@bpintea bpintea bpintea approved these changes

@astefan astefan astefan approved these changes

@costin costin costin approved these changes

Assignees
No one assigned
Labels
:Analytics/SQL SQL querying >non-issue Team:QL (Deprecated) Meta label for query languages team v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Luegg @elasticmachine @matriv @costin @astefan @bpintea @jakelandis