-
Notifications
You must be signed in to change notification settings - Fork 564
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
Encapsulate CreateTable
, CreateIndex
into specific structs
#1291
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somehow "trivial" changes :D
LGTM.
I have a branch for |
@philipcristiano either should work. If it's likely to conflict, I'd prefer to add on this one. As I suppose it would be not quite complex. |
0044f1d
to
f811ddf
Compare
Added to this PR!
|
I started the CI checks |
Pull Request Test Coverage Report for Build 9320099104Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
f811ddf
to
c55fa4b
Compare
CreateTable
into specific structCreateTable
, CreateIndex
into specific structs
Updated to fix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me -- thank you @philipcristiano and @tisonkun
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One suggestion: We may implement Display
on this struct so that it's more smooth to use downstream.
Currently, IIUC we use the pub
fields and implement the top level Display
manually.
5c418b0
to
ff2cf96
Compare
Move the Statement Display implementation to the CreateTable struct
ff2cf96
to
ebb57a5
Compare
I moved the CreateTable display implementation from Statement to the struct. |
Is there anything else that I should do before this could be merged? |
Hi @philipcristiano -- sorry -- no you don't need to do anything else. I am somewhat behind as I have been struggling to update DataFusion with the latest sqlparser release and thus haven't had bandwidth to merge more changes to SQL parser that have significant breaking changes I think I am mostly unstuck now and will merge this one shortly |
Awesome! Thank you for your help on this issue and your work on this project! |
Move the Display trait `fmt` implementation to the CreateIndex struct to to allow for display! The struct was created in a0f511c with the CreateTable `fmt` being moved, but the CreateIndex was not. Follows up from apache#1291
cc @alamb, @tisonkun
Continue moving structs as part of #1204
The
CreateTableBuilder
could maybe be combined into theCreateTable
. For the purposes of minimizing breaking changes I did not attempt to merge them.