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

Implement filtering options for Number types. #385

Closed
Tracked by #248
kgodey opened this issue Jul 15, 2021 · 10 comments · Fixed by #1069
Closed
Tracked by #248

Implement filtering options for Number types. #385

kgodey opened this issue Jul 15, 2021 · 10 comments · Fixed by #1069
Assignees
Labels
type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL

Comments

@kgodey
Copy link
Contributor

kgodey commented Jul 15, 2021

Problem

We need to ensure that records that include columns of all Number types support the following filters via API:

  • between {x} and {y}
  • equals {x}
  • does not equal {x}
  • greater than {x}
  • lesser than {x}
  • greater than or equal to {x}
  • less than or equal to {x}
  • is empty
  • is not empty

This involves:

  • Implementing the filters in the backend
  • Updating the /api/v0/databases/<id>/types/ endpoint to store available filters on this type
    • Filter information should include the number of parameters needing to be passed in (e.g. between needs 2 parameters, is empty needs 0)

Additional context

@kgodey kgodey added this to the 07. Initial Data Types milestone Jul 15, 2021
@kgodey kgodey added needs: unblocking Blocked by other work type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL work: database labels Jul 15, 2021
@kgodey kgodey changed the title Implement filtering options for numeric types. Implement filtering options for Number types. Jul 15, 2021
@mathemancer
Copy link
Contributor

I suggest making sure we use inclusive BETWEEN (i.e., x BETWEEN y AND Z should be equivalent to x >= y AND x <= z rather than x > y AND x < z). This will make things consistent with the BETWEEN operator in PostgreSQL.

Do we want to use symmetric between, or not? I.e., should 5 BETWEEN 8 AND 2 be true, or false? (for details about symmetric between, see: https://www.postgresql.org/docs/13/functions-comparison.html).

@kgodey
Copy link
Contributor Author

kgodey commented Jul 16, 2021

I agree with using inclusive BETWEEN. I think using symmetric between also makes sense.

@dmos62
Copy link
Contributor

dmos62 commented Nov 16, 2021

Since "between" is ambiguous about its symmetry and inclusiveness, maybe instead let's have the user create/compose two simpler filters where these properties are explicit: a >= x AND a <= y instead of a BETWEEN x AND y. I doubt there's a performance concern, but if there were we could use pattern matching to optimize the filter behind the scenes. Seems desirable to not have the user wonder what "between" means.

@mathemancer
Copy link
Contributor

I definitely think we should have the former, but I'm not convinced that should be in lieu of the latter. Maybe initially, though. @kgodey I note that sqlalchemy-filters doesn't include a between operator, so we'd have to write one custom for that. I guess I'm ambivalent w.r.t. whether BETWEEN should be prioritized at the moment.

@kgodey
Copy link
Contributor Author

kgodey commented Nov 16, 2021

I'm fine with skipping between.

@kgodey kgodey added status: started and removed ready Ready for implementation labels Nov 16, 2021
@dmos62
Copy link
Contributor

dmos62 commented Nov 18, 2021

I'm starting to draft a design. The idea at the moment is to formalize the filters on the db namespace using dataclasses. In some ways this will be a test bed for #823.

@dmos62
Copy link
Contributor

dmos62 commented Nov 18, 2021

Actually, since the filtering will target Mathesar types (correct?), which the db namespace is oblivious of, and which are defined in mathesar.database, filters should probably be defined there too.

@kgodey
Copy link
Contributor Author

kgodey commented Nov 18, 2021

Filtering should be for DB types. Mathesar types are only used to group DB types, they shouldn't have other functionality applied to them.

@dmos62
Copy link
Contributor

dmos62 commented Nov 18, 2021

Filtering should be for DB types. Mathesar types are only used to group DB types, they shouldn't have other functionality applied to them.

Oh, what led me astray was that the Boolean filters are defined on Mathesar Types. It's not a big deal, I was going to rewrite them anyway.

@kgodey
Copy link
Contributor Author

kgodey commented Nov 18, 2021

Yes, please rewrite them to fit, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL
Projects
No open projects
4 participants