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

*: support AST to SQL text #56

Merged
merged 28 commits into from
Dec 4, 2018
Merged

*: support AST to SQL text #56

merged 28 commits into from
Dec 4, 2018

Conversation

zier-one
Copy link
Contributor

@zier-one zier-one commented Nov 29, 2018

1、add Recoverable interface
2、add Recoverable implement example(CreateDatabaseStmt,DropDatabaseStmt,UnaryOperationExpr)
3、add stmt Recoverable test frame
4、replace the tidb dependency to github.com/leoppro/tidb temporarily,
github.com/leoppro/tidb forked from pingcap/tidb and impl the Recoverable interface for ValueExpr and ParamMarkerExpr, when it is stable,i will pull it to pingcap/tidb

@zier-one
Copy link
Contributor Author

PTAL @GregoryIan @tiancaiamao

@zier-one zier-one changed the base branch from master to ast2sqltext November 29, 2018 04:21
@XuHuaiyu
Copy link
Contributor

It'll be nice if you can create an issue in the tidb repo to trace the process of this task.
We can list the sub-tasks in the issue, thus the community can choose their interested tasks to participate.

This can be referred. pingcap/tidb#7623

@zier-one
Copy link
Contributor Author

@kennytm PTAL again

Copy link
Contributor

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

Rest LGTM

@tiancaiamao
Copy link
Collaborator

tiancaiamao commented Nov 29, 2018

LGTM
Don't forget to address comment from kenny @leoppro

@kennytm kennytm added the status/LGT1 LGT1 label Nov 29, 2018
kennytm
kennytm previously approved these changes Nov 29, 2018
Copy link
Contributor

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

LGTM

@zier-one
Copy link
Contributor Author

@kennytm kennytm added the status/LGT1 LGT1 label Nov 30, 2018
@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Dec 3, 2018

PTAL @tiancaiamao @zz-jason

@zz-jason zz-jason dismissed kennytm’s stale review December 3, 2018 08:14

needs one more LGTM

@zz-jason
Copy link
Member

zz-jason commented Dec 3, 2018

If there are still some comments not addressed and you think they should be addressed, please do not comment a "LGTM" @tiancaiamao

How about using the visitor model? Implement a visitor visits the AST tree, complement the sql during the traverse? Thus we don't need to introduce a Recoverable interface and change the AST node implementations. @leoppro

@zier-one
Copy link
Contributor Author

zier-one commented Dec 3, 2018

How about using the visitor model? Implement a visitor visits the AST tree, complement the sql during the traverse? Thus we don't need to introduce a Recoverable interface and change the AST node implementations. @leoppro

I have considered this way, it's hard to implement.
Firstly, we should use type assertion to handle different ast.Node, it will be complicated;
Secondly, we can‘t control visiting order for child node.

Copy link
Collaborator

@tiancaiamao tiancaiamao left a comment

Choose a reason for hiding this comment

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

I think Restore interface is necessary to implement AST to SQL
Maybe we can add a method to Node, rather than define a Restorable interface.

@zz-jason
Copy link
Member

zz-jason commented Dec 3, 2018

How about using the visitor model? Implement a visitor visits the AST tree, complement the sql during the traverse? Thus we don't need to introduce a Recoverable interface and change the AST node implementations. @leoppro

I have considered this way, it's hard to implement.
Firstly, we should use type assertion to handle different ast.Node, it will be complicated;
Secondly, we can‘t control visiting order for child node.

Got it!

@zier-one
Copy link
Contributor Author

zier-one commented Dec 3, 2018

I think Restore interface if necessary to implement AST to SQL
Maybe we can add a method to Node, rather than define a Restorable interface.

some struct which isn't a Node need implement Restorable too, such as DatabaseOption

@tiancaiamao tiancaiamao changed the title *: support AST to SQL text ``*: support AST to SQL text Dec 3, 2018
@tiancaiamao
Copy link
Collaborator

type Node interface {
    Recoverable
    ...
}

Using embed interface , you still need to implement Restore method for DababaseOption @leoppro

So what's the difference if you define Node like this ?

type Node interface {
    Recover(builder) error
    ...
}

@tiancaiamao
Copy link
Collaborator

And if you define the Recover() method in the node struct (the conceptual base class), it would save you many codes to define

func (xx XXNode) Recover() error {
   return errors.New("not implemented")
}

@leoppro

@tiancaiamao tiancaiamao changed the title ``*: support AST to SQL text *: support AST to SQL text Dec 3, 2018
@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Dec 4, 2018

LGTM

@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Dec 4, 2018

PTAL @zz-jason @tiancaiamao

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added status/LGT2 LGT2 and removed status/LGT1 LGT1 labels Dec 4, 2018
Copy link
Contributor

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

(rest LGTM)

@zier-one zier-one merged commit ce4d755 into pingcap:master Dec 4, 2018
@zier-one zier-one deleted the ast2sql branch December 4, 2018 04:03
tiancaiamao pushed a commit to tiancaiamao/parser that referenced this pull request Apr 27, 2021
* ast2sql demo

* add SQLSentence

* mock func Restore

* change copyright

* add comment

* add test

* change tidb dependentment

* fix error comment

* use strings.Builder

* fix test

* add error return value for Recoverable

* add error return value for Recoverable

* add error return value for Recoverable

* impl Restore of UnaryOperationExpr

* integrate utils

* switch tidb dependency

* add comment

* add comment

* update unit test

* fix Restore of DatabaseOption

* fix Restore of DatabaseOption

* fix Restore of DatabaseOption

* update unit test

* use errorf
lyonzhi pushed a commit to lyonzhi/parser that referenced this pull request Apr 25, 2024
* ast2sql demo

* add SQLSentence

* mock func Restore

* change copyright

* add comment

* add test

* change tidb dependentment

* fix error comment

* use strings.Builder

* fix test

* add error return value for Recoverable

* add error return value for Recoverable

* add error return value for Recoverable

* impl Restore of UnaryOperationExpr

* integrate utils

* switch tidb dependency

* add comment

* add comment

* update unit test

* fix Restore of DatabaseOption

* fix Restore of DatabaseOption

* fix Restore of DatabaseOption

* update unit test

* use errorf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants