Arel: make Or nodes "Nary" like And

[casperisfine]

Fix: #51386

This significantly reduce the depth of the tree for large OR conditions. I was initially a bit on the fence about that fix, but given that And is already implemented this way, I see no reasons not to do the same.

Amusingly, the reported repro script now makes SQLite fail:

SQLite3::SQLException: Expression tree is too large (maximum depth 1000)

FYI: @markedmondson

[matthewd]

Agree we shouldn't backport, given the (albeit internal) API change -- but yeah this seems like a nice improvement even just for the general consistency with And

[nertzy]

I noticed (via a "canary" build test failure in pg_search's test suite) that this introduces a breaking change to the constructor of Arel::Nodes::Or.

It now requires an Array/Enumerable argument instead of two separate arguments.

A cursory search of GitHub shows a few different codebases that rely on this interface.

Would there be any interest in reintroducing (and probably deprecating?) the previous constructor interface?

Either way, I can release an update to pg_search that detects the arity of the method and switches between the two implementations.

I understand that the Arel constructors are considered an "internal" API, so I will accept whatever you think is best here.

[byroot]

I understand that the Arel constructors are considered an "internal" API

Yeah, it would be weird to emit a deprecation warning for something that's a private API.

I think it would be best if the caller was feature testing to handle compatibility:

if Arel::Nodes::Or.instance_method(:initialize).arity == 1 # Active Record 7.2+
  def conditions
    config.features
      .reject { |_feature_name, feature_options| feature_options && feature_options[:sort_only] }
      .map { |feature_name, _feature_options| feature_for(feature_name).conditions }
      .inject { |accumulator, expression| Arel::Nodes::Or.new([accumulator, expression]) }
  end
else
  def conditions
    config.features
      .reject { |_feature_name, feature_options| feature_options && feature_options[:sort_only] }
      .map { |feature_name, _feature_options| feature_for(feature_name).conditions }
      .inject { |accumulator, expression| Arel::Nodes::Or.new(accumulator, expression) }
  end
end

I totally understand this will cause issues with multiple gems, and even some private codebase, but I'm a bit afraid of creating a precendent by deprecating private APIs. @rafaelfranca what do you think?