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.

Sign up for GitHub

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

Jump to bottom

Implement ToSql trait which displays AST as SQL text format #554

Merged
merged 49 commits into from Jun 6, 2022
Merged

Implement ToSql trait which displays AST as SQL text format #554

merged 49 commits into from Jun 6, 2022

Conversation

ghost
Copy link

@ghost ghost commented May 16, 2022 •
edited by ghost
Loading

Implementing this so that we can add a definition/description column for the show indexes command.

For example, if we have the following expression;

Expr::BinaryOp {
                left: Box::new(Expr::Identifier("id".to_string())),
                op: BinaryOperator::Plus,
                right: Box::new(Expr::Identifier("num".to_string()))
 }

this would be converted to:
id + num

@codecov-commenter
Copy link

codecov-commenter commented May 16, 2022 •
edited
Loading

Codecov Report

Merging #554 (043896b) into main (65f744b) will decrease coverage by 0.25%.
The diff coverage is 79.91%.

@@            Coverage Diff             @@
##             main     #554      +/-   ##
==========================================
- Coverage   93.80%   93.54%   -0.26%     
==========================================
  Files         185      188       +3     
  Lines       12551    12790     +239     
==========================================
+ Hits        11774    11965     +191     
- Misses        777      825      +48
Impacted Files Coverage Δ
core/src/ast/operator.rs 65.85% <48.14%> (-34.15%) ⬇️
core/src/ast/ast_literal.rs 83.33% <83.33%> (ø)
core/src/ast/expr.rs 83.52% <83.52%> (ø)
core/src/ast/function.rs 91.66% <91.66%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65f744b...043896b. Read the comment docs.

@ghost
Copy link
Author

ghost commented May 16, 2022

I am thinking about merging this PR with PR #551 since the decode function will primarily be used with the show indexes from table functionality. Should I keep it as a separate PR, or merge with #551?

@ghost ghost marked this pull request as ready for review May 16, 2022 18:39
@ever0de ever0de assigned ghost May 17, 2022
@ever0de ever0de requested review from panarch and ever0de May 17, 2022 07:59
@ghost ghost marked this pull request as draft May 20, 2022 22:30
@ghost
Copy link
Author

ghost commented May 20, 2022

Will continue this PR once PR #551 is merged. Changed status to draft until then..

@ghost ghost changed the title implement procedure to create psuedo SQL statetment fragments from Expr enums. Add description column to show indexes from table command May 26, 2022
@ghost ghost marked this pull request as ready for review May 26, 2022 15:25
@ghost
Copy link
Author

ghost commented May 26, 2022

I'm open to renaming the ExprDecode module (and the decode function) names. I they are the best I could think of at the time.

@ghost ghost requested a review from panarch June 2, 2022 20:17
panarch
panarch requested changes Jun 3, 2022
View reviewed changes
core/src/ast/expr.rs Outdated Show resolved Hide resolved
core/src/ast/expr.rs Outdated Show resolved Hide resolved
core/src/ast/expr.rs Outdated Show resolved Hide resolved
core/src/ast/expr.rs Outdated Show resolved Hide resolved
core/src/ast/expr.rs Outdated Show resolved Hide resolved
core/src/ast/expr.rs Outdated Show resolved Hide resolved
core/src/ast/operator.rs Outdated Show resolved Hide resolved
cli/src/print.rs Outdated Show resolved Hide resolved
core/src/ast/expr.rs Outdated Show resolved Hide resolved
panarch
panarch requested changes Jun 5, 2022
View reviewed changes
Copy link
Member

@panarch panarch left a comment

Choose a reason for hiding this comment

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

Nice!
Here... could you split the PR into two?
Two independent tasks exist in the single PR.

  1. provide sql representation to gluesql ast
  2. add description column to show indexes from table command

cli/src/print.rs Outdated Show resolved Hide resolved
core/src/ast/ast_literal.rs Outdated Show resolved Hide resolved
core/src/ast/expr.rs Outdated Show resolved Hide resolved
core/src/ast/expr.rs Outdated Show resolved Hide resolved
core/src/ast/expr.rs Outdated Show resolved Hide resolved
core/src/ast/expr.rs Outdated Show resolved Hide resolved
@ever0de ever0de added the enhancement New feature or request label Jun 5, 2022
@ghost
Copy link
Author

ghost commented Jun 5, 2022

Nice!
Here... could you split the PR into two?
Two independent tasks exist in the single PR.

  1. provide sql representation to gluesql ast
  2. add description column to show indexes from table command

OK. Will do

panarch
panarch requested changes Jun 6, 2022
View reviewed changes
Copy link
Member

@panarch panarch left a comment

Choose a reason for hiding this comment

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

Now, I've checked everything.
With these changes, we can merge this meaningful changes to the main.
Thanks really a lot!

core/src/ast/function.rs Outdated Show resolved Hide resolved
core/src/ast/expr.rs Outdated Show resolved Hide resolved
core/src/ast/expr.rs Outdated Show resolved Hide resolved
core/src/ast/expr.rs Outdated Show resolved Hide resolved
core/src/ast/ast_literal.rs Outdated Show resolved Hide resolved
@panarch panarch self-requested a review June 6, 2022 14:42
panarch
panarch approved these changes Jun 6, 2022
View reviewed changes
Copy link
Member

@panarch panarch left a comment

Choose a reason for hiding this comment

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

Beautiful.. I'd really appreciate this.
There exists lots of cases which ToSql can be used to provide much better info to db users.
e.g.
Better more detailed error message, ..

Previously, it was not possible to provide readable info about AST but now ToSql can directly hit the needs!

Thanks a lot again for this meaningful contribution.
All looks great, merging 👍 👍 👍

@panarch panarch changed the title Add description column to "show indexes from table" command Implement ToSql trait which displays AST as SQL text format Jun 6, 2022
@panarch panarch merged commit 7bf4b99 into gluesql:main Jun 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ever0de ever0de Awaiting requested review from ever0de

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@codecov-commenter @panarch @ever0de @bearney74