Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fixed #373 -- Added CompositePrimaryKey-based Meta.primary_key. #18056
base: main
Are you sure you want to change the base?
Fixed #373 -- Added CompositePrimaryKey-based Meta.primary_key. #18056
Changes from 13 commits
4be1c68
cee213d
e2d8868
259605a
9e01443
fa4af00
8354bd0
d20f8e0
dcd78fc
3576dec
1ef76e4
3d0dcf3
6a2bb92
a2ec10c
3071ad9
b7179b6
cb4f300
6922337
b7b9580
9b17831
6285cff
22d6c9d
ad51da4
9c9e57d
c8f796e
26fb669
87b1a23
389daa9
2515066
350c58d
220b3ed
b9a8143
141b27d
926822f
ac29218
0b194eb
b6727d2
21d48d5
b839b2b
006736c
3bf4004
90a8d43
7bc3f40
f4db656
e7deac6
e15f55d
79bf5e8
92f2807
c5c5239
8e408b6
69143d9
586434c
0478d46
23e0e0b
a9ef232
694fcc8
b49ea71
9fbb766
96156dd
13c0053
9ce6781
2f24f77
2c8d38c
cff46d5
39a2a09
2478e14
76191b5
208a457
b74f3ef
70fd4be
4caee01
272184d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there might be a way to define a
TupleLookupMixin
that can be mixed withExact
,In
,GreaterThan
, and others to do the right thing by default.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could remove the
TupleExact
class and handle the case of comparing tuples insideExact
, if that's what you're suggesting? It would be a bit more elegant I suppose.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I looked into it and I started implementing the other lookups. I don't think it's possible to define a mixin that does the right thing by default, but it's possible to make some abstractions to reduce duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gt
,gte
,lt
,lte
have been implemented. Also,TupleLookupMixin
has been added. Please look into it when you have some time. It's not exactly what you wanted, there's no default behavior as each lookup has their own complexity, but I think this is the only way to implement this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you explored overriding
resolve_expression
to buildQ
objects instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have never seen
Q
objects used inas_sql
, I don't think we're supposed to use them here, are we?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice to eventually use row level comparison instead for backends that support it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could work on this in the next PR, however, I'm not sure if there's any real benefit to it other than generating nicer SQL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there is an opportunity to merge this
TuplesIn
,Cols
, and friends logic withMultiColSource
so it's less of an 👽. They both do very similar thing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @charettes , I'll need to look into this, I wasn't aware.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I merged
Cols
withMultiColSource
(ad51da4) however, I'm not sure this is correct.As far as I understand,
MultiColSource
was meant to represent columns in a JOIN, and as such, it has asources
field.Cols
, on the other hand, was meant to represent a list of columns and it doesn't need asources
field. WDYT?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted back to
Cols
. Please resolve if you agree.