Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nulls ordering skips records if there is more than one NULL #21

Closed
glebm opened this issue Mar 20, 2018 · 6 comments
Closed

Nulls ordering skips records if there is more than one NULL #21

glebm opened this issue Mar 20, 2018 · 6 comments
Labels

Comments

@glebm
Copy link
Owner

glebm commented Mar 20, 2018

Example from #20 (comment):

seek_args: [[:title, :desc, {:nulls=>:first}]]
Entry Load (1.5ms)  SELECT  `entries`.* FROM `entries` ORDER BY `entries`.`title` IS NULL DESC, `entries`.`title` DESC, `entries`.`id` ASC LIMIT 20
Entry Load (0.8ms)  SELECT  `entries`.* FROM `entries` WHERE `entries`.`id` = 1 LIMIT 1
Entry Load (1.0ms)  SELECT  `entries`.* FROM `entries` WHERE (`entries`.`title` IS NULL AND (`entries`.`title` IS NULL OR `entries`.`title` IS NULL AND `entries`.`id` > 1)) ORDER BY `entries`.`title` IS NULL ASC, `entries`.`title` ASC, `entries`.`id`     DESC LIMIT 1

seek_args: [[:title, :desc, {:nulls=>:last}]]
Entry Load (1.0ms)  SELECT  `entries`.* FROM `entries` ORDER BY `entries`.`title` DESC, `entries`.`id` ASC LIMIT 20
Entry Load (0.7ms)  SELECT  `entries`.* FROM `entries` WHERE `entries`.`id` = 1 LIMIT 1
Entry Load (0.8ms)  SELECT  `entries`.* FROM `entries` WHERE (`entries`.`title` IS NOT NULL AND (`entries`.`title` IS NOT NULL OR `entries`.`title` IS NULL AND `entries`.`id` > 1)) ORDER BY `entries`.`title` ASC, `entries`.`id` DESC LIMIT 1

/cc @twelve17

@glebm glebm added the bug label Mar 20, 2018
@twelve17
Copy link

Thanks for looking into this @glebm , and let me know if I can assist.

@glebm
Copy link
Owner Author

glebm commented Mar 20, 2018

After adding a test for multiple nulls, I can see that the query it generates without wrap_top_level_or has an incorrect OR clause:

  SELECT "posts".* FROM "posts"
   WHERE ("posts"."published_at" IS NULL OR -- <- *this is wrong*
          "posts"."published_at" IS NULL AND "posts"."id" < 47)
ORDER BY "posts"."published_at" IS NULL ASC,
         "posts"."published_at" ASC,
         "posts"."id" DESC LIMIT 1

@glebm glebm changed the title Top-level query optimization wrapping breaks top-level NULL conditions Nulls ordering skips records if there is more than one NULL Mar 20, 2018
glebm added a commit that referenced this issue Mar 20, 2018
glebm added a commit that referenced this issue Mar 20, 2018
glebm added a commit that referenced this issue Mar 20, 2018
glebm added a commit that referenced this issue Mar 20, 2018
@glebm
Copy link
Owner Author

glebm commented Mar 20, 2018

@twelve17 Got a fix here: #22

If it works for you, I'll release a new version.

You can try it out with:

gem 'order_query', git: 'https://github.com/glebm/order_query', branch: 'null-wtlo'

There should be no need to set wrap_top_level_or = false in that branch (it wasn't a sufficient workaround anyway).

glebm added a commit that referenced this issue Mar 20, 2018
@twelve17
Copy link

twelve17 commented Mar 21, 2018

Hi @glebm , things seem to work with the updated code. 🎉

I did not use wrap_top_level_or = false for this test (I omitted the setting altogether).

For posterity, here its the resulting query:

seek_args: [[:title, :desc, {:nulls=|:first}]]
Entry Load (1.0ms)  SELECT  `entries`.* FROM `entries` ORDER BY `entries`.`title` IS NULL DESC, `entries`.`title` DESC, `entries`.`id` ASC LIMIT 20
Entry Load (0.7ms)  SELECT  `entries`.* FROM `entries` WHERE `entries`.`id` = 1 LIMIT 1
Entry Load (0.7ms)  SELECT  `entries`.* FROM `entries` WHERE (`entries`.`title` IS NULL AND `entries`.`id` | 1) ORDER BY `entries`.`title` IS NULL ASC, `entries`.`title` ASC, `entries`.`id` DESC LIMIT 1

Thanks for your help with this!

@twelve17
Copy link

twelve17 commented Mar 21, 2018

BTW, please feel free to close the other issue, it's obviously less pressing given this is working. Thanks again.

@glebm glebm closed this as completed in #22 Mar 21, 2018
glebm added a commit that referenced this issue Mar 21, 2018
glebm added a commit that referenced this issue Mar 21, 2018
@glebm
Copy link
Owner Author

glebm commented Mar 21, 2018

Fix released in v0.4.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants