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

Add or-filtering to smartquery by passing in a list of dicts #75

Closed
wants to merge 1 commit into from
Closed

Conversation

nico525
Copy link

@nico525 nico525 commented Jun 6, 2021

This PR adds additional or filtering to the smart query mixing by passing in a list of dicts.
No breaking change as the input parameter of filter is checked and potentially converted

@michaelbukachi
Copy link
Collaborator

Hi @nico525, thanks for the PR. Currently, there's another PR (#63 ) which is trying to handler or/and operators. We trying to see what's the best approach to go with before merging any PR. There's an open issue (#47) where we can discuss it further.

@nico525
Copy link
Author

nico525 commented Jun 6, 2021

I've seen that but also I think that this provides an easier user handling by treating list items as or condition and dicts as separate and clauses.
I am using the code of this PR since 6 months now and due to the sqlalchemy 1.4 upgrade I thought about making it public.

@fredludlow
Copy link
Contributor

Thanks for this @nico525

Am I right that this only allows a single level of nesting and it has to be AND on the inside and OR on the outside:

(clause1a AND clause1b AND ...) OR (clause2a AND clause2b AND... ) OR ...

My intention with #63 was that it allows arbitrary nesting, e.g.

  1. (A AND B) OR (C AND D)
  2. (A OR B) AND (C OR D)
  3. (A OR (B AND C))
  4. (A OR (B AND C) OR ((D OR E) AND (F OR G)))
    etc. (though I hope I don't often hit something like case 4....)

If we make a list always result in an OR, it makes it harder to express:

  1. col_A like '%foo%' AND col_A like '%bar%'

dicts can't repeat keys, so you need e.g. a list or tuple to handle this (so if we say list === OR then we need to introduce another way to achieve 5.).

I'm not necessarily saying #63 is the right way to do this, and I agree the syntax isn't as clean as the above, but I think it's a lot more general. If we can come up with a syntax for #63 that's as clean as the one here but with the full generality (arbitrary depth, repeated keys) we'd have a winner :)

@nico525
Copy link
Author

nico525 commented Jun 9, 2021

Yes @fredludlow I am currently missing the recursion which could be easily added. It just wasn't a use case of mine so far 👍

Nevertheless you are right, my approach doesn't allow filtering on the same column with different attributes on the same filter. For me, I use the mixing for providing dynamic filters through a URL for a RestAPI and in that case I am fearing that handling complex objects such as yours would overcomplicate using this mixin by a lot - at least for my use case.

I'll leave it up to the owner to decide on what to use.

@michaelbukachi
Copy link
Collaborator

I agree with @fredludlow. Having a list represent an operator might not really be a good idea. Especially during complex conditions. #63 seems to be the most solid solution at the moment. If nothing better comes up before the weekend, I'll merge it and create a release.

This pull request was closed.
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