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

issues-347 Add SqlWriter trait #436

Merged
merged 6 commits into from
Sep 18, 2022
Merged

issues-347 Add SqlWriter trait #436

merged 6 commits into from
Sep 18, 2022

Conversation

ikrivosheev
Copy link
Member

@ikrivosheev ikrivosheev commented Sep 8, 2022 •
edited
Loading

PR Info

Adds

  • new trait SqlWriter
  • impl SqlWriter for String - write sql to String with inline Value
  • impl SqlWriter for SqlWriterValues - write sql to String with store Value into Vec

Breaking Changes

  • Remove struct SqlWriter
  • Remove collector from QueryBuilder and other backend traits
  • backend traits instead SqlWriter parameter get &mut dyn SqlWriter

Context: #428 (comment)

pub trait SqlWriter: Write + ToString {
   fn push_param(&mut self, value: Value, query_builder: &dyn QueryBuilder);

   fn as_writer(&mut self) -> &mut dyn Write;
}

@ikrivosheev ikrivosheev mentioned this pull request Sep 8, 2022
Closed
3 tasks
billy1624
billy1624 reviewed Sep 9, 2022
View reviewed changes
src/prepare.rs Outdated
Comment on lines 6 to 9
pub trait SqlWriter: Write {
fn push_param(&mut self, value: Value, query_builder: &dyn QueryBuilder);
fn result(self) -> String;
}
Copy link
Member

Choose a reason for hiding this comment

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

Hey @ikrivosheev, I think we don't need the result() method. The SqlWriter trait is only responsible for writing SQL and push parameters.

The to_string alike method can be place in struct SqlStringWriter and SqlWriterObj.

Copy link
Member Author

Choose a reason for hiding this comment

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

@billy1624 hello!

  1. trait SqlWriter: Writer + ToString
  2. remove SqlStringWriter, use String and impl SqlWriter for String
  3. Rename SqlWriterObj to SqlWriterValues

@ikrivosheev
Copy link
Member Author

A Small feature grew into big refactoring!

@ikrivosheev ikrivosheev force-pushed the feature/issues-347_sqlwriter_trait branch from 2a84a16 to fbd5bb5 Compare September 13, 2022 19:32
@ikrivosheev ikrivosheev marked this pull request as ready for review September 14, 2022 17:30
@ikrivosheev
Copy link
Member Author

@billy1624 @tyt2y3 can you review this PR?

@ikrivosheev ikrivosheev requested a review from tyt2y3 September 14, 2022 17:30
@ikrivosheev ikrivosheev mentioned this pull request Sep 15, 2022
Draft
3 tasks
tyt2y3
tyt2y3 reviewed Sep 17, 2022
View reviewed changes
src/prepare.rs
Comment on lines +54 to +58
impl ToString for SqlWriterValues {
fn to_string(&self) -> String {
self.string.clone()
}
}
Copy link
Member

@tyt2y3 tyt2y3 Sep 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Is this necessary? I think it'd be better if to_string returns the version with parameters injected rather than just a clone of the SQL with placeholder

Copy link
Member Author

Choose a reason for hiding this comment

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

My thoughts were: injection parameters are expensive. If someone needs an inject parameter into sql query it is better to call it explicit.
Do I need to fix that?

Copy link
Member

Choose a reason for hiding this comment

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

Umm I think we have two options 1) remove this impl 2) make it also inject parameters. I am okay with either.

Copy link
Member Author

Choose a reason for hiding this comment

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

@tyt2y3 I have problems:

  1. I cannot remove this impl, because I need it, for example: https://github.com/SeaQL/sea-query/blob/master/src/query/traits.rs#L21
  2. I cannot make inject_parameters because I need: query_builder but I didnot store it in SqlWriter...

tyt2y3
tyt2y3 reviewed Sep 17, 2022
View reviewed changes
src/index/drop.rs
@@ -64,14 +64,14 @@ impl IndexDropStatement {

impl SchemaStatementBuilder for IndexDropStatement {
fn build<T: SchemaBuilder>(&self, schema_builder: T) -> String {
let mut sql = SqlWriter::new();
let mut sql = String::with_capacity(256);
Copy link
Member

Choose a reason for hiding this comment

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

I feel like we want to extract this as a common function, or at least extract the default string size as a constant

tyt2y3
tyt2y3 approved these changes Sep 17, 2022
View reviewed changes
Copy link
Member

@tyt2y3 tyt2y3 left a comment

Choose a reason for hiding this comment

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

Thank you for the big effort. Can you also draft the Changelog for a summary of these changes?

@tyt2y3 tyt2y3 merged commit 6cf7932 into SeaQL:master Sep 18, 2022
@ikrivosheev ikrivosheev deleted the feature/issues-347_sqlwriter_trait branch September 18, 2022 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tyt2y3 tyt2y3 tyt2y3 approved these changes

@billy1624 billy1624 Awaiting requested review from billy1624

Assignees
No one assigned
Labels
None yet
Projects
SeaQL Dev Tracker
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Table Column with Default Expression
3 participants
@ikrivosheev @tyt2y3 @billy1624