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

Fixed bug in which DISTINCT clauses were forcibly removed from supplied querysets #76

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

bernd-wechner
Copy link

@bernd-wechner bernd-wechner commented Jan 28, 2019

I was supplying a queryset which was already distcint! And moreover I am using .distinct(field) in order to make a complex interaction between url filters and Window functions work. Works a dream in fact except that url_filter was clobbering the necessary DISTINCT ON clause in my query.

This is where it happened and it was because it paid no heed to the existing status of the query's distinct() setting. It need only apply distinct() if the caller has not already applied one (not least as called knows what fields they need to distinguish on.

…ed querysets

I was supplyin a queryset which was already distcint! And moreover I am using .distinct(field) in order to make a complex inetraction between url filters and Window functions work. Works a dream in fact except that url_filter was clobberig the necessary DISTINCT ON clause in my query. 

THis is where it happened and it was because it paid no heed to the existing status of the querie's distinct() setting. It neeed only apply distinct() if the caller has anot already applied on (not least as called knows what fields they need to distinguish on.
Simple spelling mistake. It's quiet not quite. Well a comix might say it's quite quiet ;-).
miki725 and others added 2 commits January 28, 2019 16:59
Sounds good. Thanks for the suggestion! Am on a learning curve here. And of course the only goal is that the stated one, that an existing DISTINCT clause be honored and not clobbered.

Co-Authored-By: bernd-wechner <[email protected]>
@bernd-wechner
Copy link
Author

I'm curious what the Travis CI failures are about? This is a two line change (much improved with your well informed insight (actually works now ;-), and I can't imagine that triggering any failures. Similarly I forked, cloned, ran make install and all dependencies are installed yet make test-all fails in a few places.

Can we see this two liner in a release some time (so I can rely on my production environment seeing it, right now it works on my dev box ...).

@bernd-wechner
Copy link
Author

I should add while I'm here, that using .distinct() was clear error on my part. Not sure what I was thinking. The way I learn Django and other things as quickly as I can, is to complement my reading with object inspection. And so I paused my the code on that line, and examined the queryset looking down its properties. I swear I saw one called distinct with a value of True. So I went with .distinct. Memory is not a reliable beast ant it was late last night and was tired, but if it serves me, that crashed with some exception that suggested to me it was a method, so I just put () on the end and it all rolled. Of course queryset.distinct() if I wasn't half asleep at the time trying to close this issue off (finding this fixed a big problem for me of sorts) I would have noticed that that is a queryset and always returns true if there are any returned objects, doh! And has nothing to do with what I wanted, which was to test if the queryset already had a DISTINCT clause so to speak.

@bernd-wechner
Copy link
Author

I should add I've worked around this, but I do wonder if this fixed fix will make into a release. Very quiet here no the front, given I have ni idea what All checks have failed by Travis means.

@miki725
Copy link
Owner

miki725 commented Feb 10, 2019

Fix looks good. Waiting for the ci to pass. Check Travis logs for exact error.

@bernd-wechner
Copy link
Author

How?

@simone6021
Copy link

Hello @bernd-wechner, this would be a nice enhancement, could you please fix the travis-ci issue?
make: *** No rule to make target 'install-quite'. Stop. ERROR: InvocationError for command '/usr/bin/make install-quite' (exited with code 2)

@bernd-wechner
Copy link
Author

Wow, my apologies, I sure dropped the ball on this one. Been months. I'm wonder how best to fix this, for lack of experience. It looks related to a simple tweak I made as an install-quiet command was misspelled install-quite in the Makefile. Shall I simply revert back to the misspelling? Or is there some way to inform Travis of the intent?

@simone6021
Copy link

simone6021 commented Feb 26, 2021

Hello @bernd-wechner, i think you should rebase onto master because that section of the Makefile was removed in latest version.

@bernd-wechner
Copy link
Author

OK think I succeeded, somehow. git rebase is candidate for most confusing thing in the known universe of course. But the files changed on this PR now look clean. Waiting for checks to complete mind you. Hard to imagine this two line change will trigger any issues ;-)

@simone6021
Copy link

Unfortunately, i think CI/CD server is not available at the moment.
@miki725 can you please check it out?

@bernd-wechner
Copy link
Author

Yes, it's been overnight and the check is still "Expected — Waiting for status to be reported", so it does appear to be down.

@bernd-wechner
Copy link
Author

Still down ... what gives?

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