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

[Sqlparser] VALUES(...) should take a fully-qualified column name #3883

Merged
merged 1 commit into from
May 11, 2018

Conversation

danieltahara
Copy link

@danieltahara danieltahara commented May 1, 2018

A values expression is allowed to take a fully qualified (table.col), but the current parser only allows unqualified columns. I have modified the grammar, plan builder, and AST accordingly.

Signed-off-by: Daniel Tahara tahara@dropbox.com

@sougou
Copy link
Contributor

sougou commented May 1, 2018

More changes are needed. See the build failure :).

@danieltahara
Copy link
Author

Yep yep :) It's easier to punt these up here and have all the ci run than set it up locally :P

@danieltahara danieltahara force-pushed the insert-on-dup branch 4 times, most recently from fed5fbb to 58bb282 Compare May 1, 2018 22:08
@danieltahara
Copy link
Author

Added a negative test (mistmatched table name) and rebased.

@sougou sougou self-requested a review May 2, 2018 00:11
@acharis
Copy link
Contributor

acharis commented May 2, 2018

LGTM

@danieltahara
Copy link
Author

Bump :)

@sougou
Copy link
Contributor

sougou commented May 3, 2018

The ETA on this is probably weekend. I'll try to find time before if possible.

@danieltahara
Copy link
Author

bump :)

@sougou
Copy link
Contributor

sougou commented May 9, 2018

I've started working my way through. I should get to this soon enough now.

Copy link
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

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

@rafael Can you take a look at this? This code needs to be modified to accommodate your recent change for handling ondup constructs.

if !node.Name.Qualifier.IsEmpty() && node.Name.Qualifier != ins.Table {
formatErr = vterrors.Errorf(vtrpcpb.Code_NOT_FOUND,
"could not find qualified column %v in table %v",
node.Name, ins.Table)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use sqlparser.String(node.Name) instead. It will make the error more readable. Same for table name.

Copy link
Author

Choose a reason for hiding this comment

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

done, and updated tests.

@rafael
Copy link
Member

rafael commented May 10, 2018

@sougou - LGTM.

@danieltahara - To make this change compatible with mine, I think you just need to change this:

if !valueExpr.Name.Equal(assignment.Name.Name) {

to:

if !valueExpr.Name.Name.Equal(assignment.Name.Name) {

In: https://github.com/vitessio/vitess/blob/master/go/vt/vtgate/planbuilder/insert.go#L243

@danieltahara
Copy link
Author

fixed. :)

A values expression is allowed to take a fully qualified (table.col),
but the current parser only allows unqualified columns. I have modified
the grammar, plan builder, and AST accordingly.

Signed-off-by: Daniel Tahara <tahara@dropbox.com>
@sougou sougou merged commit c75d1c9 into vitessio:master May 11, 2018
@danieltahara danieltahara deleted the insert-on-dup branch May 11, 2018 01:19
notfelineit pushed a commit to planetscale/vitess that referenced this pull request Mar 12, 2024
…n VPlayer: support statement and transaction batching (vitessio#3883)

* cherry pick of 14502 (vitessio#3881)

Signed-off-by: Matt Lord <mattalord@gmail.com>

* Corrections

Signed-off-by: Matt Lord <mattalord@gmail.com>

---------

Signed-off-by: Matt Lord <mattalord@gmail.com>
Co-authored-by: Matt Lord <mattalord@gmail.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.

4 participants