Navigation Menu

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

v3: how to draw an owl #1492

Merged
merged 95 commits into from Mar 17, 2016
Merged

v3: how to draw an owl #1492

merged 95 commits into from Mar 17, 2016

Conversation

sougou
Copy link
Contributor

@sougou sougou commented Feb 9, 2016

@erzel
Ref: http://lmgtfy.com/?q=how+to+draw+an+owl
Well, it's not quite the entire owl, but it's close. As parts get ready, I'll point you to them to perform the review.
In this installment, you may start with doc.go. It's just a description of the high level approaches and issues. There should be no surprises there. You can glance at the rest of the stuff for reference, but comments may be outdated or missing.
I'll probably work on polishing symtab next.

Review on Reviewable

We now need a place to push where clauses and other constructs.
We'll need all constructs of a Select statement since that's what
we're eventually building.
This will be used for the v3 planbuilder.
Skeletal code to identify the rightmost route where
we can push individual parts of a where clause.
We also add flags for RouteBuilder and TableAlias
if they're on the RHS of a left join.
WHERE clause filters can now be pushed down, and we
also perform routing changes if applicable.
There are no colvindexes in the new v3 routes. So, we need a better way
to test that the correct vindexes were chosen.
Still need to write more tests.
Subqueries won't be addressed till the end.
We need to know the Order of left and right nodes for the
join builders. This allow us to know the correct path from
the root node to the necessary RouteBuilder.
This part just reuses the filter functions with some slightly
modified rules.
Changed the symbol table to have slices because map is overkill
for a handful of symbols.
Changed select to use rightmost route, just like where clause.
When symbols get resolved, those decisions need to be persisted.
Otherwise, the symbol table evolves and introduces new symbols
that may hide old ones (when SELECT is parsed). A re-resolution
after the fact can produce inconsistent/incorrect results.
In other words, we need the original resolution to be remembered
when we decide to wire things up at the end.
The symtab Find will now return an error if a reference could not
be resolved. It's also now broken out into another function, Vindex,
which returns a Vindex of one can be found for the expression.
Merging FindInScope and Find, because it wasn't clear when to use
what. Now, Find returns an isLocal flag. This forces you to think
if that needs to be checked in the current context.
The previous scheme was hard to implement at execution time.
In the new scheme, every join node knows contains info about
which vars it needs to supply.
A few features are still to be done, like subquery in FROM
clause, but everything is functional end-to-end, and the old
v3 tests are passing against the new code.
Comprehensive tests need to be written, and the code needs
commenting and cleanup.
Allowing '*' expressions for simple routes. This is for
supporting legacy apps that may still be using this construct.
Renamed some functions for better readability
Adding NEXTVAL as keyword. This lets us control
where this can or cannot be allowed at the parser level.
Also, the argument to NEXTVAL is a table name, which
is technically a different syntax than a normal function.
@aaijazi
Copy link
Contributor

aaijazi commented Mar 6, 2016

Reviewed 1 of 47 files at r1, 4 of 30 files at r2, 1 of 37 files at r3, 2 of 18 files at r9, 1 of 11 files at r10.
Review status: 12 of 61 files reviewed at latest revision, 21 unresolved discussions.


go/vt/vtgate/planbuilder/doc.go, line 45 [r13] (raw file):
Should specify which query is LHS and which is RHS here, and the order that queries are run in (always LHS first, then RHS).


test/vtgatev3_test.py, line 286 [r13] (raw file):
Is there any significance to doing select id, name instead of select *? If so, it would be good to call that out.


test/vtgatev3_test.py, line 753 [r13] (raw file):
I feel like TestJoins would be better as its own class, and the insert queries would be part of a setup method. That way, it's more clear what 's actually under test, and you can extend it out to multiple join cases.


Comments from the review on Reviewable.io

sougou added a commit that referenced this pull request Mar 17, 2016
@sougou sougou merged commit c6ac16b into master Mar 17, 2016
@sougou sougou deleted the suguwork branch June 12, 2016 01:49
dbussink pushed a commit that referenced this pull request Jan 30, 2023
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants