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

Sub queries for is_in and not_in #786

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

dantownsend
Copy link
Member

Resolves #785

@dantownsend dantownsend added this to In progress in Enhancements via automation Mar 6, 2023
@dantownsend dantownsend added the enhancement New feature or request label Mar 6, 2023
@sinisaos
Copy link
Member

@dantownsend Sorry to bother, but I wonder why you didn't merge this (I know the tests are missing) because I think it's really useful and convenient, and the performance is approx. 50% better on a GET request because we don't have to make two separate queries but just one?

@dantownsend
Copy link
Member Author

@sinisaos Yeah, it's a good point. Like you say, it just needs test and docs, and they shouldn't take too long to add. I'll try and finish it up and get it merged.

@sinisaos
Copy link
Member

@dantownsend Great. Thanks.

@codecov-commenter
Copy link

codecov-commenter commented Jun 22, 2023

Codecov Report

Merging #786 (4643993) into master (29f4c2f) will decrease coverage by 0.07%.
The diff coverage is 60.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #786      +/-   ##
==========================================
- Coverage   91.62%   91.56%   -0.07%     
==========================================
  Files         109      109              
  Lines        7885     7897      +12     
==========================================
+ Hits         7225     7231       +6     
- Misses        660      666       +6     
Impacted Files Coverage Δ
piccolo/columns/combination.py 90.90% <57.14%> (-2.57%) ⬇️
piccolo/columns/base.py 94.04% <62.50%> (-0.82%) ⬇️

@powellnorma
Copy link

Can we perhaps make it so that piccolo handles the case where we put in an empty list? It can be useful when the list is dynamically generated, and there are different conditions in the where clause. Maybe this would work?

@dantownsend
Copy link
Member Author

@powellnorma I usually have to add a check before any query with an is_in to make sure the list isn't empty. It has tripped me up a few times.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Enhancements
In progress
Development

Successfully merging this pull request may close these issues.

Sub queries for is_in and not_in clause
4 participants