[ActiveRecord] ToSql#HomogeneousIn uses visitor pattern to produce Attribute sql

[kbrock]

Summary

In ToSql, operators that generate the sql for an Attribute use visit(o.left, collector) This can be seen in Equality (and friends), Case, SelectStatement, and In.

Before

HomogeneousIn manually produces the sql for an Attribute, introduced in 72fd0bae

To get this to work, knowledge of attributes and table aliases need to be in the node.

After

This change makes HomogeneousIn follow the pattern by delegating to ToSql#Attribute.

Why

Our code base uses Arel on the left hand of the where() clauses. This change lets it flow nicely.

It is also amusing that Homogeneous-In is not consistent with the other operators, including the forked parent In.

This added consistency allows for more flexible and diverse sql generation.

Slightly tweaked code examples

This code has been simplified to help illuminate the need for the change.

def visit_Arel_Nodes_In(o, collector)
  collector.preparable = false

  ### using visit
  visit(o.left, collector) << " IN ("
  visit(o.right, collector) << ")"
end

def visit_Arel_Nodes_Equality(o, collector)
  ### using visit
  visit o.left, collector

  if o.right.nil?
    collector << " IS NULL"
  else
    collector << " = "
    visit o.right, collector
  end
end

def visit_Arel_Attributes_Attribute(o, collector)
  join_name = o.relation.table_alias || o.relation.name
  collector << quote_table_name(join_name) << "." << quote_column_name(o.name)
end

def visit_Arel_Nodes_HomogeneousIn(o, collector)
  collector.preparable = false

  # before: not using visit, but partially recreating Attribute instead
  collector << quote_table_name(o.table_name) << "." << quote_column_name(o.column_name)
  # after:
  # visit o.left, collector

  collector << " IN ("

  values = o.casted_values
  collector.add_binds(values, o.proc_for_binds, &bind_block)

  collector << ")"
end

[kbrock]

Hey @matthewd -- pretty sure you know the background of my intent, please push me in what ever direction you want me to take. (even if it is for a pet project of yours)

ASIDE: I had tried going closer in the attribute route, but the only way I could get this working was to make the attributes look like db columns, but then couldn't find a way to make optional columns that weren't in every SELECT *.

@tenderlove if it was a conscious intention to code the attribute handling in HomogeneousIn, please share.

Thanks all

[kbrock]

Hi @eileencodes

This is all about an internal API, so thank you for the consideration.

If you look in to_sql.rb, [almost] all the places that produce SQL for an attribute delegate to the Attribute visitor via visit o.left, collector. I'm hoping we can do the same in HomogeneousIn.

The performance gains you and Aaron made were on the right hand side / the casting portion. Such a great find that you two made.

reproducer: https://gist.github.com/kbrock/7e7d2343801f0211fef11d6024e72e37

[matthewd]

While acknowledging that it's indistinguishable from public API, so not testable at the AR level, @kbrock can you see how practical it is to cover this with an Arel-internal test (activerecord/test/cases/arel/)? If we're saying this is a technically better implementation, it'd be good to make it obvious if we later make a change that undoes it.

[kbrock]

@matthewd not sure if you wanted a test that is testing basic In functionality, or if you wanted a failing one. I put one of each but feel the failing one is a bit much.

The casting functionality of HomogeneousIn is a little out of scope for Arel and crosses the line into ActiveRecord/ActiveModel. It made it a little tricky but I borrowed from the Attribute tests.

Thanks for the help

[kbrock]

update:

• rebased

@matthewd I did put in an arel only test for HomogeneousIn. Let me know if you need more tests for this or any other nodes.

[kbrock]

Thank you @eileencodes This is changing an internal interface and I appreciate you humoring me on this change.

Sorry to /cc Mathew on this and not you. I did it because he is familiar with my project's code base and can give me flack when I am asking for something stupid.

Thanks agin for the incredible HomogeneousIn changes that you made (among thousands of other improvements). This code rocks.

[eileencodes]

I'm not offended you didn't cc me, I still saw the email about the test but didn't merge it because I wanted to wait if Matthew had any objections or suggestions.