added PostgreSQL Interval value variant #709

yasamoka · 2023-10-02T04:33:24Z

First of all, thank you for this feature-filled query builder.

This PR adds the Value::Interval enum variant containing a PgInterval provided by SQLx. It requires this change in SQLx and is required for these changes in SeaORM.

If this PR is considered relevant to the project, then I need some help with how to improve it in some aspects. I will mention the various points in further review requests.

yasamoka · 2023-10-02T04:37:09Z

Cargo.toml

@@ -44,6 44,7 @@ time = { version = "0.3", default-features = false, optional = true, features =
 ipnetwork = { version = "0.20", default-features = false, optional = true }
 mac_address = { version = "1.1", default-features = false, optional = true }
 ordered-float = { version = "3.4", default-features = false, optional = true }
+sqlx = { git = "https://github.com/yasamoka/sqlx", branch = "pg-interval-hash", default-features = false, optional = true }


There was previously no dependency on SQLx. However, PgInterval is provided by SQLx. Is this fine?

yasamoka · 2023-10-02T04:38:15Z

Cargo.toml

@@ -60,7 61,7 @@ derive = ["sea-query-derive"]
 attr = ["sea-query-attr"]
 hashable-value = ["derivative", "ordered-float"]
 postgres-array = []
-postgres-interval = ["proc-macro2", "quote"]
+postgres-interval = ["proc-macro2", "quote", "sqlx/postgres"]


Is this the preferred way of adding this feature dependency?

yasamoka · 2023-10-02T04:38:45Z

sea-query-binder/Cargo.toml

@@ -42,6 42,7 @@ with-time = ["sqlx?/time", "sea-query/with-time", "time"]
 with-ipnetwork = ["sqlx?/ipnetwork", "sea-query/with-ipnetwork", "ipnetwork"]
 with-mac_address = ["sqlx?/mac_address", "sea-query/with-mac_address", "mac_address"]
 postgres-array = ["sea-query/postgres-array"]
+postgres-interval = ["sqlx-postgres", "sqlx/chrono"]


Is this the preferred way of adding this feature dependency?

yasamoka · 2023-10-02T04:44:40Z

sea-query-binder/src/sqlx_mysql.rs

@@ -73,6 73,8 @@ impl sqlx::IntoArguments<'_, sqlx::mysql::MySql> for SqlxValues {
 Value::ChronoDateTimeWithTimeZone(t) => {
 args.add(Value::ChronoDateTimeWithTimeZone(t).chrono_as_naive_utc_in_string());
 }
+ #[cfg(feature = "postgres-interval")]
+ Value::Interval(_) => {}


Is there a better of doing this other than splitting the query builder or throwing an error?

yasamoka · 2023-10-02T04:44:56Z

sea-query-binder/src/sqlx_sqlite.rs

@@ -73,6 73,8 @@ impl<'q> sqlx::IntoArguments<'q, sqlx::sqlite::Sqlite> for SqlxValues {
 Value::ChronoDateTimeWithTimeZone(t) => {
 args.add(t.map(|t| *t));
 }
+ #[cfg(feature = "postgres-interval")]
+ Value::Interval(_) => {}


Is there a better of doing this other than splitting the query builder or throwing an error?

yasamoka · 2023-10-02T04:46:36Z

src/backend/query_builder.rs

+ }
+
+ write!(s, "'::interval").unwrap();
+ }


Shall I make the units abbreviated or is this better for clarity when inspecting the built SQL statement?

yasamoka · 2023-10-02T04:47:04Z

src/value.rs

