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

Update AST helper generation #7558

Merged
merged 42 commits into from
Mar 2, 2021
Merged

Conversation

systay
Copy link
Collaborator

@systay systay commented Feb 26, 2021

Description

This PR removes the visitorgen component that used to create the Vitess Rewriter, and adds asthelpergen that generates both the rewriter, but also deep-clone methods for all AST types.

The Gen4 planner tries join in both directions. Given the following query:

SELECT a.col, b.col FROM A JOIN B ON a.id = b.id

The new planner will try to first plan the A side on the left, and the send data from A to B as params, and then will try the other variation. The two versions would be:

-- ********** A JOIN B
-- A side
SELECT a.col FROM A
-- B side
SELECT b.col FROM B WHERE ? = b.id



-- ********** B JOIN A
-- B side
SELECT b.col FROM B
-- B side
SELECT b.col FROM A WHERE a.id = ?

As can be seen in these examples, we need to be able to clone the original predicate and then change it in two different ways to test both alternatives.

Related Issue(s)

#7280

Checklist

  • Should this PR be backported? - Absolutely not
  • Tests were added or are not required - Lots of tests added
  • Documentation was added or is not required - No docs needed. Internal change only

Impacted Areas in Vitess

Components that this PR will affect:

  • Query Serving

systay and others added 30 commits February 12, 2021 07:49
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
@systay systay changed the title Deep clone Update AST helper generation Feb 27, 2021
@systay systay marked this pull request as ready for review February 27, 2021 14:23
This was referenced Feb 27, 2021
Copy link
Collaborator

@vmg vmg left a comment

Choose a reason for hiding this comment

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

Looks good to me! We spent quite a bit of time reviewing this together. ✨

return nil
}

func (gen *astHelperGen) GenerateCode() (map[string]*jen.File, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a comment for an exported function?

Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
This reverts commit cd7c4b3.

Turns out this change significantly degrades performance, and we don't want that...

Signed-off-by: Andres Taylor <[email protected]>
@vmg
Copy link
Collaborator

vmg commented Mar 2, 2021

@systay I now see where the performance regression in cd7c4b3 comes from -- you're actually allocating closures now! By removing the extra argument to every replacement function, all the visitor callbacks that were previously pure functions are becoming closures, which require an allocation even if they're not being called. Definitely a performance regression.

Copy link
Member

@harshit-gangal harshit-gangal left a comment

Choose a reason for hiding this comment

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

Really nice work.

Comment on lines +136 to +140
const (
// these consts are here to try to trick the generator
thisIsNotAType BasicType = 1
thisIsNotAType2 BasicType = 2
)
Copy link
Member

Choose a reason for hiding this comment

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

looks like they are not used in the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the comment is not lying. these consts produce code that we had to do extra checks in the generator code to avoid. They are here to mimic sqlparser types such as ReadOnly (https://github.com/vitessio/vitess/blob/master/go/vt/sqlparser/constants.go#L254)

func (tv *testVisitor) assertEquals(t *testing.T, expected []step) {
t.Helper()
var lines []string
error := false
Copy link
Member

Choose a reason for hiding this comment

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

nit: error is reserved word, better to name as err

@harshit-gangal harshit-gangal merged commit a75c159 into vitessio:master Mar 2, 2021
@harshit-gangal harshit-gangal deleted the deep-clone branch March 2, 2021 12:30
@askdba askdba added this to the v10.0 milestone Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants