Support 'NULLS LAST' and 'NULLS FIRST' #210

qyihua · 2021-12-20T05:53:56Z

Hi,
In PostgreSQL, by default, null values sort as if larger than any non-null value
ref: https://www.postgresql.org/docs/current/queries-order.html
SQLite considers NULL values to be smaller than any other values for sorting purposes
ref: https://sqlite.org/lang_select.html
Mysql seems doesn't support the syntax to sort nulls last in.

#209

billy1624

Hey @qyihua, thanks for the contribution!

I'm thinking is there any better name for these order_by_xxx_nulls_last methods?

src/types.rs

billy1624 · 2021-12-29T14:20:12Z

src/backend/query_builder.rs

+        match order_expr.nulls_last {
+            Some(true) => write!(sql, " NULLS LAST").unwrap(),
+            Some(false) => write!(sql, " NULLS FISRT").unwrap(),
+            _ => (),
+        }


This should be placed inside src/backend/postgres/query.rs. So, by default all other backend won't write order by nulls when preparing the select statement.

billy1624 · 2021-12-29T14:31:36Z

src/query/ordered.rs

+    /// Order by custom string with nulls order option.
+    fn order_by_customs_nulls_last<T>(
+        &mut self,
+        cols: Vec<(T, Order)>,
+        nulls_last: bool,
+    ) -> &mut Self
+    where
+        T: ToString,


Shouldn't this nills_last be in the cols tuple?

billy1624 · 2021-12-29T14:32:09Z

src/query/ordered.rs

+    /// Order by vector of columns with nulls order option.
+    fn order_by_columns_nulls_last<T>(
+        &mut self,
+        cols: Vec<(T, Order)>,
+        nulls_last: bool,
+    ) -> &mut Self
+    where
+        T: IntoColumnRef,


qyihua · 2021-12-30T01:26:33Z

@billy1624 Thank you for the good suggestions.
I need convert the pr to draft and go on

…_with_nulls`

…h_nulls`

qyihua · 2021-12-31T09:11:54Z

Hi @billy1624 ,
I make some change, it's ready for review now. Thank you!

billy1624 · 2021-12-31T09:24:22Z

Btw... just found something interesting. We could actually implement NULLS FIRST and NULLS LAST for MySQL and MariaDB.

SELECT ... FROM ... ORDER BY reuestId IS NULL DESC, reuestId DESC

Adapted from https://stackoverflow.com/a/9307657/7059723

qyihua · 2021-12-31T09:34:58Z

Btw... just found something interesting. We could actually implement NULLS FIRST and NULLS LAST for MySQL and MariaDB.
SELECT ... FROM ... ORDER BY reuestId IS NULL DESC, reuestId DESC
Adapted from https://stackoverflow.com/a/9307657/7059723

It's great! Let me have a try

billy1624 · 2021-12-31T09:52:46Z

Thanks!!

src/backend/postgres/query.rs

qyihua · 2021-12-31T10:21:10Z

Btw... just found something interesting. We could actually implement NULLS FIRST and NULLS LAST for MySQL and MariaDB.
SELECT ... FROM ... ORDER BY reuestId IS NULL DESC, reuestId DESC
Adapted from https://stackoverflow.com/a/9307657/7059723

Nulls order support for Mysql backend is ok now :)
I test in http://sqlfiddle.com/#!9/286c76/4

billy1624 · 2021-12-31T10:27:43Z

I have added some more test cases on billy1624@e43245b
You can cherrypick it to this PR

qyihua · 2021-12-31T10:40:18Z

I have added some more test cases on billy1624@e43245b You can cherrypick it to this PR

I have cherrypick it to this PR, and all test is passed.
Thanks for your help!

tyt2y3

Sorry for the delay but there is now several conflicts. Can help resolving?

tyt2y3 · 2022-01-02T17:07:23Z

src/types.rs

@@ -148,11 +148,19 @@ pub enum JoinType {
    RightJoin,
 }

+/// Nulls order
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum Nulls {


I think we don't need to pub this ? We can keep this as pub(crate)?
If not, we need to think of a better name than Nulls.
I am thinking may be NullOrdering or something

If this keep as pub(crate), how do you think users should use these methods order_by_xxxs_with_nulls?
I think it need pass bool as nulls order argument ?

I also think NullOrdering is better than Nulls

qyihua · 2022-01-03T05:50:22Z

Sorry for the delay but there is now several conflicts. Can help resolving?

Yeah, but something is wrong in src/driver/sqlx_postgres.rs

} else if value.is_array() {
    query.bind(value.as_ref_array())

billy1624 · 2022-01-03T07:52:53Z

Let me check

billy1624 · 2022-01-05T14:31:39Z

Should be fixed

qyihua · 2022-01-05T15:56:54Z

Should be fixed

Yes! It's fixed now. Thanks :)

tyt2y3 · 2022-01-08T14:22:30Z

Thank you for your contribution!

Support 'NULLS LAST' and 'NULLS FIRST'

14d75c3

billy1624 reviewed Dec 29, 2021

View reviewed changes

qyihua marked this pull request as draft December 30, 2021 01:26

qyihua added 4 commits December 31, 2021 16:02

By defualt, don't write 'order by nulls' in QueryBuilder

3839723

Add Nulls enum instead of boolean for represent nulls order

f60d5a9

OrderedStatement: rename order_by_xxx_nulls_last to `order_by_xxx…

486bd66

…_with_nulls`

OrderedStatement: place nulls into cols tuple in `order_by_xxxs_wit…

54607c2

…h_nulls`

qyihua marked this pull request as ready for review December 31, 2021 09:03

Add nulls order support for Mysql backend

419f98c

billy1624 reviewed Dec 31, 2021

View reviewed changes

src/backend/postgres/query.rs Outdated Show resolved Hide resolved

fix: NULLS FIRST typo

24426db

test: order by with nulls on all backends

d6b494e

billy1624 requested a review from tyt2y3 December 31, 2021 10:57

tyt2y3 reviewed Jan 2, 2022

View reviewed changes

qyihua added 2 commits January 3, 2022 11:39

Merge remote-tracking branch SeaQL/master into nulls-last

f9a5131

Rename enum Nulls to NullOrdering

7f35bb1

Merge branch 'master' into nulls-last

ef49f66

Merge branch 'master' into nulls-last

86a45c1

tyt2y3 linked an issue Jan 8, 2022 that may be closed by this pull request

Support NULLS LAST and NULLS FIRST #209

Closed

tyt2y3 merged commit 9264434 into SeaQL:master Jan 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support 'NULLS LAST' and 'NULLS FIRST' #210

Support 'NULLS LAST' and 'NULLS FIRST' #210

qyihua commented Dec 20, 2021

billy1624 left a comment

billy1624 Dec 29, 2021

billy1624 Dec 29, 2021

billy1624 Dec 29, 2021

qyihua commented Dec 30, 2021

qyihua commented Dec 31, 2021

billy1624 commented Dec 31, 2021

qyihua commented Dec 31, 2021

billy1624 commented Dec 31, 2021

qyihua commented Dec 31, 2021 •

edited

Loading

billy1624 commented Dec 31, 2021

qyihua commented Dec 31, 2021 •

edited

Loading

tyt2y3 left a comment

tyt2y3 Jan 2, 2022

qyihua Jan 3, 2022

qyihua commented Jan 3, 2022

billy1624 commented Jan 3, 2022

billy1624 commented Jan 5, 2022

qyihua commented Jan 5, 2022

tyt2y3 commented Jan 8, 2022

Support 'NULLS LAST' and 'NULLS FIRST' #210

Support 'NULLS LAST' and 'NULLS FIRST' #210

Conversation

qyihua commented Dec 20, 2021

billy1624 left a comment

Choose a reason for hiding this comment

billy1624 Dec 29, 2021

Choose a reason for hiding this comment

billy1624 Dec 29, 2021

Choose a reason for hiding this comment

billy1624 Dec 29, 2021

Choose a reason for hiding this comment

qyihua commented Dec 30, 2021

qyihua commented Dec 31, 2021

billy1624 commented Dec 31, 2021

qyihua commented Dec 31, 2021

billy1624 commented Dec 31, 2021

qyihua commented Dec 31, 2021 • edited Loading

billy1624 commented Dec 31, 2021

qyihua commented Dec 31, 2021 • edited Loading

tyt2y3 left a comment

Choose a reason for hiding this comment

tyt2y3 Jan 2, 2022

Choose a reason for hiding this comment

qyihua Jan 3, 2022

Choose a reason for hiding this comment

qyihua commented Jan 3, 2022

billy1624 commented Jan 3, 2022

billy1624 commented Jan 5, 2022

qyihua commented Jan 5, 2022

tyt2y3 commented Jan 8, 2022

qyihua commented Dec 31, 2021 •

edited

Loading

qyihua commented Dec 31, 2021 •

edited

Loading