feat(pg): support ordering with NULLS FIRST / NULLS LAST #332

mattbretl · 2018-11-02T13:43:17Z

No description provided.

mattbretl · 2018-11-02T13:49:50Z

This allows plugins to solve #331

benjie

You're on fire, @mattbretl! This looks good to me; we need a test for it before I can merge it though.

benjie · 2018-11-02T15:41:35Z

packages/graphile-build-pg/src/QueryBuilder.js

+                      ? sql.fragment` NULLS FIRST`
+                      : nullsFirst === false
+                        ? sql.fragment` NULLS LAST`
+                        : null


I had to actually read the source to make sure null was okay here! It is ✅

Yeah, I find that really handy.. don't change it! 😄

mattbretl · 2018-11-02T18:05:03Z

Given how the tests are structured, I can add a new integration test in postgraphile-core/__tests__/integration/schema, appending a small plugin to expose additional orderBy enums that use NULLS LAST. Is that what you had in mind?

mattbretl · 2018-11-02T18:17:40Z

Or.. I could add an orderByNullsLast option to PgConnectionArgOrderBy.js, and use that (if provided) when calling queryBuilder.orderBy(). Then the test would be as simple as passing orderByNullsLast: true through graphileBuildOptions. That's probably all that most people (including myself) will want to accomplish with this.. overriding the PG default of NULLS FIRST for DESC.

benjie · 2018-11-02T19:24:36Z

I think the latter makes sense and requires minimal effort. I don't really care so long as it is tested and can be show to work both ways. Basically what you're saying is "treat nulls as if they are smaller than everything" - i.e. they're first when ascending and last when descending?

Can you flag that this behaviour will become the default in v5? I always want it so that when you reverse the order (ASC -> DESC) the rows return in the exact opposite order (which is not the case currently, if there are nulls).

Thanks Matt!

benjie · 2018-11-02T19:25:03Z

I wonder if there's a performance impact of adding the NULLS FIRST/LAST clause...

mattbretl · 2018-11-03T12:30:01Z

Basically what you're saying is "treat nulls as if they are smaller than everything" - i.e. they're first when ascending and last when descending?

Actually, no. This is about using NULLS LAST for both ASC and DESC. As a simple example, consider a tasks table with a nullable integer priority field. (Tasks can optionally be assigned a priority.) On the UI side, if you're sorting by high or low priority, you probably want to show prioritized tasks before unprioritized tasks, and that requires specifying NULLS LAST for DESC.

If 5 is high-priority and 1 is low-priority:

High-priority tasks: ORDER BY priority DESC NULLS LAST (without NULLS LAST, you get all of the unprioritized tasks showing up first.. definitely not what you want)
Low-priority tasks: ORDER BY priority ASC

If 1 is high-priority and 5 is low-priority:

High-priority tasks: ORDER BY priority ASC
Low-priority tasks: ORDER BY priority DESC NULLS LAST (without NULLS LAST, you get all of the unprioritized tasks showing up first.. probably not what you want)

Can you flag that this behaviour will become the default in v5? I always want it so that when you reverse the order (ASC -> DESC) the rows return in the exact opposite order (which is not the case currently, if there are nulls).

It works that way currently. Records are returned in the exact opposite order, with nulls treated as greater than all non-nulls. So there should be no need to change the default in v5.

benjie · 2018-11-03T13:37:47Z

Hmmm. Add --order-nulls-last flag then I guess.

mattbretl · 2018-11-03T14:25:13Z

I wasn't sure if you wanted to add yet-another global config option, so I left it at the graphileBuildOptions level for now. Let me know what you think.

mattbretl · 2018-11-13T11:25:56Z

I wonder if there's a performance impact of adding the NULLS FIRST/LAST clause...

From the PG docs..

By default, B-tree indexes store their entries in ascending order with nulls last. This means that a forward scan of an index on column x produces output satisfying ORDER BY x (or more verbosely, ORDER BY x ASC NULLS LAST). The index can also be scanned backward, producing output satisfying ORDER BY x DESC (or more verbosely, ORDER BY x DESC NULLS FIRST, since NULLS FIRST is the default for ORDER BY DESC).

So unless you explicitly create a NULLS LAST index, using DESC NULLS LAST could have a performance impact.

benjie · 2018-11-21T14:01:13Z

Ohhhh. Thanks for sharing. Reviewing now 👍

benjie

I'm going to merge this. A small follow-up PR that adds | null to the relevant places would be welcome but TBH I'm happy to cross this bridge when we get to it.

benjie · 2018-11-21T15:17:20Z

packages/graphile-build-pg/src/QueryBuilder.d.ts

@@ -25,7 +25,7 @@ export default class QueryBuilder {
  public where(exprGen: SQLGen): void;
  public whereBound(exprGen: SQLGen, isLower: boolean): void;
  public setOrderIsUnique(): void;
-  public orderBy(exprGen: SQLGen, ascending: boolean): void;
+  public orderBy(exprGen: SQLGen, ascending: boolean, nullsFirst: boolean): void;


Technically this should be nullsFirst?: boolean because it doesn't have to be specified

You know what... Lets have it required.

benjie · 2018-11-21T15:18:11Z

packages/graphile-build-pg/src/QueryBuilder.js

@@ -236,9 +236,9 @@ class QueryBuilder {
  setOrderIsUnique() {
    this.data.orderIsUnique = true;
  }
-  orderBy(exprGen: SQLGen, ascending: boolean = true) {
+  orderBy(exprGen: SQLGen, ascending: boolean = true, nullsFirst: boolean) {


Technically this should also be nullsFirst?: boolean because it doesn't have to be specified. Huh, TypeScript and Flow use the same syntax for this 👍

Actually it should be nullsFirst: boolean | null = null I think.

You know what... Lets have it required. It should still be | null technically...

Add optional nullsFirst arg to QueryBuilder orderBy

d03d106

Support null ordering option in connection orderBy arg

875596c

benjie requested changes Nov 2, 2018

View reviewed changes

mattbretl added 2 commits November 3, 2018 14:21

Add orderByNullsLast option

c8befd6

Test

c1957a9

Merge branch 'master' into order-by-nulls-first-or-last

38a40cb

benjie approved these changes Nov 21, 2018

View reviewed changes

benjie merged commit 545d082 into graphile:master Nov 21, 2018

mattbretl deleted the order-by-nulls-first-or-last branch November 21, 2018 18:36

benjie mentioned this pull request Nov 21, 2018

Feature request: add option to sort NULLs last #331

Closed

mattbretl mentioned this pull request Nov 22, 2018

chore: cleanup function deprecation and nullsFirst features #350

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(pg): support ordering with NULLS FIRST / NULLS LAST #332

feat(pg): support ordering with NULLS FIRST / NULLS LAST #332

mattbretl commented Nov 2, 2018

mattbretl commented Nov 2, 2018

benjie left a comment

benjie Nov 2, 2018

mattbretl Nov 2, 2018

mattbretl commented Nov 2, 2018

mattbretl commented Nov 2, 2018

benjie commented Nov 2, 2018

benjie commented Nov 2, 2018

mattbretl commented Nov 3, 2018

benjie commented Nov 3, 2018

mattbretl commented Nov 3, 2018

mattbretl commented Nov 13, 2018

benjie commented Nov 21, 2018

benjie left a comment

benjie Nov 21, 2018

benjie Nov 21, 2018

benjie Nov 21, 2018

benjie Nov 21, 2018

benjie Nov 21, 2018

feat(pg): support ordering with NULLS FIRST / NULLS LAST #332

feat(pg): support ordering with NULLS FIRST / NULLS LAST #332

Conversation

mattbretl commented Nov 2, 2018

mattbretl commented Nov 2, 2018

benjie left a comment

Choose a reason for hiding this comment

benjie Nov 2, 2018

Choose a reason for hiding this comment

mattbretl Nov 2, 2018

Choose a reason for hiding this comment

mattbretl commented Nov 2, 2018

mattbretl commented Nov 2, 2018

benjie commented Nov 2, 2018

benjie commented Nov 2, 2018

mattbretl commented Nov 3, 2018

benjie commented Nov 3, 2018

mattbretl commented Nov 3, 2018

mattbretl commented Nov 13, 2018

benjie commented Nov 21, 2018

benjie left a comment

Choose a reason for hiding this comment

benjie Nov 21, 2018

Choose a reason for hiding this comment

benjie Nov 21, 2018

Choose a reason for hiding this comment

benjie Nov 21, 2018

Choose a reason for hiding this comment

benjie Nov 21, 2018

Choose a reason for hiding this comment

benjie Nov 21, 2018

Choose a reason for hiding this comment