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

Introduce query-by-tuple syntax #47729

Merged
merged 3 commits into from
Mar 22, 2023
Merged

Conversation

paarthmadan
Copy link
Contributor

Motivation / Background

This PR is related to the ongoing effort of introducing composite primary keys.

A very common Rails abstraction pattern is performing a search given a primary key. Across Rails we see numerous instances of code like this:

Now that a model can have a primary key that's composed of more than 1 column, a common querying pattern is to look for a set of records given their composite keys.

This PR brings support for querying using tuple-syntax (happy to hear suggestions on other names):

book_one = Cpk::Book.create!(author_id: 1, number: 2)
book_two = Cpk::Book.create!(author_id: 3, number: 4)

# Provide a mapping between a list of columns and an array of corresponding tuples
Cpk::Book.where([:author_id, :number] => [[1, 2]]) # Relation(book_one)
Cpk::Book.where(Cpk::Book.primary_key => [[1, 2], [3, 4]]) # Relation(book_one, book_two)

Detail

This PR changes the predicate builder to consider the case when a key in the Hash supplied to #where is an array. In that case, it iterates across the set of tuples and computes what the query would look like individually. Previously, it was assumed keys and values were singular. Now, the method accepts an array. When encountering one, we iterate through each tuple, and compute a query by deferring each sub-task to the original implementation (i.e recursively), and then finally regrouping (collecting) the queries.

The implementation of grouping_queries injects an OR in between sub-queries.

Note: This PR doesn't concern itself too heavily with the performance or format of the generated SQL. We've started to consider future iterations where a user-defined config (think: active_record.config.bulk_query_strategy) can be used to configure how and what SQL Active Record generates to perform this query. This would allow us to consider building row constructor SQL, appending hints in sharded contexts, or simply falling back to the current "OR-separated" clauses.

In any case, this PR only concerns itself with designing the API for querying by tuple and is less focused on how the query executes.

This PR will help avoid branching and commits like 26994cb06e6214bf387cc34adbb7fb29834b715e will soon be common as we replace many instances of branching with this new syntax.

This new syntax also helps de-risk the composite primary key rollout because many callsites will "automatically" work now that the abstraction works in both the singular and composite case.

Additional information

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@paarthmadan
Copy link
Contributor Author

Thoughts on this change requiring a CHANGELOG entry? I'd assume it warrants one because it introduces a pretty significant syntax change that's not directly tied to CPK. Thoughts: cc @eileencodes @nvasilevski

@paarthmadan paarthmadan changed the title Introduced query-by-tuple syntax Introduce query-by-tuple syntax Mar 21, 2023
@eileencodes
Copy link
Member

And yes please add a changelog entry 👍🏼

With the introduction of composite primary keys, a common usecase is querying for records with tuples representing the composite key. This change introduces new syntax to the where clause that allows specifying an array of columns mapped to a list of corresponding tuples. It converts this to an OR-joined list of separate queries, similar to previous implementations that rely on grouping queries.
@paarthmadan
Copy link
Contributor Author

@eileencodes CHANGELOG entry added 👍 !

FWIW: I've split the change into two commits because the latter isn't related at all to the introduction of the syntax. There'll be more refactoring changes like that one, so I didn't want to convolute the commit which makes concrete changes to the public API.

@eileencodes
Copy link
Member

@paarthmadan - intersting, the build is failing but not consistently. Looks like one of the new tuple tests is flaky. Can you take a look? https://buildkite.com/rails/rails/builds/94980#01870a78-1d3b-4bb6-ad35-e859d3597858/1036-1046

Having this record persisted across test runs was leading to flakey behaviour in tests that were creating Cpk::Book with the same primary key.
@eileencodes eileencodes merged commit 2cef3ac into rails:main Mar 22, 2023
@eileencodes eileencodes deleted the pm/cpk-where-syntax branch March 22, 2023 21:08
gmcgibbon added a commit to gmcgibbon/rails that referenced this pull request Sep 5, 2023
Adds documentation to ActiveRecord::QueryMethods#where mentioning tuple
syntax support added in rails#47729.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants