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.

Sign up for GitHub

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

Jump to bottom

Fixed #34967 -- Fixed queryset crash when grouping by constants on SQLite < 3.39. #17468

Merged
merged 1 commit into from
Nov 13, 2023
Merged

Fixed #34967 -- Fixed queryset crash when grouping by constants on SQLite < 3.39. #17468

merged 1 commit into from
Nov 13, 2023

Conversation

shangxiao
Copy link
Contributor

@shangxiao shangxiao commented Nov 12, 2023 •
edited

https://code.djangoproject.com/ticket/34967

The issue is only when values() is applied with a static Value, eg: qs.values(static=Value('static')).annotate(<some-aggregate>).filter(<on-aggregate>)

Postgres, MySQL (haven't tested Oracle) accept queries with HAVING yet no GROUP BY clause. The resulting behaviour is to consider the entire result (ie virtual table after where is applied) the group.

Equivalent behaviour would be to GROUP BY 'static' 🤷‍♂️ Forcing Value to return something fixes this (but at what cost? 🤔) (see conversation below)

@shangxiao shangxiao force-pushed the ticket_34967_sqlite_doesnt_like_no_group_by branch from 0cf7b58 to f7c0743 Compare November 12, 2023 13:53
@shangxiao
Copy link
Contributor Author

Looks like this fails if Value(None) is passed…

@charettes
Copy link
Member

@shangxiao your second solution is definitely the good one.

There's no point in grouping by constants and it's even harmful in the case of integer literal as that translates into unintended grouping by SELECT column index.

@shangxiao
Copy link
Contributor Author

@shangxiao your second solution is definitely the good one.

There's no point in grouping by constants and it's even harmful in the case of integer literal as that translates into unintended grouping by SELECT column index.

hi @charettes yeah I'm thinking perhaps even putting the 2nd solution behind a feature flag because I don't know how Oracle will respond to that given the older versions don't know what a "TRUE" is.

@shangxiao
Copy link
Contributor Author

buildbot test on oracle

@shangxiao
Copy link
Contributor Author

Buildbot, test on oracle

@shangxiao
Copy link
Contributor Author

buildbot, test on oracle.

@shangxiao shangxiao force-pushed the ticket_34967_sqlite_doesnt_like_no_group_by branch 2 times, most recently from 8a64f85 to ace07c4 Compare November 13, 2023 01:48
@shangxiao shangxiao changed the title Fixed #34967 -- Made Value() return itself in get_group_by_cols() to prevent SQLite from crashing Fixed #34967 -- Add missing GROUP BY when HAVING specified for SQLite Nov 13, 2023
felixxm
felixxm reviewed Nov 13, 2023
View reviewed changes
Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@shangxiao Thanks 👍 I left two small suggestions.

@felixxm felixxm changed the title Fixed #34967 -- Add missing GROUP BY when HAVING specified for SQLite Fixed #34967 -- Fixed queryset crash when grouping by constants on SQLite < 3.39. Nov 13, 2023
@shangxiao
Copy link
Contributor Author

@shangxiao Thanks 👍 I left two small suggestions.

nice I can squash things up and add Simon as co-author if you like

@shangxiao @charettes @felixxm
Fixed #34967 -- Fixed queryset crash when grouping by constants on SQ…
b863c5f 
…Lite < 3.39.

On SQLite < 3.39, this forces a GROUP BY clause with a HAVING clause
when no grouping is specified.

Co-authored-by: Simon Charette <charette.s@gmail.com>
@felixxm felixxm force-pushed the ticket_34967_sqlite_doesnt_like_no_group_by branch from 21e2643 to b863c5f Compare November 13, 2023 11:09
@felixxm
Copy link
Member

felixxm commented Nov 13, 2023

@shangxiao Thanks 👍 I left two small suggestions.

nice I can squash things up and add Simon as co-author if you like

No need, squashed & rebased.

@felixxm felixxm merged commit b863c5f into django:main Nov 13, 2023
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charettes charettes charettes left review comments

@felixxm felixxm felixxm left review comments

@jacobtylerwalls jacobtylerwalls jacobtylerwalls left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@shangxiao @charettes @felixxm @jacobtylerwalls