Use Arel instead of String for AR Enumerator conditionals

[pedropb]

Problem statement

On our application we noticed some inconsistencies with the usage of Rails 7.1 and job-iteration.

For example:

enumerator_builder.active_record_on_batches(Model.all, cursor: cursor)

when iterating would result in queries like:

 WHERE (reconciled_transaction_fees.id > '238427110') ORDER BY reconciled_transaction_fees.id LIMIT 3
 WHERE (reconciled_transaction_fees.id > '304256793') ORDER BY reconciled_transaction_fees.id LIMIT 3
 WHERE (reconciled_transaction_fees.id > '421883103') ORDER BY reconciled_transaction_fees.id LIMIT 3

While reconciled_transaction_fees.id > '421883103' is not wrong for adapters like MySQL and Postgres, it relies on behaviour from those DBMS to convert from strings to integer and still use the indices.

While working on a BigQuery adapter for ActiveRecord, we noticed that BigQuery will raise when comparing numeric columns with strings.

Proposal

So instead of manipulating strings to build the paging conditional:
https://github.com/Shopify/job-iteration/blob/main/lib/job-iteration/active_record_batch_enumerator.rb#L97-L101

We propose using Rails Arel:

job-iteration/lib/job-iteration/active_record_cursor.rb

Lines 82 to 86 in 5d18040

	conditions = if @columns.size == @position.size
	column.gt(@position[i])
	else
	column.gteq(@position[i])
	end

which will emit queries without the quotes:

WHERE `reconciled_transaction_fees`.`id` > 238427110 ORDER BY `reconciled_transaction_fees`.`id` LIMIT 3
WHERE `reconciled_transaction_fees`.`id` > 304256793 ORDER BY `reconciled_transaction_fees`.`id` LIMIT 3
WHERE `reconciled_transaction_fees`.`id` > 421883103 ORDER BY `reconciled_transaction_fees`.`id` LIMIT 3

What's in this PR?

These changes affect:

• enumerator_builder.active_record_on_records
• enumerator_builder.active_record_on_batches

The change affects only the columns: optional argument.

• columns: can still be passed as strings or symbols. Internally, the job-iteration code will convert the symbols to Arel attributes and manipulate them as Arel instead of strings.
  - (new!) columns: can also be passed as Arel attributes.

The Ugly

There are 2 main problems with the approach:

1. Arel is a private API subject to change. I tested this with Rails 7 (the job-iteration unit tests) and 7.1 (my app's unit tests). I'm relying on CI to test against other builds.

2. The new code does not support multi-column cursor when more than one table is used (cursor: ["products.id", "comments.id"]). While trying to add support to that, I created a test file and noticed that job-iteration code had a bug for multi-column cursors on different tables. This PR does not resolve the bug (job-iteration can cause infinite loop when using multi-column cursor in different tables with the same attribute #457 )

[nvasilevski]

Arel is a private API subject to change.

Fair but it feels like building raw SQL is even less stable especially when it comes to supporting multiple db adapters

(new!) columns: can also be passed as Arel attributes.

But I wonder if that's something we should still disallow. One thing is to rely on Arel internally but the other thing is to allow applications to pass arel attributes and promise that the gem will always (within a major release) maintain it. Perhaps it should actually be disallowed

The new code does not support multi-column cursor when more than one table is used

But it wasn't supported before, right? Or at least it wasn't working due to a bug, right? So I'm not too concerned about it as long as it doesn't break something that used to be working

Overall I think this is a reasonable change. @Shopify/rails would love to hear more opinions!

[Mangara]

The overall change makes sense to me. On the input, I agree with Nikita - using Arel internally in the cursor is fine (assuming we have good test coverage for it), but the interface for users of the gem should just accept strings and symbols.

[pedropb]

Thanks for the feedback @nvasilevski @Mangara
I think it's a fair compromise to keep the Arel dependency internal to job-iteration and not expose/support it publicly.

Pushed commit 51d8f8e to drop the conditional that handled Arel inputs as suggested.

[Mangara]

I'm happy with the new implementation. Could you add a test case that fails before this change but passes with it?

[pedropb]

I'm happy with the new implementation. Could you add a test case that fails before this change but passes with it?

I can add a new unit test file and some test cases for #conditions, if that makes sense.

I want to call out that ActiveRecordCursor is marked as private class and the conditions method is protected. I'm mentioning this in case there are any strong opinions on testing patterns, because from a library perspective, testing if the query is emitted with quotes or not might not fit the grand-scheme of things.

[Mangara]

If it's important to prevent regressions, I'm fine with testing the precise SQL queries we generate. Ideally, we'd test the public interface of ActiveRecordEnumerator instead of ActiveRecordCursor or the conditions method directly.

[pedropb]

@Mangara, as suggested:

• assert_sql helper introduced in ff5935d
• and test case for the integer conditional verification that fails on main: f71d0ac

[Mangara]

Oh, before we merge - could you add a note to the change log?

[pedropb]

Change log added!
Merging.

[segiddins]

is this still necessary, now that it's an arel attribute being passed in instead of a string?

[pedropb]

This logic was left untouched because on the presence of join conditions, the ORDER by columns can become ambiguous and fail the queries.
The .all? clause was a constraint to prevent this failure mode.

Also FWIW, the changes in this PR were reverted in #483 (v1.5.1). There's more context about the bug it introduced in #483.