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

Additional options support for SELECT INTO and LOAD DATA #6872

Merged

Conversation

GuptaManan100
Copy link
Member

@GuptaManan100 GuptaManan100 commented Oct 13, 2020

  1. Support for using SELECT INTO and LOAD DATA on unsharded keyspaces, in addition to already supported bypass queries.

  2. Provide additional options support for SELECT INTO and LOAD statements.

From the MySQL docs:

    INTO OUTFILE 'file_name'
        [CHARACTER SET charset_name]
           [{FIELDS | COLUMNS}
                [TERMINATED BY 'string']
                [[OPTIONALLY] ENCLOSED BY 'char']
                [ESCAPED BY 'char']
            ]
            [LINES
                [STARTING BY 'string']
                [TERMINATED BY 'string']
          ]  

This PR adds support for all the optional options following INTO OUTFILE 'file_name'

…paces

Signed-off-by: GuptaManan100 <manan@planetscale.com>
Signed-off-by: GuptaManan100 <manan@planetscale.com>
Signed-off-by: GuptaManan100 <manan@planetscale.com>
@shlomi-noach
Copy link
Contributor

@GuptaManan100 can you kindly elaborate a bit on what this PR does?

Provided additional options support for SELECT INTO and LOAD statements.

Which options are these?

Also, support for unsharded keyspaces is added apart from bypass queries.

Not sure I parse the sentence. What are the bypass queries?

@shlomi-noach
Copy link
Contributor

Which options are these?

Ignore that question, I can see the options in sql.y.

https://github.com/vitessio/vitess/pull/6872/files#diff-bfff6f6dd088d8318ba5604a902fdf50ed5d0fe1706b82c166c99a67d0d81d39

go/vt/sqlparser/parse_test.go Show resolved Hide resolved
go/vt/vtgate/planbuilder/builder.go Outdated Show resolved Hide resolved
@GuptaManan100
Copy link
Member Author

bypass queries are the ones where the user first specifies the shard that they want the query to run on i.e. "use ks@replica:-80".
Before we do any planning we first check if the destination has already been set, if that is the case then we send the query directly. checkout this function https://github.com/vitessio/vitess/blob/master/go/vt/vtgate/planbuilder/bypass.go#L27 and the place where it is called from https://github.com/vitessio/vitess/blob/master/go/vt/vtgate/planbuilder/builder.go#L300

Signed-off-by: GuptaManan100 <manan@planetscale.com>
…aces

Signed-off-by: GuptaManan100 <manan@planetscale.com>
@systay systay changed the title Additional options support for SELECT INTO and LOAD Additional options support for SELECT INTO and LOAD DATA Oct 13, 2020
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.

This would need few end to end test.

go/vt/sqlparser/ast_funcs.go Show resolved Hide resolved
go/vt/sqlparser/parse_test.go Outdated Show resolved Hide resolved
go/vt/vtgate/planbuilder/builder.go Outdated Show resolved Hide resolved
go/vt/vtgate/planbuilder/route.go Outdated Show resolved Hide resolved
go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@shlomi-noach shlomi-noach left a comment

Choose a reason for hiding this comment

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

Overall this looks very good to me, though this isn't in my comfort zone. I'd change the wording on error messages, or invest in documentation. What "bypass" is, our users cannot know at this time.

Signed-off-by: GuptaManan100 <manan@planetscale.com>
@GuptaManan100
Copy link
Member Author

I have changed the error messages as discussed. I have also added the documentation for "bypass" in my todo list and will do that as a separate PR.

Signed-off-by: GuptaManan100 <manan@planetscale.com>
Signed-off-by: GuptaManan100 <manan@planetscale.com>
…est case

Signed-off-by: GuptaManan100 <manan@planetscale.com>
Signed-off-by: GuptaManan100 <manan@planetscale.com>
Signed-off-by: GuptaManan100 <manan@planetscale.com>
@GuptaManan100 GuptaManan100 marked this pull request as ready for review October 14, 2020 16:02
Signed-off-by: GuptaManan100 <manan@planetscale.com>
Signed-off-by: GuptaManan100 <manan@planetscale.com>
Signed-off-by: GuptaManan100 <manan@planetscale.com>
Signed-off-by: GuptaManan100 <manan@planetscale.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