added PostgreSQL Interval value variant by yasamoka · Pull Request #709 · SeaQL/sea-query

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

 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

 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

 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

                    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

                    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

+                }
+
+                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

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


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

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

        #[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

+        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

                }
+                #[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 transact-rs/sqlx#2793

Merged

implemented TryGetable for PgInterval SeaQL/sea-orm#1890

Open

yasamoka commented Oct 2, 2023

View reviewed changes

replaced redundant cfg attribute

1c594ea

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

Uh oh!

Conversation

yasamoka commented Oct 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyt2y3 commented Oct 2, 2023

Uh oh!

tyt2y3 left a comment

Choose a reason for hiding this comment

Uh oh!

tyt2y3 commented Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yasamoka commented Oct 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yasamoka Oct 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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