Add issubset, issuperset and size to Range type #563
Conversation
asyncpg/types.py
Outdated
| lowerOk = True | ||
| else: | ||
| if self._lower is None: | ||
| return False |
There was a problem hiding this comment.
It would read better if you set lowerOk = False here and use an elif. Also, please use lower_ok for the variable name for style consistency.
asyncpg/types.py
Outdated
| def issuperset(self, other): | ||
| return other.issubset(self) | ||
|
|
||
| def size(self, step=None): |
There was a problem hiding this comment.
I do not understand why the step argument is necessary. The size of the range is determined unambiguously by whether the upper and lower bounds are inclusive.
There was a problem hiding this comment.
The returned size for [1,2) and [1,2] should be different. In postgres they have a distinction between a discrete and continuous range. If this was a discrete integer range the results would be 2-1 = 1 and 2-1+1=2. +1 since the discrete step for an integer is 1. What is the step value for continuous range like a float? Or if using datetime what is your lowest time increment you care about.
There was a problem hiding this comment.
OK, I think the generic Range class shouldn't be implementing the size() method then. It's too dependent on the data type and, since Postgres allows user-defined range types, hardcoding it for int, float, and date types wouldn't save us.
There was a problem hiding this comment.
The distinction between "discrete" and "continuous" ranges has an impact also on issubset() and issuperset().
Consider the following two ranges: (1,2]::int4range and [2,3)::int4range. They represent the very same set of values, and indeed select '(1,2]'::int4range <@ '[2,3)'::int4range returns True, but
>>> r1 = asyncpg.Range(1, 2, lower_inc=False, upper_inc=True)
>>> r2 = asyncpg.Range(2, 3, lower_inc=True, upper_inc=False)
>>> r1.issubset(r2)
False
asyncpg/types.py
Outdated
|
|
||
| def size(self, step=None): | ||
| if self._upper is None or self._lower is None: | ||
| return None |
There was a problem hiding this comment.
It would be better to raise a ValueError here. size() on an open range is meaningless and akin to division by zero, returning None in this case risks masking bugs.
There was a problem hiding this comment.
As I understood it, a None in either lower/upper means infinity which matches the null/missing bound definition of postgres. Hence why I return the "infinity". I'm ok either way.
For numbers this could be changed from None to float("inf"), and for datetimes to datetime.datetime.min/max.
There was a problem hiding this comment.
Since postgres only has numeric and date/time ranges adding type check for datetime and numeric and returning values for each one and removing None completely might be worthwhile refactor.
It would also simply the logic in here since there is no longer a need for checking None.
asyncpg/types.py
Outdated
| else: | ||
| upperOk = self._upper < other._upper | ||
|
|
||
| return lowerOk and upperOk |
There was a problem hiding this comment.
lowerOk is always True by this point, so returning just upperOk is sufficient.
Hard to generalize. Based on specific usage
|
Looks good now. Please add some tests. |
|
Please fix flake8 issues, and let's merge. Thanks! |
Changes ------- * Drop support for Python 3.5 (#777) (by @and-semakin in da58cd2 for #777) * Add support for Python 3.10 (#795) (by @elprans in abf5569 for #795) * Add support for asynchronous iterables to copy_records_to_table() (#713) (by @elprans in 1d33ff6 for #713) * Add support for coroutine functions as listener callbacks (#802) (by @elprans in 41da093 for #802) * Add support for sslcert, sslkey and sslrootcert parameters to DSN (#768) (by @jdobes and @elprans in c674e86 for #768) * Add copy_ wrappers to Pool (#661) (by @elprans in a6b0f28 for #661) * Add issubset and issuperset methods to the Range type (#563) (by @kdorsel in de07d0a for #563) Fixes ----- * Break connection internal circular reference (#774) (by @fantix in d08a9b8 for #774) * Make Server Version Extraction More Flexible (#778) (by @Natrinicle in d076169 for #778)
Changes ------- * Drop support for Python 3.5 (#777) (by @and-semakin in da58cd2 for #777) * Add support for Python 3.10 (#795) (by @elprans in abf5569 for #795) * Add support for asynchronous iterables to copy_records_to_table() (#713) (by @elprans in 1d33ff6 for #713) * Add support for coroutine functions as listener callbacks (#802) (by @elprans in 41da093 for #802) * Add support for sslcert, sslkey and sslrootcert parameters to DSN (#768) (by @jdobes and @elprans in c674e86 for #768) * Add copy_ wrappers to Pool (#661) (by @elprans in a6b0f28 for #661) * Add issubset and issuperset methods to the Range type (#563) (by @kdorsel in de07d0a for #563) Fixes ----- * Break connection internal circular reference (#774) (by @fantix in d08a9b8 for #774) * Make Server Version Extraction More Flexible (#778) (by @Natrinicle in d076169 for #778)
Changes ------- * Drop support for Python 3.5 (#777) (by @and-semakin in da58cd2 for #777) * Add support for Python 3.10 (#795) (by @elprans in abf5569 for #795) * Add support for asynchronous iterables to copy_records_to_table() (#713) (by @elprans in 1d33ff6 for #713) * Add support for coroutine functions as listener callbacks (#802) (by @elprans in 41da093 for #802) * Add support for sslcert, sslkey and sslrootcert parameters to DSN (#768) (by @jdobes and @elprans in c674e86 for #768) * Add copy_ wrappers to Pool (#661) (by @elprans in a6b0f28 for #661) * Add issubset and issuperset methods to the Range type (#563) (by @kdorsel in de07d0a for #563) Fixes ----- * Break connection internal circular reference (#774) (by @fantix in d08a9b8 for #774) * Make Server Version Extraction More Flexible (#778) (by @Natrinicle in d076169 for #778)
Adding issubset, issuperset and size methods to the Range type.
This should correctly handle all corner cases with empty, lower/upper included or not for set methods. For empty sets follows the same rules as Python's super and sub set methods.
Size method takes in optional step parameter for when lower_inc == upper_inc since this will either add or remove 1 step size to the size of the range.
If this initial implementation is something worthwhile I can also add tests for it.
Closes #559