Skip to content

Nulls Last and Nulls First Parameters Sorting#769

Open
SepehrBazyar wants to merge 25 commits intoormar-orm:masterfrom
SepehrBazyar:nulls_last_first
Open

Nulls Last and Nulls First Parameters Sorting#769
SepehrBazyar wants to merge 25 commits intoormar-orm:masterfrom
SepehrBazyar:nulls_last_first

Conversation

@SepehrBazyar
Copy link
Contributor

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Aug 1, 2022

Codecov Report

Merging #769 (9690176) into master (e923513) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 9690176 differs from pull request most recent head 93d520d. Consider uploading reports for the commit 93d520d to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #769   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          192       192           
  Lines        16057     16113   +56     
=========================================
+ Hits         16057     16113   +56     
Impacted Files Coverage Δ
ormar/queryset/__init__.py 100.00% <100.00%> (ø)
ormar/queryset/actions/order_action.py 100.00% <100.00%> (ø)
ormar/queryset/clause.py 100.00% <100.00%> (ø)
ormar/queryset/field_accessor.py 100.00% <100.00%> (ø)
tests/test_queries/test_order_by.py 100.00% <100.00%> (ø)

@SepehrBazyar
Copy link
Contributor Author

Hi @collerek
Thank you, please, if you do the review.

@collerek
Copy link
Collaborator

Since both options are mutually exclusive (you cannot have nulls both last and first at the same time) that should be a single option, something like: nulls_ordering that accepts an Enum like Nulls.LAST or Nulls.FIRST and Nulls.UNSET as default (or None).

That would also greatly simplify the conditions in the code as you have explicit value to handle.

return result + f" nulls {self.nulls}" # pragma: no cover

condition: str = "not" if self.nulls == "first" else "" # pragma: no cover
return f"{field_name} is {condition} null, {result}" # pragma: no cover
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you split this condition into two the result now lacks prefix and will not work with nested models.

You would have in example {prefix}{table_name}.{field_name} is null, {field_name} {self.direction}.

The second field_name does not have the prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you split this condition into two the result now lacks prefix and will not work with nested models.

You would have in example {prefix}{table_name}.{field_name} is null, {field_name} {self.direction}.

The second field_name does not have the prefix.

Of course, I must mention that without this case, the test related to nested models will also pass correctly. Can you give an example of a possible error that may occur? Thanks

Copy link
Collaborator

Choose a reason for hiding this comment

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

When you have a parent model with two relations to the child model. So like items with created_by and modified_by that lead to the same User model. Each of those relations in joins has to have different prefixes as otherwise, you would have ambiguous column names.

return self._select_operator(op="isnull", other=other)

def asc(self) -> OrderAction:
def asc(self, nulls_ordering: Optional[NullsOrdering] = None) -> OrderAction:
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should also be able to pass this param in order_by in queryset and querysetproxy for related models.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You should also be able to pass this param in order_by in queryset and querysetproxy for related models.

I do not understand this
How to determine this amount in order_by() and without asc() or desc()?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In order by you can pass OrderActions or strings too. If you pass sort_order it's ascending, if you pass -sort_order it's descending. For nested models double underscore is required so like main_model_relation_field__child_model_field. It also supports putting a - before nested related model fields and then it's descending. The logic is starting in order_by where OrderActions are created from strings.

@SepehrBazyar SepehrBazyar requested a review from collerek March 27, 2023 19:48
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.

3 participants