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: return constants for all matching records in constants-containing SELECTs #34576

Merged
merged 2 commits into from Oct 18, 2018

Conversation

@astefan
Copy link
Contributor

commented Oct 18, 2018

This PR fixes #31863 by:

  • making a constant to be used outside aggregation only queries: SELECT 123 FROM index
  • not skipping an ES query in case of constants-only selects
  • loosen the binary pipe restriction of being used only in aggregation queries
astefan added 2 commits Oct 18, 2018
A constant can be used outside aggregation only queries
Don't skip an ES query in case of constants-only selects
Loosen the binary pipe restriction of being used only in aggregation queries
Fixes #31863

@astefan astefan requested review from costin and matriv Oct 18, 2018

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Oct 18, 2018

Pinging @elastic/es-search-aggs

@matriv
matriv approved these changes Oct 18, 2018
Copy link
Contributor

left a comment

LGTM. nice!

@costin
costin approved these changes Oct 18, 2018
Copy link
Member

left a comment

LGTM

@@ -1796,7 +1797,7 @@ protected LogicalPlan rule(LogicalPlan plan) {
if (plan instanceof Project) {
Project p = (Project) plan;
List<Object> values = extractConstants(p.projections());
if (values.size() == p.projections().size()) {
if (values.size() == p.projections().size() && !(p.child() instanceof EsRelation)) {

This comment has been minimized.

Copy link
@costin

costin Oct 18, 2018

Member

This causes a match_all query to be issued which is not ideal - I've raised a separate issue to optimize this properly (#34579). In the meantime, it's good as is.

@astefan astefan merged commit 60b8118 into elastic:master Oct 18, 2018

4 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details
astefan added a commit to astefan/elasticsearch that referenced this pull request Oct 18, 2018
A constant can be used outside aggregation only queries (elastic#34576)
A constant can now be used outside aggregation only queries.
Don't skip an ES query in case of constants-only selects.
Loosen the binary pipe restriction of being used only in aggregation queries.
Fixes elastic#31863

@astefan astefan deleted the astefan:34540_fix branch Oct 18, 2018

kcm added a commit that referenced this pull request Oct 30, 2018
A constant can be used outside aggregation only queries (#34576)
A constant can now be used outside aggregation only queries.
Don't skip an ES query in case of constants-only selects.
Loosen the binary pipe restriction of being used only in aggregation queries.
Fixes #31863

@colings86 colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.