Opened 11 months ago

Closed 11 months ago

Last modified 3 months ago

#35064 closed Bug (fixed)

Window.order_by decimal field is broken on SQLite

Reported by: Simon Charette Owned by: Simon Charette
Component: Database layer (models, ORM) Version: 3.2
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Initially reported on the Discord and demonstrated in this Django project.

class RankTest(models.Model):
    name = models.CharField(max_length=30)
    category = models.CharField(max_length=30)
    rating = models.DecimalField(max_digits=8, decimal_places=5)

list(
    RankTest.objects.annotate(
        rank=Window(
            expression=Rank(),
            order_by='rating'
        )
    )
)

The solution implemented in #31723 (71d10ca8c90ccc1fd0ccd6683716dd3c3116ae6a) wish addressed the improper of casting for Window.expression caused some problematic one for order_by and likely partition_by as well.

Change History (8)

comment:1 by Mariusz Felisiak, 11 months ago

Triage Stage: UnreviewedAccepted

comment:2 by Simon Charette, 11 months ago

Owner: changed from nobody to Simon Charette
Status: newassigned

comment:3 by Simon Charette, 11 months ago

Has patch: set
Last edited 11 months ago by Mariusz Felisiak (previous) (diff)

comment:4 by Mariusz Felisiak, 11 months ago

Triage Stage: AcceptedReady for checkin

comment:5 by Mariusz Felisiak <felisiak.mariusz@…>, 11 months ago

In 90d365d8:

Refs #35064 -- Made OrderableAggMixin avoid creating empty OrderByList.

This paves the way for making OrderByList a simple shim over
ExpressionList which requires at least a single item to be provided.

comment:6 by Mariusz Felisiak <felisiak.mariusz@…>, 11 months ago

Resolution: fixed
Status: assignedclosed

In e16d0c1:

Fixed #35064 -- Fixed Window(order_by) crash with DecimalFields on SQLite.

This avoids cast of Window(order_by) for DecimalFields on SQLite.

This was achieved by piggy-backing ExpressionList which already
implements a specialized as_sqlite() method to override the inherited
behaviour of Func through SQLiteNumericMixin.

Refs #31723.

Thanks Quoates for the report.

comment:7 by Sarah Boyce <42296566+sarahboyce@…>, 3 months ago

In 602fe96:

Fixed #35665 -- Fixed a crash when passing an empty order_by to Window.

This also caused un-ordered sliced prefetches to crash as they rely on Window.

Regression in e16d0c176e9b89628cdec5e58c418378c4a2436a that made OrderByList
piggy-back ExpressionList without porting the empty handling that the latter
provided.

Supporting explicit empty ordering on Window functions and slicing is arguably
a foot-gun design due to how backends will return undeterministic results but
this is a problem that requires a larger discussion.

Refs #35064.

Thanks Andrew Backer for the report and Mariusz for the review.

comment:8 by Sarah Boyce <42296566+sarahboyce@…>, 3 months ago

In df236b0:

[5.1.x] Fixed #35665 -- Fixed a crash when passing an empty order_by to Window.

This also caused un-ordered sliced prefetches to crash as they rely on Window.

Regression in e16d0c176e9b89628cdec5e58c418378c4a2436a that made OrderByList
piggy-back ExpressionList without porting the empty handling that the latter
provided.

Supporting explicit empty ordering on Window functions and slicing is arguably
a foot-gun design due to how backends will return undeterministic results but
this is a problem that requires a larger discussion.

Refs #35064.

Thanks Andrew Backer for the report and Mariusz for the review.

Backport of 602fe961e6834d665f2359087a1272e9f9806b71 from main.

Note: See TracTickets for help on using tickets.
Back to Top