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 coercing all SELECTs with the same column name to the same value #36186

Closed
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -96,12 +96,12 @@ def execute(sql, name = nil)

def exec_query(sql, name = "SQL", binds = [], prepare: false)
execute_and_clear(sql, name, binds, prepare: prepare) do |result|
types = {}
types = []
fields = result.fields
fields.each_with_index do |fname, i|
ftype = result.ftype i
fmod = result.fmod i
types[fname] = get_oid_type(ftype, fmod, fname)
types << get_oid_type(ftype, fmod, fname)
end
build_result(columns: fields, rows: result.values, column_types: types)
Copy link
Member

Choose a reason for hiding this comment

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

I think this column_types: parameter should be renamed to types:.

The default value is now of the wrong type too:

def build_result(columns:, rows:, column_types: {})

end
Expand Down
26 changes: 18 additions & 8 deletions activerecord/lib/active_record/result.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,13 @@ module ActiveRecord
class Result
include Enumerable

attr_reader :columns, :rows, :column_types
attr_reader :columns, :rows, :types
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a strong reason to rename the interface/instance variable to types. This can break something that's not covered by tests, e.g. the initialize_copy is now broken because it only copies @column_type but not @types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 Good callout re: initialize_copy! I'll audit uses of the instance variable in this class.

I wouldn't expect the instance variable to be part of this classes interface, though, and I did implement column_types (below) to maintain the classes interface.

My concern with simply reusing @column_types for the new third argument to initialize is that the type of that instance variable has changed (from a dictionary of types correlated to columns by name to an array of types correlated to columns by position).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

initialize_copy appears to be the only place where @column_types was referenced — I made that change! 👍 ✅


def initialize(columns, rows, column_types = {})
def initialize(columns, rows, types = [])
@columns = columns
@rows = rows
@hash_rows = nil
@column_types = column_types
@types = types
boblail marked this conversation as resolved.
Show resolved Hide resolved
end

# Returns true if this result set includes the column named +name+
Expand Down Expand Up @@ -110,13 +110,15 @@ def cast_values(type_overrides = {}) # :nodoc:
if columns.one?
# Separated to avoid allocating an array per row

type = column_type(columns.first, type_overrides)
type = column_type(columns.first, 0, type_overrides)

rows.map do |(value)|
type.deserialize(value)
end
else
types = columns.map { |name| column_type(name, type_overrides) }
types = columns.map.with_index do |name, i|
column_type(name, i, type_overrides)
end

rows.map do |values|
Array.new(values.size) { |i| types[i].deserialize(values[i]) }
Expand All @@ -127,14 +129,22 @@ def cast_values(type_overrides = {}) # :nodoc:
def initialize_copy(other)
@columns = columns.dup
@rows = rows.dup
@column_types = column_types.dup
@hash_rows = nil
@types = types.dup
end

def column_types
if @types.present?
boblail marked this conversation as resolved.
Show resolved Hide resolved
Hash[@columns.zip(@types)]
Copy link
Member

Choose a reason for hiding this comment

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

This method is sometimes called inside a loop:

calculated_data.column_types.fetch(aliaz, Type.default_value)

The result should be memoised so that it will only be calculated once, as it was before this change.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it's worth roflscaling this a bit, similar to hash_rows below, since this implementation allocates more intermediate objects than before (a two-element array for each column, and an array to hold them all).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since @st0012 was suggesting it earlier, I roflscaled this by assigning @column_types in initialize

else
{}
end
end

private
def column_type(name, type_overrides = {})
def column_type(name, index, type_overrides = {})
type_overrides.fetch(name) do
column_types.fetch(name, Type.default_value)
types[index] || Type.default_value
end
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
require "cases/helper"
require "support/ddl_helper"
require "support/connection_helper"
require "models/book"
require "models/author"

module ActiveRecord
module ConnectionAdapters
Expand Down Expand Up @@ -342,6 +344,25 @@ def test_reload_type_map_for_newly_defined_types
reset_connection
end

def test_support_different_types_for_results_with_the_same_column_name
result = @connection.select_all "select COALESCE(NULL, 'four'), COALESCE(NULL, 4)"
assert_equal [["four", 4]], result.cast_values
assert_instance_of(ActiveRecord::Type::Text, result.types[0])
assert_instance_of(ActiveRecord::Type::Integer, result.types[1])
end

def test_support_different_types_for_columns_with_the_same_name_but_different_tables
author = Author.create!(name: "Jules Verne", status: "deceased")
Book.create!(author: author, name: "Around the World in Eighty Days", status: :published)
result = @connection.select_all <<~SQL
SELECT authors.status, books.status
FROM authors
INNER JOIN books ON books.author_id = authors.id
WHERE books.name = 'Around the World in Eighty Days'
SQL
assert_equal [["deceased", 2]], result.cast_values
end

def test_only_reload_type_map_once_for_every_unrecognized_type
reset_connection
connection = ActiveRecord::Base.connection
Expand Down
2 changes: 1 addition & 1 deletion activerecord/test/cases/json_serialization_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ def test_should_allow_except_option_for_list_of_authors
authors = [@david, @mary]
encoded = ActiveSupport::JSON.encode(authors, except: [
:name, :author_address_id, :author_address_extra_id,
:organization_id, :owned_essay_id
:organization_id, :owned_essay_id, :status
])
assert_equal %([{"id":1},{"id":2}]), encoded
end
Expand Down
8 changes: 4 additions & 4 deletions activerecord/test/cases/result_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ def result
test "cast_values returns rows after type casting" do
values = [["1.1", "2.2"], ["3.3", "4.4"]]
columns = ["col1", "col2"]
types = { "col1" => Type::Integer.new, "col2" => Type::Float.new }
types = [Type::Integer.new, Type::Float.new]
result = Result.new(columns, values, types)

assert_equal [[1, 2.2], [3, 4.4]], result.cast_values
Expand All @@ -78,7 +78,7 @@ def result
test "cast_values uses identity type for unknown types" do
values = [["1.1", "2.2"], ["3.3", "4.4"]]
columns = ["col1", "col2"]
types = { "col1" => Type::Integer.new }
types = [Type::Integer.new]
result = Result.new(columns, values, types)

assert_equal [[1, "2.2"], [3, "4.4"]], result.cast_values
Expand All @@ -87,7 +87,7 @@ def result
test "cast_values returns single dimensional array if single column" do
values = [["1.1"], ["3.3"]]
columns = ["col1"]
types = { "col1" => Type::Integer.new }
types = [Type::Integer.new]
result = Result.new(columns, values, types)

assert_equal [1, 3], result.cast_values
Expand All @@ -96,7 +96,7 @@ def result
test "cast_values can receive types to use instead" do
values = [["1.1", "2.2"], ["3.3", "4.4"]]
columns = ["col1", "col2"]
types = { "col1" => Type::Integer.new, "col2" => Type::Float.new }
types = [Type::Integer.new, Type::Float.new]
result = Result.new(columns, values, types)

assert_equal [[1.1, 2.2], [3.3, 4.4]], result.cast_values("col1" => Type::Float.new)
Expand Down
1 change: 1 addition & 0 deletions activerecord/test/schema/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@
t.references :author_address_extra
t.string :organization_id
t.string :owned_essay_id
t.string :status
end

create_table :author_addresses, force: true do |t|
Expand Down