From 71e45582b794d7cdcbd72a93dc2f3c8665859653 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Fri, 1 May 2020 09:46:23 +0900 Subject: [PATCH] Avoid quite useless loop in `find_by_sql` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `column_types` is empty except PostgreSQL adapter, and `attribute_types.each_key { |k| column_types.delete k }` is also empty even if PostgreSQL adapter almost all case, so that code is quite useless. This improves performance for `find_by_sql` to avoid that useless loop as much as possible. ```ruby ActiveRecord::Schema.define do create_table :active_storage_blobs do |t| t.string :key, null: false t.string :filename, null: false t.string :content_type t.text :metadata t.string :service_name, null: false t.bigint :byte_size, null: false t.string :checksum, null: false t.datetime :created_at, null: false t.index [ :key ], unique: true end end class ActiveStorageBlob < ActiveRecord::Base end Benchmark.ips do |x| x.report("find_by") { ActiveStorageBlob.find_by(id: 1) } end ``` Before: ``` Warming up -------------------------------------- find_by 1.256k i/100ms Calculating ------------------------------------- find_by 12.595k (± 3.4%) i/s - 64.056k in 5.091599s ``` After: ``` Warming up -------------------------------------- find_by 1.341k i/100ms Calculating ------------------------------------- find_by 13.170k (± 3.5%) i/s - 67.050k in 5.097439s ``` To avoid column types loop for PostgreSQL adapter, this skips returning additional column types if a column has already been type casted by pg decoders. Fortunately this fixes #36186 partly for common types. --- .../postgresql/database_statements.rb | 6 ++++- activerecord/lib/active_record/querying.rb | 8 +++++-- activerecord/test/cases/calculations_test.rb | 22 +++++++++++++++++++ 3 files changed, 33 insertions(+), 3 deletions(-) diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb b/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb index 6ec3826226..ade731038c 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb @@ -54,7 +54,11 @@ def exec_query(sql, name = "SQL", binds = [], prepare: false) fields.each_with_index do |fname, i| ftype = result.ftype i fmod = result.fmod i - types[fname] = get_oid_type(ftype, fmod, fname) + case type = get_oid_type(ftype, fmod, fname) + when Type::Integer, Type::Float, Type::Decimal, Type::String, Type::DateTime, Type::Boolean + # skip if a column has already been type casted by pg decoders + else types[fname] = type + end end build_result(columns: fields, rows: result.values, column_types: types) end diff --git a/activerecord/lib/active_record/querying.rb b/activerecord/lib/active_record/querying.rb index 7260a22c7c..d8b0b50811 100644 --- a/activerecord/lib/active_record/querying.rb +++ b/activerecord/lib/active_record/querying.rb @@ -44,8 +44,12 @@ module Querying # Post.find_by_sql ["SELECT body FROM comments WHERE author = :user_id OR approved_by = :user_id", { :user_id => user_id }] def find_by_sql(sql, binds = [], preparable: nil, &block) result_set = connection.select_all(sanitize_sql(sql), "#{name} Load", binds, preparable: preparable) - column_types = result_set.column_types.dup - attribute_types.each_key { |k| column_types.delete k } + column_types = result_set.column_types + + unless column_types.empty? + column_types = column_types.reject { |k, _| attribute_types.key?(k) } + end + message_bus = ActiveSupport::Notifications.instrumenter payload = { diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index 36805ac1b3..3c226669e9 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -861,6 +861,28 @@ def test_pluck_columns_with_same_name assert_equal expected, actual end + def test_pluck_functions_with_alias + assert_equal [ + [1, "The First Topic"], [2, "The Second Topic of the day"], + [3, "The Third Topic of the day"], [4, "The Fourth Topic of the day"], + [5, "The Fifth Topic of the day"] + ], Topic.order(:id).pluck( + Arel.sql("COALESCE(id, 0) id"), + Arel.sql("COALESCE(title, 'untitled') title") + ) + end + + def test_pluck_functions_without_alias + assert_equal [ + [1, "The First Topic"], [2, "The Second Topic of the day"], + [3, "The Third Topic of the day"], [4, "The Fourth Topic of the day"], + [5, "The Fifth Topic of the day"] + ], Topic.order(:id).pluck( + Arel.sql("COALESCE(id, 0)"), + Arel.sql("COALESCE(title, 'untitled')") + ) + end + def test_calculation_with_polymorphic_relation part = ShipPart.create!(name: "has trinket") part.trinkets.create!