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

feat: Support Through Relationships #1780

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

ken-kost
Copy link
Contributor

@ken-kost ken-kost commented Feb 8, 2025

#72

Contributor checklist

  • Bug fixes include regression tests
  • Chores
  • Documentation changes
  • Features include unit/acceptance tests
  • Refactoring
  • Update dependencies

@ken-kost
Copy link
Contributor Author

ken-kost commented Feb 8, 2025

is this a feat or enhancement? 🤔😅

@zachdaniel
Copy link
Contributor

This would be a feature 😄 . Will have to review next week.

@ken-kost ken-kost force-pushed the feat-relationships-enable-through-option branch from 39b2366 to d301173 Compare February 9, 2025 09:11
@ken-kost ken-kost marked this pull request as draft February 9, 2025 10:07
@@ -1191,6 +1251,9 @@ defmodule Ash.Actions.Read.Relationships do
has_page? = query.page not in [nil, false]

cond do
is_list(relationship.through) ->
false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this will actually be the way that we enable data layers to join through relationships. So this should return true if the data layer can :lateral_join_through_relationships or something. For now, nothing will implement this, so we can leave it as a stub and talk about that implementation later. It is actually quite complex to do 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact...yeah returning false makes sense here, never mind. When its time to support doing this in an optimized way in the data layer, then we'd return true here.

@@ -612,6 +623,50 @@ defmodule Ash.Actions.Read.Relationships do
)
end

defp load_related_records(records, through, cardinality) do
load_statement =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to pass through the actor, tenant, authorize? and tracer options from the parent call to this call.

Additionally, we should call Ash.load with the list of records, not each individual record. That will be much more optimized.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I overengineered it. 👷

@ken-kost ken-kost requested a review from zachdaniel February 16, 2025 12:00
@zachdaniel
Copy link
Contributor

I'm realizing an issue with our "do it in memory" solution here, which is that limits/offsets in Ash relationships apply "per-related-record". Our in-memory loader here won't be able to provide those same guarantees. I think what we may need to do is make it such that ash_postgres and ash_sql actually have first class support for through relationships 🥶. That will make this a bigger project, but it will also be a major win.

What that would essentially look like in ash_postgres code base is

  • modifying the run_query_from_lateral_join code to support arbitrarily long paths.
  • modifying the joining code in ash_sql to expand the through path automatically whenever it needs to join for any reason (aggregates.ex, join.ex)

Then in this loader, we'd do the same thing that lateral joining currently does, but setting the full path to the relationship.

What I'm describing is a pretty intense change TBH. I'm happy to support you in the effort, but I imagine it would be quite some time before we can get it where we want it 😓

@ken-kost
Copy link
Contributor Author

which is that limits/offsets in Ash relationships apply "per-related-record".

wdym? Could you give an example? I assume if I put limit 1 I should also get 1 related?

but yea, I agree, this naive attempt only served to elucidate what really needs to be done.

I'm still down to bring this to an end. Step by step, heart to heart. I can at least try. ✊♥️

@zachdaniel
Copy link
Contributor

zachdaniel commented Feb 17, 2025

What I mean is that when you do posts |> Ash.load(comments: Ash.Query.limit(Foo, 10)) you get 10 records per post

@ken-kost
Copy link
Contributor Author

Should this PR be closed then?
So if I understand correctly, first on the list is enabling arbitrary path length for lateral_join_query in ash_postgres.
Currently there is

  defp lateral_join_query(
         query,
         root_data,
         [{source_query, source_attribute, destination_attribute, relationship}] = path
       ) do
      ...
 end

and

  defp lateral_join_query(
         query,
         root_data,
         [
           {source_query, source_attribute, source_attribute_on_join_resource, relationship},
           {through_resource, destination_attribute_on_join_resource, destination_attribute,
            through_relationship}
         ] = path
       ) do
       ...
  end

@zachdaniel
Copy link
Contributor

Yep! There is also logic in ash_sql that does similar things that needs to be updated. As for closing this PR, my suggestion would instead be to pare back all of the features, leaving just the through syntax, and add a resource verifier that checks if the data layer supports a new capability, like :lateral_join_through that gets both the resource and the relationship structure.

Then we can merge this PR, despite no one being able to use this new relationship option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants