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 order_by() #26

Closed
wants to merge 1 commit into from
Closed

Conversation

marco-silva0000
Copy link

@marco-silva0000 marco-silva0000 commented Apr 16, 2019

implements the order by function

please review thoroughly as I'm not experienced with python3, async and sqiachemy

fixes #6

Copy link
Member

@tomchristie tomchristie left a comment

Choose a reason for hiding this comment

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

Great stuff!
Minor change, in that order_by shouldn't evalute the queryset itself, and should still require an explicit .all(), but otherwise looks good!

await Product.objects.create(name="Dress", rating=3)
await Product.objects.create(name="Coat", rating=3, in_stock=True)

coat = await Product.objects.order_by("name")
Copy link
Member

Choose a reason for hiding this comment

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

Let's use coat = await Product.objects.order_by("name").all() here.

@@ -190,6 +197,10 @@ def select_related(self, related):
raise MultipleMatches()
return self.model_cls.from_row(rows[0], select_related=self._select_related)

async def order_by(self, *args, **kwargs):
self._order_args = args
return await self.all(**kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't have order_by run .all(...), so let's just do:

def order_by(self, *args):
        self._order_args = args

Copy link
Member

Choose a reason for hiding this comment

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

Actually we should include order_args in the __init__, and create a new queryset in the same way we're currently doing in filter and select_related

@florimondmanca
Copy link
Member

florimondmanca commented May 8, 2019

Hi @marco-silva0000, have you had a chance to review Tom's requested changes yet? If you don't have time, I can take over a submit an updated PR based on this. I'd be very interested in adding .order_by() functionality.

@@ -48,6 +48,11 @@ def __new__(
return new_model


def prepare_order_args(order_args):
from sqlalchemy.sql import text
return [text(arg) for arg in order_args]
Copy link
Member

Choose a reason for hiding this comment

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

After trying this out, it seems this isn't sufficient to use DESC on more complex datatypes such as datetimes.

May I suggest the more explicit implementation below? I tested this locally and it worked fine e.g. on .order_by('-created').

def prepare_order(model_cls, order):
    prepared = []

    for clause in order:
        if clause.startswith("-"):
            desc = True
            col_name = clause.lstrip("-")
        else:
            desc = False
            col_name = clause

        col = model_cls.__table__.columns[col_name]
        prepared.append(col.desc() if desc else col)

    return prepared

Copy link
Author

Choose a reason for hiding this comment

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

that war the initial idea, and the reason I've made a helper function in the first place. But my knowledge of sqlalchemy is low. Do you think any other datatypes may cause problems?

Copy link
Member

@florimondmanca florimondmanca May 10, 2019

Choose a reason for hiding this comment

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

No problem, I’m also just getting started with the SQLAlchemy core internals. :-)

The documentation highlights .desc() as the recommended way to sorting in descending order: https://docs.sqlalchemy.org/en/13/core/sqlelement.html#sqlalchemy.sql.expression.desc

So I think this should be good. Maybe @tomchristie has hints on how best this should be tested?

@marco-silva0000
Copy link
Author

sorry, I wanted to reply earlier but the message stayed as a draft.

About the helper method that imports the 'text' class from alchemy, I made it like this because I was thinking I had to use different classes based on what fields were to be ordered.  But in the end it worked with just text.

@jsatt
Copy link

jsatt commented Oct 24, 2019

Any movement on this PR?

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.

.order_by(...)
4 participants