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

Add support for descending sibling ordering, multi-field sibling ordering, and related field sibling ordering #62

Merged
merged 12 commits into from
Mar 26, 2024

Conversation

rhomboss
Copy link
Contributor

Addresses #6.

Uses the ROW_NUMBER() window expression to map any ordering criteria onto an ascending set of ordinal integers which can be used to properly order a tree.

Parameters for this new ordering method are generated by get_sibling_order_params() which uses the base Django SQLCompiler to convert a dummy Django queryset object to SQL. This way we are able to rely on Django's backend to generate the extra SQL needed for more complex orderings like ordering by a related field.

This change will also make supporting additional forms of ordering simpler. For example, ordering based on annotations is not currently supported. However, it could be added by having TreeQuerySet override the base annotate() method so that it additionally stores its initial args and kwargs as instance variables which can later be submitted to the dummy queryset used to generate ordering parameters.

rhomboss added 12 commits March 18, 2024 13:45
Update the compiler to support descending, and multi-field ordering through the use of the ROW_NUMBER() SQL window expression.
Update the order_siblings_by method to take a list of fields
Fix sibling_order_as_sql
Remove the quoting of the order_by param because its now handled by django when creating the ROW_NUMBER() SQL
added tuple handling to sibling_order_as_sql
Fix description of order_siblings_by to reflect support of multi-field ordering
Modified the compiler to accept multi-direction and multi-field ordering
Updated compiler to use a basic django query to help write the __rank_table used for ordering
Added models for testing ordering based on a related field
Added tests for ordering by descending order, ordering by a related field, and ordering by multiple fields
Make ordering during test_attributes deterministic
Copy link
Member

@matthiask matthiask left a comment

Choose a reason for hiding this comment

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

This looks very nice, thank you! I like it a lot.

Do you have any idea what the performance impact of the additional CTE is? I suspect it's minimal since the recursive CTE and tree building is probably much more work for the database. In any case, it would be nice if we had some data and not just gut feelings.

@rhomboss
Copy link
Contributor Author

I'm sorry, but I don't have much insight into how the extra CTE will impact performance. My basic understanding is that you can run into problems when you have too many nested CTEs because it will start to confuse the SQL engine. But one nested CTE is probably not too many.

Additionally, depending on the underlying data having the extra CTE could be more efficient than the standalone. For example #50 , if we were to add a WHERE statement to the __rank_table CTE to filter based on user_id we could increase performance by reducing the overall number of nodes that need to be traversed when constructing the __tree.

@matthiask
Copy link
Member

Re. filtering: Ah yes, this is an excellent point. I still have the hope that PostgreSQL will stop treating recursive CTEs as optimization fences at some point in the future. For the time being most trees I have are so small that it's not really a problem anyway.

Also, we could use a materialized view or something for the rank table and even the recursive table in the future if it's a problem. For now I'm going to merge this and upload a new release shortly. Thanks for your great work!

@matthiask matthiask merged commit bc17cd4 into feincms:main Mar 26, 2024
15 checks passed
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