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: Allow look-ahead resolution of aliases for WHERE clause #38450

Merged
merged 4 commits into from Feb 6, 2019

Conversation

Projects
None yet
5 participants
@costin
Copy link
Member

commented Feb 5, 2019

Aliases defined in SELECT (Project or Aggregate) are now resolved in the
following WHERE clause. The Analyzer has been enhanced to identify this
rule and replace the field accordingly so that queries such as:

SELECT int AS i FROM t WHERE i > 10

Do not fail anymore (as i was unresolved inside the WHERE clause).

Close #29983

SQL: Allow look-ahead resolution of aliases for WHERE clause
Aliases defined in SELECT (Project or Aggregate) are now resolved in the
following WHERE clause. The Analyzer has been enhanced to identify this
rule and replace the field accordingly.

Close #29983
@elasticmachine

This comment has been minimized.

Copy link

commented Feb 5, 2019

@matriv

matriv approved these changes Feb 5, 2019

Copy link
Contributor

left a comment

LGTM. Nice, this typically is not supported by RDBMS (MySQL, PostgreSQL, etc.)

postgres=# SELECT emp_no AS a FROM test_emp WHERE a > 10;
ERROR:  column "a" does not exist
LINE 1: SELECT emp_no AS a FROM test_emp WHERE a > 10;                                        ^

The reason is that SELECT is supposed to wrap the FROM t WHERE... so WHERE is unaware of the projection. But anyway, it's handy for the user to have it.

@@ -29,6 +30,7 @@
import static java.util.Collections.emptyMap;
import static java.util.Collections.singletonMap;

@TestLogging("org.elasticsearch.xpack.sql:TRACE")

This comment has been minimized.

Copy link
@matriv

matriv Feb 5, 2019

Contributor

Is this on purpose?

This comment has been minimized.

Copy link
@costin

costin Feb 5, 2019

Author Member

No, leftover.

}

private Expression replaceAliases(Expression condition, List<? extends NamedExpression> named) {
List<Alias> aliases = new ArrayList<>();

This comment has been minimized.

Copy link
@matriv

matriv Feb 5, 2019

Contributor

Wouldn't be more efficient to have a Map<String, Alias> where the key is the alias.name() and avoid iteration over a list?

This comment has been minimized.

Copy link
@costin

costin Feb 5, 2019

Author Member

It is; will update.

This comment has been minimized.

Copy link
@costin

costin Feb 5, 2019

Author Member

Actually scratch that - it's not that easy due to the qualifier which might be present or not. I'll update the resolution though.

@astefan

astefan approved these changes Feb 5, 2019

Copy link
Contributor

left a comment

LGTM


return condition.transformDown(u -> {
for (Alias alias : aliases) {
if (alias.name().equals(u.name()) || Objects.equals(alias.qualifiedName(), u.qualifiedName())) {

This comment has been minimized.

Copy link
@matriv

matriv Feb 5, 2019

Contributor

Is there a case where the names are not equal but the qualifiedName() are equal?
Couldn't we check just the qualifiedName()?
If there is such case can we have it in a test?

This comment has been minimized.

Copy link
@costin

costin Feb 5, 2019

Author Member

An alias can be qualified as part of a subquery - the qualifiedName doesn't work in cases where one side is qualified the other isn't (the names are equal by one of the qualifier is missing).
I couldn't come up with a test case however I've updated the comparison to be in sync with that of the identifier resolution (in simplified form).

@costin

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2019

@elasticmachine run elasticsearch-ci/2

@costin costin merged commit 1a02445 into elastic:master Feb 6, 2019

7 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci/1 Build finished.
Details
elasticsearch-ci/2 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

@costin costin deleted the costin:fix-29983 branch Feb 6, 2019

@costin

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2019

costin added a commit that referenced this pull request Feb 6, 2019

SQL: Allow look-ahead resolution of aliases for WHERE clause (#38450)
Aliases defined in SELECT (Project or Aggregate) are now resolved in the
following WHERE clause. The Analyzer has been enhanced to identify this
rule and replace the field accordingly.

Close #29983

(cherry picked from commit 1a02445)

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 6, 2019

Merge remote-tracking branch 'elastic/master' into enable-bwc-retenti…
…on-leases-recovery

* elastic/master:
  SQL: Allow look-ahead resolution of aliases for WHERE clause (elastic#38450)
  Add API key settings documentation (elastic#38490)
  Remove support for maxRetryTimeout from low-level REST client (elastic#38085)
  Update IndexTemplateMetaData to allow unknown fields (elastic#38448)
  Mute testRetentionLeasesSyncOnRecovery (elastic#38488)
  Change the min supported version to 6.7.0 for API keys (elastic#38481)

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

dimitris-athanasiou added a commit to dimitris-athanasiou/elasticsearch that referenced this pull request Feb 12, 2019

SQL: Allow look-ahead resolution of aliases for WHERE clause (elastic…
…#38450)

Aliases defined in SELECT (Project or Aggregate) are now resolved in the
following WHERE clause. The Analyzer has been enhanced to identify this
rule and replace the field accordingly.

Close elastic#29983
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.