Make preload query to prepared statements

[kamipo]

Currently preload query cannot be prepared statements even if
prepared_statements: true. This commit fixes the issue.

[rails-bot]

r? @eileencodes

(@rails-bot has picked a reviewer for you, use r? to override)

[sgrif]

Instead of handling it there, can we fix the predicate builder to use prepared statements any time it's passed an array of one item?

[kamipo]

It is possible, but more complicated. To decouple building Arel ASTs from uniqueness validator, the predicate builder needs to support array and range types. #26072
It seems the handling of arrays in the predicate builder becomes complicated.

[kamipo]

I tried to fix the predicate builder to handle an array of one item. Now the predicate builder uses prepared statements any time when passed an array of one item. Thanks!

[rafaelfranca]

r? @sgrif

[sgrif]

Can you rebase?

[kamipo]

Rebased!

[matthewd]

Does this conflict with my desire to use prepared statements for all array predicates, by binding the array as a single value, on array-supporting RDBMSes? 😕

[kamipo]

If RDBMSes supports an array type and the column is an array type, it is bound as an array value.

  PgArray.find_by(tags: ["black"])
  # => SELECT  "pg_arrays".* FROM "pg_arrays" WHERE "pg_arrays"."tags" = $1 LIMIT $2  [["tags", "{black}"], ["LIMIT", 1]]

[matthewd]

@kamipo I mean where we currently do:

User.where(id: [1, 2])
# => SELECT ... WHERE id IN (1, 2)

I'd like to instead produce:

User.where(id: [1, 2])
# => SELECT ... WHERE id = ANY($1)   ["{1,2}"]

.. as this allows us to prepare a statement once, and not need a new one for different array lengths.

It's currently just a future thought, but I wondered if introducing a condition here might make it harder to do the above (which obviously must be adapter specific) later.

[kamipo]

I think it is easy to support the above later by the condition.

--- a/activerecord/lib/active_record/relation/predicate_builder.rb
+++ b/activerecord/lib/active_record/relation/predicate_builder.rb
@@ -96,7 +96,7 @@ def create_binds_for_hash(attributes)
           when value.is_a?(Relation)
             binds += value.bound_attributes
           when value.is_a?(Array) && !table.type(column_name).respond_to?(:subtype)
-            if value.size == 1 && can_be_bound?(column_name, value.first)
+            if !supports_array_types? && value.size == 1 && can_be_bound?(column_name, value.first)
               result[column_name] = Arel::Nodes::BindParam.new
               binds << build_bind_param(column_name, value.first)
             end

[sgrif]

We should not be relying on the internals of type objects here.

[kamipo]

If removed the type condition, test_where_by_attribute_with_array fails.
https://github.com/rails/rails/blob/master/activerecord/test/cases/adapters/postgresql/array_test.rb#L293-L297

  def test_where_by_attribute_with_array
    tags = ["black", "blue"]
    record = PgArray.create!(tags: tags)
    assert_equal record, PgArray.where(tags: tags).take
  end

# correct
SELECT  "pg_arrays".* FROM "pg_arrays" WHERE "pg_arrays"."tags" = $1 LIMIT $2  [["tags", "{black,blue}"], ["LIMIT", 1]]

# removed type condition
SELECT  "pg_arrays".* FROM "pg_arrays" WHERE "pg_arrays"."tags" IN ('black', 'blue') LIMIT $1  [["LIMIT", 1]]

[sgrif]

Wait when was that added? That isn't supposed to work in the first place.

…

On Thu, Dec 8, 2016 at 5:58 PM Ryuta Kamizono ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In activerecord/lib/active_record/relation/predicate_builder.rb <#26098>: > @@ -95,6 +95,11 @@ def create_binds_for_hash( attributes) next when value.is_a?(Relation) binds += value.bound_attributes + when value.is_a?(Array) && !table.type(column_name).respond_to?(:subtype) If removed the type condition, test_where_by_attribute_with_array fails. https://github.com/rails/rails/blob/master/activerecord/test/cases/adapters/postgresql/array_test.rb#L293-L297 def test_where_by_attribute_with_array tags = ["black", "blue"] record = PgArray.create!(tags: tags) assert_equal record, PgArray.where(tags: tags).take end # correct SELECT "pg_arrays".* FROM "pg_arrays" WHERE "pg_arrays"."tags" = $1 LIMIT $2 [["tags", "{black,blue}"], ["LIMIT", 1]] # removed type condition SELECT "pg_arrays".* FROM "pg_arrays" WHERE "pg_arrays"."tags" IN ('black', 'blue') LIMIT $1 [["LIMIT", 1]] — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#26098>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdWK74O-_49sTX402rRAhKh5BNWoSVHks5rGIt-gaJpZM4JgnmC> .

[sgrif]

I'd really prefer not to add more code which relies on type internals in this way. I think the code below this which does the same is a major smell. There's a greater problem with the structure here that we should address first.

[kamipo]

I agree that there's a problem with the structure.
How about the alternative #29033?
#29033 is a simple one line fix and we already have a similar code (for keeping existing expectation:

rails/activerecord/lib/active_record/relation/predicate_builder/polymorphic_array_value.rb

Line 13 in a6d6e90

associated_table.association_foreign_key.to_s => ids.size > 1 ? ids : ids.first

).
I'd like to add the expectation that typical generated queries will be preparable even if association queries.

[sgrif]

Yeah I like #29033. Should we close this one?

[kamipo]

Yeah, I'm closing this for now.

[kamipo]

Closing in favor of #29033.