Skip to content
This repository has been archived by the owner on Jan 28, 2021. It is now read-only.

sql/analyzer: fix use of declared aliases in the same projection #609

Merged
merged 1 commit into from
Jan 30, 2019

Conversation

mcarmonaa
Copy link
Contributor

Fixes #590

Since this is just a SQL syntax issue finally I've just added a new rule to check the aliases usage at the beginning of the analysis phase that runs once before the default rules. This way it doesn't interfere with the transformations made by the rest of the rules.

 src-d#590]

Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>
@mcarmonaa
Copy link
Contributor Author

asking for a code review to the new awesome @src-d/data-processing team

@@ -27,6 +27,7 @@ var DefaultRules = []Rule{
var OnceBeforeDefault = []Rule{
{"resolve_subqueries", resolveSubqueries},
{"resolve_tables", resolveTables},
{"check_aliases", checkAliases},
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be a validation rule

Copy link
Contributor Author

@mcarmonaa mcarmonaa Jan 29, 2019

Choose a reason for hiding this comment

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

validation rules are executed after regular rules, this must be executed on the parsed plan before transformations

Copy link
Contributor

Choose a reason for hiding this comment

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

But it's a validation rule. Why can't it be run in validation phase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since validation phase runs after the default rules, the plan to be validated could be modified enough to not keep this alias case.

An alias defined and used in the same projection in the original query, it couldn't be there in the plan after the transformations (reorder/remove/pushdown/joins...) (that's the case of the issue).

To be sure that a query which is making this is not allowed, the check must be done on the parsed plan or on a plan near to it.

@ajnavarro ajnavarro merged commit 31ad0f9 into src-d:master Jan 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants