Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Added support of nulls first/last expression #207

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants

pftg commented Sep 6, 2013

In order that in SQL specification there is option for order how to sort nulls valuesORDER BY name DESC NULLS LAST, added to Ordering node nulls order direction option.

👍 Would love to see this pulled in

I needed this feature the other day. Surprised to see it wasn't already implemented. Any chance this will get merged?

tamird commented Sep 16, 2014

@pftg looks good! could you rebase?

pftg commented Sep 16, 2014

@tamird sure

pftg commented Sep 28, 2014

@tamird rebased

@tamird tamird commented on the diff Sep 28, 2014

lib/arel/nodes/ordering.rb
@@ -0,0 +1,27 @@
+module Arel
+ module Nodes
+ class Ordering < Unary
@tamird

tamird Sep 28, 2014

Ordering is also being defined in unary.rb - should be removed from there

@tamird tamird commented on the diff Sep 28, 2014

lib/arel/nodes/ordering.rb
@@ -0,0 +1,27 @@
+module Arel
+ module Nodes
+ class Ordering < Unary
+ REVERSE_NULLS = { first: :last, last: :first }
+
+ attr_accessor :nulls
@tamird

tamird Sep 28, 2014

do you think nulls is better represented as a boolean? there are various places where the symbol name leaks out of the implementation; seems like a boolean would be more robust.

@pftg

pftg Sep 29, 2014

we have 3 states: nulls first, nulls last and without nulls. So in my proposed dsl i want to have something like this: users[:id].asc(nulls: :first). as I got you want to have boolean flag which indicates should we have nulls support and should be nulls sorted first.

Ordering.new(nulls: false) mean nulls last?

I'd like to have previous explicit variant with symbols or with string.

@tamird tamird commented on the diff Sep 28, 2014

lib/arel/nodes/ordering.rb
@@ -0,0 +1,27 @@
+module Arel
+ module Nodes
+ class Ordering < Unary
+ REVERSE_NULLS = { first: :last, last: :first }
+
+ attr_accessor :nulls
+
+ def initialize(expr, options = {})
@tamird

tamird Sep 28, 2014

i'd prefer the option be passed in explicitly rather than in a hash

@pftg

pftg Sep 29, 2014

Ordering.new(expr, :first) - does this what you want to see?

@kbrock

kbrock Jan 6, 2016

I played around with the alternative, and it reads much better the way originally implemented ( nulls: :first) I vote to keep as is

I do find myself wanting to
order(column, nulls: :first) instead of order(column).asc(nulls: :first) - but that is minor and wasn't as quick a fix as I first thought

pftg commented Oct 15, 2014

@tamird thanks for your review, I updated code with some updates. Please check my comments and new updates

crmaxx commented Feb 18, 2015

Any chance this will get merged?

kbrock commented Jan 6, 2016

@rafaelfranca does this look good from a high level? Worth bringing back to life?

Owner

rafaelfranca commented Jan 6, 2016

Worth bringing back to life?

Yes, it is worth. Could you open a new PR rebasing the original commit? It would retain the original author's credit.

pftg commented Jan 6, 2016

👍

pftg commented Jan 6, 2016

@kbrock would you rebase, or better to complete it by me?

pftg commented Jan 6, 2016

@rafaelfranca @kbrock pls check this PR

kbrock commented Jan 6, 2016

@pftg thanks so much. oops. this was so old I assumed you'd be MIA.

feedback: jetthoughts/arel@orders_params...kbrock:orders_params

summary:

  1. remove Ordering from unary.rb
  2. add test for both null first and last
  3. add test for dsl

this is very cool.

pftg commented Jan 7, 2016

thanks @kbrock for your patch

@rafaelfranca pls review

kbrock commented Jan 7, 2016

Tangental thoughts. Based upon feedback I can put into another PR(s)

  1. I think there is now an arel case statement that you may be able to use.
  2. I always see people use ORDER BY CASE WHEN col IS NULL THEN 0 ELSE 1 END, col. But it seems to me it would be better to use ORDER BY COALESCE(col, ' '). Any insight why the first is used? I guess COALESCE vs ISNULL is not consistent.

pftg commented Jan 12, 2016

@kbrock I have not use COALESCE, but I see problem, that if there is non null value equal to " " and our NULL value replaced with " ", so they will be treated the same.

thanks for your suggestion, I updated with CASE node usage.

kbrock commented Jan 12, 2016

@pftg this is sweet. thanks. Looking forward to using this once it lands

pftg commented Jan 12, 2016

@rafaelfranca pls review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment