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

Avoid duplicate clauses when using #or #29950

Merged
merged 2 commits into from Jul 31, 2017

Conversation

MaxLap
Copy link
Contributor

@MaxLap MaxLap commented Jul 26, 2017

Attempt number 2 for this change. A refactor of WhereClause by @sgrif made this change a lot simpler. Initial proposal at #29805.

Right now, when using #or, nothing is done to try to avoid duplicated conditions. As a result, query length increases at an exponential rate. This makes it really hard to parse for humans, making debugging / understanding the query log really hard.

To show the scale of the problem:

class Project
    scope :big, -> { where(big1: true).or(where(big2: true)) }
    scope :important, -> { where(important1: true).or(where(important2: true)) }
    scope :ongoing, -> { where(ongoing1: true).or(where(ongoing2: true)) }
end

Project.big # 2 conditions
#=> SELECT...WHERE ("projects"."big1" = ? OR "projects"."big2" = ?) 
Project.big.important # 6 conditions
#=> SELECT...WHERE (("projects"."big1" = ? OR "projects"."big2" = ?) AND "projects"."important1" = ? OR ("projects"."big1" = ? OR "projects"."big2" = ?) AND "projects"."important2" = ?)
Project.big.important.ongoing # 14 conditions
#=> To long to show
# Then 30 conditions
# Then 62
# Then 126 conditions, for using #or 6 times. This should be 12!

# After this fix, it is correctly 2, 4, 6, 8, 10, ... conditions.

@rails-bot
Copy link

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @pixeltrix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

Condenses the clauses that are common to both sides of the OR and put them outside, before the OR
This fix the current behavior where the number of conditions is exponential based on the number of times #or is used.
@MaxLap MaxLap force-pushed the avoid_or_clause_duplicates branch from 0b4c3e3 to 110e0e1 Compare July 26, 2017 02:18
actual = (actual + wcs[1]).or(actual + wcs[2])
expected = expected + wcs[1].or(wcs[2])

actual = (actual + wcs[3] + wcs[4]).or(actual + wcs[5] + wcs[6])
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this addtional manipulation really necessary for the test?

expected = wcs[0]

actual = (actual + wcs[1]).or(actual + wcs[2])
expected = expected + wcs[1].or(wcs[2])
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The redefinition makes this harder to understand. Why not instead:

common = wcs[0]

actual = (common + wcs[1]).or(common + wcs[2])
expected = common + wcs[1].or(wcs[2])

common = self - left
right = other - common

return common if left.empty? || right.empty?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to avoid postfix conditionals (and early returns for that matter) if possible. How about this instead?

if left.empty? || right.empty?
  common
else
  or_clause = WhereClause.new(
    [left.ast.or(right.ast)],
  )
  common + or_clause
end

@@ -181,6 +181,39 @@ class WhereClauseTest < ActiveRecord::TestCase
assert_equal WhereClause.empty, WhereClause.empty.or(where_clause)
end

test "or places common conditions before the OR" do
wcs = (0..6).map do |i|
WhereClause.new([table["col_#{i}"].eq(bind_param(i))])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sort of thing makes this test really difficult to read. How about something like this?

a = WhereClause.new(
  [table["id"].eq(bind_param(1)), table["name"].eq(bind_param("Sean"))],
)
b = WhereClause.new(
  [table["id"].eq(bind_param(1)) ,table["hair_color"].eq(bind_param("black"))],
)
common = WhereClause.new(
  [table["id"].eq(bind_param(1))],
)
or_clause = WhereClause.new(
  [table["name"].eq(bind_param("Sean")).or(table["hair_color"].eq(bind_param("black")))],
)

assert_equal common + or_clause, a.or(b)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tried, that doesn't work because WhereClause#or isn't call for both sides, and WhereClause#or always adds a And node around the predicates, even if there only one.

I'll tweak the test.

extra = WhereClause.new([table["extra"].eq(bind_param("pluto"))])

actual_extra_only_on_left = (common + extra).or(common)
actual_extra_only_on_right = (common).or(common + extra)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also find this really hard to read. Can we either just test this once, or just write out the actual conditions that we're testing?

@MaxLap
Copy link
Contributor Author

MaxLap commented Jul 28, 2017

Personally, I thought that using variables and then doing + to concatenate conditions together helped with readability. But i'll change it to be more basic and will split the tests into smaller ones.

@MaxLap
Copy link
Contributor Author

MaxLap commented Jul 31, 2017

I did the changes and added another test. Thnx for the reviews

@sgrif sgrif merged commit 0f24588 into rails:master Jul 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants