Avoid coercing all SELECTs with the same column name to the same value

[boblail]

Overview of Issue

Rails had been mapping types to columns in a Result Set by indexing types on columns' names. When two values in a Result Set have the same column name, they are forced to share a type.

There are several scenarios where multiple values may share the same column name.

One involves

1. One scenario (where two tables happen to have columns with the same name but the column's values differ) is explained in Plucking associated data from an enum column returns nil #36042. This PR does not resolve the issue in Plucking associated data from an enum column returns nil #36042 but proved part of a solution to that issue.

2. Another is when using functions in a .select or .pluck. In the absence of an Alias clause, Postgres names values with the last function called, so if two values in the same query happen to use COALESCE, they'll both be named "coalesce".

   psql> SELECT COALESCE(NULL, 'four'), COALESCE(NULL, 4);
    coalesce | coalesce
   ----------+----------
    four     |        4
   (1 row)
   

   ActiveRecord::Base.pluck(
     Arel.sql("COALESCE(NULL, 'four')"),
     Arel.sql("COALESCE(NULL, 4)")
   ) # => [[0, 4]]

I was able to reproduce this on Rails 4.2.11.1, 5.0.7.2, 5.1.7, 5.2.3, and master.

Solution

This commit matches types with columns and values (in a row) by array position (which is an assumption already made in PostgreSQL::DatabaseStatements#exec_query). It adds column_types as a public method on ActiveRecord::Result to preserve the class's public API.

Concerns

Does Rails expect client code to be constructing ActiveRecord::Result objects? Do we need to check whether its third argument is a Hash, give — maybe — a deprecation warning and convert it to an Array?

[st0012]

Just some suggestions after quick review:

• I think the solution makes sense, great job 😄
• Can you fix the codeclimate issue? So it doesn't look broken on the PR list page.
• Since you this PR fixes Plucking associated data from an enum column returns nil #36042, can you add some test cases similar to the ones in that issue?

[rails-bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

[rails-bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

[boblail]

@st0012, rebased. I think the specs were failing b/c of a flaky test. Rebuilt and it looks OK now.

I added two test cases for this. The second — test_support_different_types_for_columns_with_the_same_name_but_different_tables is the same scenario as in #36042.

[boblail]

@kamipo, could I get 👀 on this PR? I know you've got a strong familiarity with ActiveRecord and Postgres. I'm not sure if mentioning you is the right protocol — sorry if it's not 😬. (I'm not quite sure how to promote my PR and solicit a review.)

🙏 Thanks!

[st0012]

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.

[boblail]

🤔 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).

[boblail]

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

[eugeneius]

This method is sometimes called inside a loop:

rails/activerecord/lib/active_record/relation/calculations.rb

Line 373 in a3f62cb

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.

[eugeneius]

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).

[boblail]

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

[eugeneius]

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

The default value is now of the wrong type too:

rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb

Line 746 in a3f62cb

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

[kamipo]

Actually this doesn't fix #36042, I'm not excited to change the cast_values' behavior which is private API unless it does fix public API (e.g. pluck etc).

[boblail]

@kamipo, great callout! This does not fix #36042. We'd have to handle type_overrides differently here.

I added a test, though, to demonstrate a bug with pluck that this PR does still fix: 6b57bf9

When run in master, the new test fails like this:

[kamipo]

I know this issue affects only for PostgreSQL adapter (originally works for other MySQL, SQLite3, Oracle, and SQL Server adapters), I'm still not excited extra allocation for all cases to address such a minor case.

Like as a workaround for #36042, is it not enough to give a column alias?

relation.pluck(Arel.sql("COALESCE(format, 'unspecified') a, COALESCE(nullable_status, 0) b"))

[boblail]

@kamipo,

Yep! Using an alias is a totally viable workaround! (When I first made this PR, my team also went that route: boblail/pluck_map#14)

So if you want to decline this PR, we do have an acceptable path forward.

I just think that the behavior is surprising — especially since you get different results from MySQL and Sqlite3. For the developer troubleshooting the issue, the root cause is not immediately obvious!!

Only a developer who has spelunked in Rails' source would be able to tell that Rails keys type information on name and Postgres returns more-generic names than the other two databases:

Here's the output from each database in its REPL:

SQLite

sqlite> select COALESCE(NULL, 'four'), COALESCE(NULL, 4);
COALESCE(NULL, 'four')  COALESCE(NULL, 4)
----------------------  -----------------
four                    4

MySQL

mysql> select COALESCE(NULL, 'four'), COALESCE(NULL, 4);
+------------------------+-------------------+
| COALESCE(NULL, 'four') | COALESCE(NULL, 4) |
+------------------------+-------------------+
| four                   |                 4 |
+------------------------+-------------------+
1 row in set (0.00 sec)

PostgreSQL

boblail=# select COALESCE(NULL, 'four'), COALESCE(NULL, 4);
 coalesce | coalesce
----------+----------
 four     |        4
(1 row)

---

Do you think it would make sense for Rails to raise an exception if an ActiveRecord::Result uses the same name more than once???

OR — the extra allocation you're referring to — is that the Hash of @column_types in addition to the Array of @types? Would this PR be acceptable if we did that allocation only on-demand or could do without it?

(Thanks for the feedback!! 😄)

[simi]

As @boblail mentioned (and I agree)

ActiveRecord::Base.pluck(
  Arel.sql("COALESCE(NULL, 'four')"),
  Arel.sql("COALESCE(NULL, 4)")
) # => [[0, 4]]

is not expected result. Anyway I agree with @kamipo this is big change to handle this (probably minor) problem. It would be nice to at least mention this in documentation or raise on unsupported (conflicting) input to not surprise developers (and keep the Rails experience nice).