-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Perf: Improve performance of where when using an array of values #39009
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,14 +8,21 @@ def serialize(value) | |
cast(value) | ||
end | ||
|
||
def serialize2(value) | ||
cast(value) | ||
end | ||
|
||
def cast(value) | ||
value = \ | ||
value = if value <=> 0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably add a comment here about why we're doing this. This is a huge "hack" to check to see whether or not There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm curious why not use |
||
value | ||
else | ||
case value | ||
when true then 1 | ||
when false then 0 | ||
when ::String then value.presence | ||
else value | ||
else value.presence | ||
end | ||
end | ||
|
||
super(value) | ||
end | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,10 @@ def initialize(precision: nil, limit: nil, scale: nil) | |
@limit = limit | ||
end | ||
|
||
def serializable?(_) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be part of the public API? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it should. I would like it if active model types can tell us whether or not a value can be serialized for that particular type. We should add some docs, but I didn't want to go that far unless everyone was OK with adding this to the public API |
||
true | ||
end | ||
|
||
def type # :nodoc: | ||
end | ||
|
||
|
@@ -46,6 +50,10 @@ def serialize(value) | |
value | ||
end | ||
|
||
def serialize2(value) | ||
value | ||
end | ||
|
||
# Type casts a value for schema dumping. This method is private, as we are | ||
# hoping to remove it entirely. | ||
def type_cast_for_schema(value) # :nodoc: | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -21,10 +21,22 @@ def call(attribute, value) | |||
when 0 then NullPredicate | ||||
when 1 then predicate_builder.build(attribute, values.first) | ||||
else | ||||
values.map! do |v| | ||||
predicate_builder.build_bind_attribute(attribute.name, v) | ||||
if nils.empty? && ranges.empty? | ||||
predicate_builder.connection.in_clause_length | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think we can remove this. |
||||
join_name = attribute.relation.table_alias || attribute.relation.name | ||||
quoted_column_name = "#{predicate_builder.connection.quote_table_name(join_name)}.#{predicate_builder.connection.quote_column_name(attribute.name)}" | ||||
type = attribute.type_caster | ||||
|
||||
casted_values = values.map do |raw_value| | ||||
type.serialize2(raw_value) if type.serializable?(raw_value) | ||||
end | ||||
|
||||
casted_values.compact! | ||||
|
||||
return Arel::Nodes::HomogeneousIn.new(quoted_column_name, casted_values, attribute, :in) | ||||
else | ||||
build_in(values, predicate_builder, attribute) | ||||
end | ||||
values.empty? ? NullPredicate : attribute.in(values) | ||||
end | ||||
|
||||
unless nils.empty? | ||||
|
@@ -39,6 +51,12 @@ def call(attribute, value) | |||
private | ||||
attr_reader :predicate_builder | ||||
|
||||
def build_in(values, predicate_builder, attribute) | ||||
attribute.in values.map { |v| | ||||
predicate_builder.build_bind_attribute(attribute.name, v) | ||||
} | ||||
end | ||||
|
||||
module NullPredicate # :nodoc: | ||||
def self.or(other) | ||||
other | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
# frozen_string_literal: true | ||
|
||
module Arel # :nodoc: all | ||
module Nodes | ||
class HomogeneousIn < Node | ||
attr_reader :attribute, :quoted_column_name, :values, :type | ||
|
||
def initialize(quoted_column_name, values, attribute, type) | ||
@quoted_column_name = quoted_column_name | ||
@values = values | ||
@attribute = attribute | ||
@type = type | ||
end | ||
|
||
def hash | ||
ivars.hash | ||
end | ||
|
||
def eql?(other) | ||
super || (self.class == other.class && self.ivars == other.ivars) | ||
end | ||
alias :== :eql? | ||
|
||
def equality? | ||
true | ||
end | ||
|
||
def invert | ||
Arel::Nodes::HomogeneousIn.new(quoted_column_name, values, attribute, type == :in ? :notin : :in) | ||
end | ||
|
||
def left | ||
attribute | ||
end | ||
|
||
def fetch_attribute(&block) | ||
if attribute | ||
yield attribute | ||
else | ||
expr.fetch_attribute(&block) | ||
end | ||
end | ||
|
||
protected | ||
def ivars | ||
[@attribute, @quoted_column_name, @values, @type] | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,6 +44,8 @@ def to_sql(engine = Table.engine) | |
|
||
def fetch_attribute | ||
end | ||
|
||
def equality?; false; end | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why
serialize2
exists? All definitions of it in this PR seems to have the same code asserialize
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used here https://github.com/rails/rails/pull/39009/files#diff-3bcf02e80bc52706c3df424d2a8c4db2R31 and it's a terrible name but we're not sure what to call it. The other serialize methods do both serialize and validation, this one just does serialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if all the implementations of
serialize2
on this PR is always the same as the implementation asserialize
I don't see the different andserialize2
is also doing serialization and validation, no?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not the same. Check the serialize method on Integer here. It's doing both serialization and validation. This one is only doing serialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the role of
serialize2
to be a fast, uncheckedserialize
(i.e. "garbage in, garbage out")? If so, wouldunchecked_serialize
be an appropriate name?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, that was the context I was missing. In this diff all
serialize2
methods have the same implementation of theserialize
in the same files, but I was missing that the subclasses were overriding the method.If we want to
serialize2
to be part of the public API I'd go with @jonathanhefner's suggestion, but with the long term goal of renaming it toserialize
and renaming the currentserialize
to something asverify_and_serialize
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rafaelfranca I like @jonathanhefner's suggestion too. @eileencodes wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me!