Zachmu/analyzer #88
Merged
Zachmu/analyzer #88
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
…ve columns Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
…their own file Signed-off-by: Zach Musgrave <zach@liquidata.co>
…rywhere Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
…ary table key expressions in the order specified by the secondary index. Signed-off-by: Zach Musgrave <zach@liquidata.co>
…aliases. Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
…one of two broken tests Signed-off-by: Zach Musgrave <zach@liquidata.co>
…nation and ordering, for correctness and query plan. Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
…that weren't adding anything Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
|
Pretty well tested, but take a quick look for any obvious problems. |
bheni
approved these changes
Apr 20, 2020
| @@ -76,7 +77,7 @@ func TestMisusedAlias(t *testing.T) { | |||
| } | |||
|
|
|||
| func TestQualifyColumns(t *testing.T) { | |||
| require := require.New(t) | |||
There was a problem hiding this comment.
Don't have an issue with this... but why the preference for assert in this case?
There was a problem hiding this comment.
require stops the test on the first failed check, assert lets it continue.
sql/analyzer/resolve_columns.go
Outdated
| for _, col := range child.Schema() { | ||
| childSch := child.Schema() | ||
| for _, col := range childSch { | ||
| // columns of tables with an alias can be referred to either by their aliased or unaliased name, so add an entry |
There was a problem hiding this comment.
sentence is wordy and hard to read. Maybe take out the word "either".
sql/analyzer/analyzer.go
Outdated
| Debug bool | ||
| // Whether to output the query plan at each step of the analyzer | ||
| Verbose bool | ||
| debugCtx []string |
There was a problem hiding this comment.
Reading the code below I had to scroll back up to see what debugCtx was. I read Ctx and think context.Context. Feel like another name would clear up some confusion.
sql/analyzer/fix_field_indexes.go
Outdated
| return result, nil | ||
| } | ||
|
|
||
| // fixFieldIndexes transforms the given expression setting correct indexes |
There was a problem hiding this comment.
"the given expression setting correct indexes for GetField"
wat?
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Analyzer improvements, mostly related to treatment of aliases to support selecting the same table twice, but lots of other improvements as well.