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

Fix validation rule to detect tuples in projections or groupbys #672

Merged
merged 4 commits into from
Apr 23, 2019

Conversation

juanjux
Copy link
Contributor

@juanjux juanjux commented Apr 11, 2019

Fixes src-d/gitbase/issues/785

Modifies a rule to check if distinct have a tuple argument (DISTINCT(1,2)), recursively. Unrelatedly, it also fix ordering in the README for the FROM_BASE64 function (my fault).

There are also a couple unrelated files with go fmt changes which I decided to add to this PR since it won't hurt.

Signed-off-by: Juanjo Alvarez juanjo@sourced.tech

@juanjux juanjux self-assigned this Apr 11, 2019
@juanjux juanjux added the bug Something isn't working label Apr 11, 2019
@juanjux juanjux force-pushed the distinct-tuple branch 2 times, most recently from 82b9ecb to c7e7418 Compare April 11, 2019 17:33
sql/analyzer/optimization_rules.go Show resolved Hide resolved
sql/analyzer/validation_rules.go Show resolved Hide resolved
sql/analyzer/resolve_columns.go Outdated Show resolved Hide resolved
@erizocosmico
Copy link
Contributor

It should not only be distinct, but anything that is projected. A tuple simply can't be projected (neither in Project nor GroupBy)

@juanjux juanjux force-pushed the distinct-tuple branch 3 times, most recently from d038f7f to 1ba012b Compare April 17, 2019 10:39
@juanjux juanjux changed the title [Read comment!] Validation rule to detect tuples as distinct argument Validation rule to detect tuples in projections or groupbys Apr 17, 2019
@juanjux
Copy link
Contributor Author

juanjux commented Apr 17, 2019

@erizocosmico change to detect tuples in projections and groupbys instead of distinct implemented, PTAL when you can.

sql/analyzer/resolve_columns.go Outdated Show resolved Hide resolved
sql/analyzer/resolve_columns_test.go Outdated Show resolved Hide resolved
sql/analyzer/rules.go Outdated Show resolved Hide resolved
@juanjux
Copy link
Contributor Author

juanjux commented Apr 22, 2019

Update: while grepping around noticed that there was already a validation rule checking that there are not tuples in Projections or GroupBys (https://github.com/src-d/go-mysql-server/blob/master/sql/analyzer/validation_rules.go#L198), it even have tests with tuples (https://github.com/src-d/go-mysql-server/blob/master/sql/analyzer/validation_rules_test.go#L234) but adding a test with a DISTINCT->Projection->Tuple doesn't detect the error (and it crash like the issue reported if you do it on the REPL), so I'm now investigating why this existing rule doesn't work for this case.

I think that what @kuba-- said above that the rule needs to inspect nodes recursively would be the right fix.

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
@juanjux juanjux requested a review from ajnavarro April 22, 2019 13:35
@juanjux
Copy link
Contributor Author

juanjux commented Apr 22, 2019

Yes, with a recursive inspection the existing rule works beautifully, PTAL again since I removed basically everything from the previous commits and rewrote again.

Juanjo Alvarez added 3 commits April 22, 2019 15:45
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
@juanjux juanjux changed the title Validation rule to detect tuples in projections or groupbys Fix validation rule to detect tuples in projections or groupbys Apr 23, 2019
@ajnavarro ajnavarro merged commit 3fd2047 into src-d:master Apr 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

panic on SELECT DISTINCT(1, 2);
4 participants