Add scope for leaves #388
Add scope for leaves #388
Conversation
2 similar comments
lib/ancestry/materialized_path.rb
Outdated
| @@ -50,6 +50,14 @@ def sibling_conditions(object) | |||
| t[ancestry_column].eq(node[ancestry_column]) | |||
| end | |||
|
|
|||
| # idintifies leaves that belongs to the object (excluding itself) | |||
| def leaf_conditions(object) | |||
There was a problem hiding this comment.
Does this method look useful to you, or are you simply following the existing pattern?
If you are following a pattern, could we do this method without the object parameter?
aside: I'd like to remove all the *_conditions methods but they are currently in the public API.
There was a problem hiding this comment.
Yes, I simply tried to respect the existing pattern as much as possible.
Just a question: I was assuming object parameter needs to be there to limit its scope within the subtree (just like descendant_conditions or child_conditions do), but would you rather prefer them to be an general methods that return all the nodes meeting with the conditions?
I'm totally fine to take that away, but just wanted to clarify.
lib/ancestry/materialized_path.rb
Outdated
| def leaf_conditions(object) | ||
| t = arel_table | ||
| node = to_node(object) | ||
| all_ancestor_ids = self.ancestry_base_class.all.map(&:ancestor_ids).flatten.uniq |
There was a problem hiding this comment.
Is it possible to determine these ids in a query and not bring back all the records?
This is the main reason why I've been reluctant to introduce leafs method. I don't want to bring back every record.
This brings back every record, determines ids, and then brings back more records.
Would have been better to keep all in a local variable and delete from there.
But again, I want to find a sql way to determine the leafs.
aside 2: Logic like this is the driver for my proposed MaterializedPathArray changes.
There was a problem hiding this comment.
As you say this approach can be very costly - perhaps way too costly just to obtain a few number of records.
Let me come back with a new PR (if you are okay) as I already have pure sql based approaches at hand. Would be nice if I can better understand how you see it / whether or not it makes sense.
|
After juggling with Arel for a while, I came to the conclusion (finally) that the original SQL approach attempted in PR #246 seems to make much better sense. (Really liked to incorporate new CONCAT feature introduced with Rails5, but with lots of subtle version dependencies, I found the overall code structure become quite ugly with Arel) As the original PR is not active since a while ago, let me try to re-shape the PR based on the prior efforts with a bit of the tests I made here 😺 Hope that makes sence! |
Hello,
A while ago I submitted the prior PR (#365 ) for adding
leavesto ancestry and wondering if I can have any feedback on it.Looks like there has been constant need for it (including myself), let me resubmit this PR rebased on the latest master.
Tests are covered to make sure it works properly.
(I understand there are discussions on it here and there, so please let me know if there are some ongoing works I missed to recognize.)
how it works
Basically the approach consists of two steps:
There is another approach I tried by joining the table to itself using ancestry id, however, I ended up with relying on a database-dependent function
CONCAT. I'm assuming that's not appropriate approach for this widely used gem.Anyway thanks for maintaining this gem, and hope that makes senses!