Skip to content
This repository was archived by the owner on Mar 2, 2026. It is now read-only.

feat: add IS_NOT_NULL operator to filters - #971

Closed
mgraczyk wants to merge 1 commit into
googleapis:mainfrom
Quilt-AI:main
Closed

feat: add IS_NOT_NULL operator to filters#971
mgraczyk wants to merge 1 commit into
googleapis:mainfrom
Quilt-AI:main

Conversation

@mgraczyk

Copy link
Copy Markdown
Contributor

This allows filters like x.where("field", "!=", None) to work, matching the javascript implementation.

Also adds a test.

Fixes#970

@mgraczyk
mgraczyk requested review from a teamSeptember 20, 2024 18:11
@product-auto-labelproduct-auto-labelBot added size: m Pull request size is medium. api: firestore Issues related to the googleapis/python-firestore API. labels Sep 20, 2024
@mgraczyk

Copy link
Copy Markdown
ContributorAuthor

In addition to the unit test, I tested this against my production firestore database and it returns the expected results.

@mgraczykmgraczyk changed the title Implement IS_NOT_NULL operator for filtersfeat: add IS_NOT_NULL operator to filtersSep 20, 2024
@mgraczyk

Copy link
Copy Markdown
ContributorAuthor

Hi @daniel-sanche, would you mind taking a look or running the workflows? Should be a nice addition and bring the python sdk closer to feature parity with the Node sdk.

@vazome

vazome commented Nov 28, 2024

Copy link
Copy Markdown

+1, I thought I'm tripping since I could do that in the console, but it's just GUI having more features than Python SDK, hehe.. heeeeeeee... after 4 years https://firebase.google.com/support/release-notes/js#version_7210_-_september_17_2020

@qaz10102030

Copy link
Copy Markdown

I hope this PR can be accepted as soon as possible. 🙏

@vazome

Copy link
Copy Markdown

Hey @daniel-sanche! Could you take a look?

@daniel-sanche

daniel-sanche commented Dec 19, 2024

Copy link
Copy Markdown
Contributor

This looks like a good addition, I will just have to check with other languages to make sure everything is consistent. I will also need to add some system tests before we can merge

Thanks for putting this together

@mgraczyk

Copy link
Copy Markdown
ContributorAuthor

Just ran into this again today, would love if we could land this!

image

I'd be happy to write system tests are required to expedite

@mgraczyk

Copy link
Copy Markdown
ContributorAuthor

Related but somewhat off topic, it would also be nice if we could refine the argument types for firestore.FieldFilter to more fail at type-checking time whenever the operands are known to be unsupported.

Even after my change there are gotchas passing None for comparison operators, and for transform sentinels like ArrayUnion and SERVER_TIMESTAMP

@product-auto-labelproduct-auto-labelBot added size: u Pull request is empty. and removed size: m Pull request size is medium. labels Dec 19, 2024
@mgraczyk

Copy link
Copy Markdown
ContributorAuthor

@daniel-sanche I rebased on top of main

@mgraczykmgraczyk reopened this Dec 19, 2024
@product-auto-labelproduct-auto-labelBot added size: m Pull request size is medium. and removed size: u Pull request is empty. labels Dec 19, 2024
@daniel-sanche

Copy link
Copy Markdown
Contributor

I'd be happy to write system tests are required to expedite

Unfortunately we are in a release freeze for the holidays, so we won't be able to cut a new release for this until early January. I will try to add some tests today

Thanks for your contributions, sorry about the delay in reviewing this!

@daniel-sanche

Copy link
Copy Markdown
Contributor

There's a new security policy that our CI checks won't run against external forks, so it looks like I'll have to close this in favor of #988. We can continue discussion there. (I also added support for IS_NOT_NAN, which was also missing)

If you have any more contributions, I can pull them in manually from your fork, but I think we should be able to merge and release this when the freeze is over. Thanks again for putting this together!

Related but somewhat off topic, it would also be nice if we could refine the argument types for firestore.FieldFilter to more fail at type-checking time whenever the operands are known to be unsupported.

Even after my change there are gotchas passing None for comparison operators, and for transform sentinels like ArrayUnion and SERVER_TIMESTAMP

Feel free to open bugs for this, and let me know if you have a particular solution in mind. I'll take a look at this again in January

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

api: firestoreIssues related to the googleapis/python-firestore API.size: mPull request size is medium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement IS_NOT_NULL

5 participants

@mgraczyk@vazome@qaz10102030@daniel-sanche@dconeybe