Fix merging of WITH expressions when the expressions are identical
#3
Conversation
There was a problem hiding this comment.
Hi @dark-panda thanks for the changes. Looks like a step in the right direction 👍
I've left few comments. Let me know what you think.
test/activerecord/cte_test.rb
Outdated
| merged = most_popular1.merge(most_popular2) | ||
|
|
||
| assert_equal merged.size, 4 | ||
| assert_equal merged[0].views_count, 456 |
There was a problem hiding this comment.
This fails for Postgres adapter. Try running rake test:matrix (I need to setup CI or Github Actions) and you will see the error. Since Postgres does not guarantee ordering by default first item is not the one with 456 views.
Maybe you can define popular_posts = Post.where("views_count >= 100") and merged = most_popular1.merge(most_popular2).from("most_popular as posts") and then test that they return equal results assert_equal most_popular.to_a, merged.to_a.
How does that sound to you?
There was a problem hiding this comment.
I also noticed that arguments for assert_equal are wrong which produce wrong error message :)
It should be assert_equal expected, actual. Missed that on last PR for test cases above.
There was a problem hiding this comment.
Yeah I noticed that but I didn’t want to change that in this PR. I figured I’d mention that later on and it can be fixed across the board.
There was a problem hiding this comment.
I’ll rejigger the tests to make them more consistent, I hadn’t noticed the test matrix and apparently only ran the tests on SQLite.
|
|
||
| def test_with_when_merging_relations_with_identical_with_names_and_identical_quries | ||
| most_popular1 = Post.with(most_popular: Post.where("views_count >= 100")) | ||
| most_popular2 = Post.with(most_popular: Post.where("views_count >= 100")) |
There was a problem hiding this comment.
Have you tried what happens if different with values are provided that result with the same sql in the end?
Something like:
most_popular1 = Post.with(most_popular: Post.where("views_count = 100"))
most_popular2 = Post.with(most_popular: Post.where(views_count: 100))
or
most_popular1 = Post.with(most_popular: Post.where("views_count > 100"))
most_popular2 = Post.with(most_popular: Arel::Table.new(:posts)..project(Arel.star).where(posts_table[:views_count].gt(100))
There was a problem hiding this comment.
These still fail since the equality check isn't doing any sort of conversion. This would require some more extensive rejiggering I think.
There was a problem hiding this comment.
@vlado @joelvh in the case of Rails 6.1 and using strings as #with values, this broke in Rails 6.1 due to this commit:
There's no detection for strings in there. Before they just slid on through, but now they need to be accounted for.
I have a patch that I'll submit upstream, just wrapping that up now. As for what to do for the gem, a monkey patch would probably be the only recourse, as the code in question is in the guts of the SQL generation visitor methods.
There was a problem hiding this comment.
Rails PR is up at rails/rails#42563, with an issue report at rails/rails#42561.
There was a problem hiding this comment.
These still fail since the equality check isn't doing any sort of conversion. This would require some more extensive rejiggering I think.
I suggest we skip that then. Let's merge this one and open new PR when/if needed.
There was a problem hiding this comment.
I have a patch that I'll submit upstream, just wrapping that up now. As for what to do for the gem, a monkey patch would probably be the only recourse, as the code in question is in the guts of the SQL generation visitor methods.
Great find and great that you submitted PR to the Rails 👍 Let's see when it gets merged and released.
I would even be ok if we just add a note to the Readme that passing string for some Rails versions does not work due to the mentioned bug.
There was a problem hiding this comment.
Yeah, that failed in my tests w/ 6.1 and i ignored it because I'm not using strings 😬
Can we use the SQL literal suggested in your upstream PR internally here?
dark-panda
force-pushed
the
fix-duplicate-labels
branch
from
d4e1ee9
to
e253c96
Compare
|
|
||
| def test_with_when_merging_relations_with_identical_with_names_and_identical_quries | ||
| most_popular1 = Post.with(most_popular: Post.where("views_count >= 100")) | ||
| most_popular2 = Post.with(most_popular: Post.where("views_count >= 100")) |
There was a problem hiding this comment.
These still fail since the equality check isn't doing any sort of conversion. This would require some more extensive rejiggering I think.
I suggest we skip that then. Let's merge this one and open new PR when/if needed.
|
|
||
| def test_with_when_merging_relations_with_identical_with_names_and_identical_quries | ||
| most_popular1 = Post.with(most_popular: Post.where("views_count >= 100")) | ||
| most_popular2 = Post.with(most_popular: Post.where("views_count >= 100")) |
There was a problem hiding this comment.
I have a patch that I'll submit upstream, just wrapping that up now. As for what to do for the gem, a monkey patch would probably be the only recourse, as the code in question is in the guts of the SQL generation visitor methods.
Great find and great that you submitted PR to the Rails 👍 Let's see when it gets merged and released.
I would even be ok if we just add a note to the Readme that passing string for some Rails versions does not work due to the mentioned bug.
test/activerecord/cte_test.rb
Outdated
| merged = most_popular1.merge(most_popular2).from("most_popular as posts") | ||
|
|
||
| assert_equal posts(:two, :three, :four), merged.to_a |
There was a problem hiding this comment.
This still does not guarantee the order and will fail for some adapters I suppose. Maybe you should add ordering merged = most_popular1.merge(most_popular2).from("most_popular as posts").order(:id) or try something like:
| merged = most_popular1.merge(most_popular2).from("most_popular as posts") | |
| assert_equal posts(:two, :three, :four), merged.to_a | |
| expected_posts = Post.where("views_count >= 100") | |
| merged = most_popular1.merge(most_popular2).from("most_popular as posts") | |
| assert expected_posts.any? | |
| assert_equal expected_posts.to_a, merged.to_a |
There was a problem hiding this comment.
Fixed by doing assert_equal foo.sort, bar.sort, basically.
| @@ -6,10 +6,6 @@ module Querying | |||
| end | |||
|
|
|||
| module WithMerger | |||
| def normal_values | |||
There was a problem hiding this comment.
@dark-panda when I looked at the 6.0.x internals, this made it so Rails automatically did some things. What steps that Rails was doing are not not being performed as a result of not including :with here? Do we need to manually perform those steps?
I believe normal_values was removed in 6.1.x, so we need to consider how this affects both versions.
There was a problem hiding this comment.
#normal_values is still in as of the current 6-1-stable:
https://github.com/rails/rails/blob/6-1-stable/activerecord/lib/active_record/relation/merger.rb#L58
dark-panda
force-pushed
the
fix-duplicate-labels
branch
from
e253c96
to
806d39a
Compare
|
Released in version 0.1.4 |
No description provided.