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.

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

SQL: Fix issue with optimization on queries with ORDER BY/LIMIT #40256

Merged
merged 4 commits into from Mar 20, 2019

Conversation

Projects
None yet
5 participants
@matriv
Copy link
Contributor

commented Mar 20, 2019

Previously, when a trival plain SELECT or a trivial SELECT with
aggregations has also an ORDER BY or a LIMIT or both, then the
optimization to convert it to a LocalRelation was skipped resulting
in exception thrown. E.g.::

SELECT 'foo' FROM test LIMIT 10

or

SELECT 'foo' FROM test GROUP BY 1 ORDER BY 1

Fixes: #40211

SQL: Fix issue with optimization on queries with ORDER BY/LIMIT
Previously, when a trival plain `SELECT` or a trivial `SELECT` with
aggregations has also an `ORDER BY` or a `LIMIT` or both, then the
optimization to convert it to a `LocalRelation` was skipped resulting
in exception thrown. E.g.::
```
SELECT 'foo' FROM test LIMIT 10
```
or
```
SELECT 'foo' FROM test GROUP BY 1 ORDER BY 1
```

Fixes: #40211
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2019

@matriv matriv requested review from costin and astefan Mar 20, 2019


private static LocalRelation findLocalRelation(LogicalPlan plan) {
Holder<LocalRelation> logicalPlanHolder = new Holder<>();
plan.transformDown(l -> {

This comment has been minimized.

Copy link
@astefan

astefan Mar 20, 2019

Contributor

Is transformDown really needed here or forEachDown is enough?

This comment has been minimized.

Copy link
@matriv

matriv Mar 20, 2019

Author Contributor

Ah, thx, didn't think of that!

@astefan
Copy link
Contributor

left a comment

LGTM. Left one comment.

matriv added some commits Mar 20, 2019

@costin

costin approved these changes Mar 20, 2019

Copy link
Member

left a comment

LGTM.

@matriv

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

@elasticmachine run elasticsearch-ci/default-distro

@matriv

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

@elasticmachine run elasticsearch-ci/1
@elasticmachine run elasticsearch-ci/2
@elasticmachine run elasticsearch-ci/bwc
@elasticmachine run elasticsearch-ci/docbldesx
@elasticmachine run elasticsearch-ci/oss-distro-docs
@elasticmachine run elasticsearch-ci/packaging-sample

@matriv

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

@elasticmachine run elasticsearch-ci/default-distro
@elasticmachine run elasticsearch-ci/bwc

@matriv matriv merged commit 8832d25 into elastic:master Mar 20, 2019

8 checks passed

CLA All commits in pull request signed
Details
elasticsearch-ci/1 Build finished.
Details
elasticsearch-ci/2 Build finished.
Details
elasticsearch-ci/bwc Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docbldesx Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

@matriv matriv deleted the matriv:mt/fix-40211 branch Mar 20, 2019

matriv added a commit that referenced this pull request Mar 20, 2019

SQL: Fix issue with optimization on queries with ORDER BY/LIMIT (#40256)
Previously, when a trival plain `SELECT` or a trivial `SELECT` with
aggregations has also an `ORDER BY` or a `LIMIT` or both, then the
optimization to convert it to a `LocalRelation` was skipped resulting
in exception thrown. E.g.::
```
SELECT 'foo' FROM test LIMIT 10
```
or
```
SELECT 'foo' FROM test GROUP BY 1 ORDER BY 1
```

Fixes: #40211

matriv added a commit that referenced this pull request Mar 20, 2019

SQL: Fix issue with optimization on queries with ORDER BY/LIMIT (#40256)
Previously, when a trival plain `SELECT` or a trivial `SELECT` with
aggregations has also an `ORDER BY` or a `LIMIT` or both, then the
optimization to convert it to a `LocalRelation` was skipped resulting
in exception thrown. E.g.::
```
SELECT 'foo' FROM test LIMIT 10
```
or
```
SELECT 'foo' FROM test GROUP BY 1 ORDER BY 1
```

Fixes: #40211

matriv added a commit that referenced this pull request Mar 20, 2019

SQL: Fix issue with optimization on queries with ORDER BY/LIMIT (#40256)
Previously, when a trival plain `SELECT` or a trivial `SELECT` with
aggregations has also an `ORDER BY` or a `LIMIT` or both, then the
optimization to convert it to a `LocalRelation` was skipped resulting
in exception thrown. E.g.::
```
SELECT 'foo' FROM test LIMIT 10
```
or
```
SELECT 'foo' FROM test GROUP BY 1 ORDER BY 1
```

Fixes: #40211
@matriv

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

Backported to 7.x with f37f2b5
to 7.0 with d118f85
to 6.7 with 3c00866
to 6.6 with 1b30a62

matriv added a commit that referenced this pull request Mar 21, 2019

SQL: Fix issue with optimization on queries with ORDER BY/LIMIT (#40256)
Previously, when a trival plain `SELECT` or a trivial `SELECT` with
aggregations has also an `ORDER BY` or a `LIMIT` or both, then the
optimization to convert it to a `LocalRelation` was skipped resulting
in exception thrown. E.g.::
```
SELECT 'foo' FROM test LIMIT 10
```
or
```
SELECT 'foo' FROM test GROUP BY 1 ORDER BY 1
```

Fixes: #40211

@michaelbaamonde michaelbaamonde added v7.0.0-rc1 and removed v7.0.0 labels Mar 25, 2019

pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Mar 25, 2019

SQL: Fix issue with optimization on queries with ORDER BY/LIMIT (elas…
…tic#40256)

Previously, when a trival plain `SELECT` or a trivial `SELECT` with
aggregations has also an `ORDER BY` or a `LIMIT` or both, then the
optimization to convert it to a `LocalRelation` was skipped resulting
in exception thrown. E.g.::
```
SELECT 'foo' FROM test LIMIT 10
```
or
```
SELECT 'foo' FROM test GROUP BY 1 ORDER BY 1
```

Fixes: elastic#40211
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.