+ }
+
+ fn column_type() -> ColumnType {
+ ColumnType::Interval(None, None)


What do I pass to ColumnType::Interval, if anything?

yasamoka · 2023-10-02T04:47:35Z

src/value.rs

@@ -1423,6 1477,8 @@ pub fn sea_value_to_json_value(value: &Value) -> Json {
 Value::ChronoDateTimeUtc(_) => CommonSqlQueryBuilder.value_to_string(value).into(),
 #[cfg(feature = "with-chrono")]
 Value::ChronoDateTimeLocal(_) => CommonSqlQueryBuilder.value_to_string(value).into(),
+ #[cfg(feature = "postgres-interval")]
+ Value::Interval(_) => CommonSqlQueryBuilder.value_to_string(value).into(),


Is this correct?

tyt2y3 · 2023-10-02T10:59:56Z

Thank you. I will put this on my pipeline.

tyt2y3

Hi, thank you for your contribution. However, I don't think we should pull in sqlx as a dependency in the root crate.
What we can do is:

define our own PgInterval inside SeaQuery
add a variant to Value (which is now done)
support it in sea-query-binder

that way, in the future we can also support other driver libraries

tyt2y3 · 2023-10-04T18:03:35Z

Also, we already have the https://docs.rs/sea-query/latest/sea_query/table/enum.PgInterval.html column type.
Edit: I mis-clicked the close button.

yasamoka · 2023-10-10T11:11:26Z

Hi, thank you for your contribution. However, I don't think we should pull in sqlx as a dependency in the root crate. What we can do is:
1. define our own `PgInterval` inside SeaQuery

2. add a variant to `Value` (which is now done)

3. support it in `sea-query-binder`
that way, in the future we can also support other driver libraries

Done :)

I named it PgIntervalValue so that it does not conflict with PgInterval field type. Is that alright? And is it placed in the proper location?

yasamoka · 2023-10-10T11:16:13Z

src/value.rs

+ for (interval, formatted) in VALUES {
+ let query = Query::select().expr(interval).to_owned();
+ assert_eq!(
+ query.to_string(PostgresQueryBuilder),


What shall we do about MysqlQueryBuolder and SqliteQueryBuilder?

yasamoka · 2023-10-10T20:14:09Z

sea-query-binder/src/sqlx_postgres.rs

@@ -105,6 107,10 @@ impl sqlx::IntoArguments<'_, sqlx::postgres::Postgres> for SqlxValues {
 Value::TimeDateTimeWithTimeZone(t) => {
 args.add(t.as_deref());
 }
+ #[cfg(feature = "postgres-interval")]
+ Value::Interval(t) => {
+ args.add(t.as_deref());


How would I implement sqlx::Encode<'_, Postgres> for PgIntervalValue without adding sqlx as a dependency for sea-query?

added PostgreSQL Interval value variant

a403531

This was referenced Oct 2, 2023

derived Hash trait for PgInterval launchbadge/sqlx#2793

Merged

implemented TryGetable for PgInterval SeaQL/sea-orm#1890

Open

yasamoka commented Oct 2, 2023

View reviewed changes

replaced redundant cfg attribute

1c863ea

yasamoka commented Oct 2, 2023

View reviewed changes

tyt2y3 requested changes Oct 4, 2023

View reviewed changes

tyt2y3 closed this Oct 4, 2023

tyt2y3 reopened this Oct 4, 2023

moved PgInterval internally; reorganized PgInterval-related additions

e9898ec

added tests for PgInterval

c9c53dc

yasamoka force-pushed the pg-interval-value branch from 7027767 to c9c53dc Compare October 10, 2023 11:13

yasamoka commented Oct 10, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added PostgreSQL Interval value variant #709

added PostgreSQL Interval value variant #709

yasamoka commented Oct 2, 2023 •

edited

Loading

yasamoka Oct 2, 2023

yasamoka Oct 2, 2023

yasamoka Oct 2, 2023

yasamoka Oct 2, 2023

yasamoka Oct 2, 2023

yasamoka Oct 2, 2023

yasamoka Oct 2, 2023

yasamoka Oct 2, 2023

tyt2y3 commented Oct 2, 2023

tyt2y3 left a comment

tyt2y3 commented Oct 4, 2023 •

edited

Loading

yasamoka commented Oct 10, 2023 •

edited

Loading

yasamoka Oct 10, 2023

yasamoka Oct 10, 2023 •

edited

Loading

added PostgreSQL Interval value variant #709

Are you sure you want to change the base?

added PostgreSQL Interval value variant #709

Conversation

yasamoka commented Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyt2y3 commented Oct 2, 2023

tyt2y3 left a comment

Choose a reason for hiding this comment

tyt2y3 commented Oct 4, 2023 • edited Loading

yasamoka commented Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

yasamoka Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

yasamoka commented Oct 2, 2023 •

edited

Loading

tyt2y3 commented Oct 4, 2023 •

edited

Loading

yasamoka commented Oct 10, 2023 •

edited

Loading

yasamoka Oct 10, 2023 •

edited

Loading