Reflections on the SA 2.0 new PG Range

[Lele Gaifax]

Hi!

I have finally completed the "port" of my application from SA 1.4 to SA
2.0b.

TL;DR I'm writing this report to ask an opinion about possible
"improvements" to the Range class.

Given that the previous state was already "clean" (that is, executing
the test suite with SQLALCHEMY_WARN_20=1 did not raise any warning), I
did not have big surprises (a loud applause here!).

The area that required lot of changes has been the introduction of the
new Range object: as said in previous emails this application uses
DATERANGE in many entities, and TSRANGE in a couple of cases.

The common modification has been replacing

from psycopg2.extras import DateRange

with

from sqlalchemy.dialects.postgresql import Range

and adapting the code accordingly, say from

validity = DateRange(now, then, '[]')

to

validity = Range(now, then, bounds='[]'')

most of that thru regexp search and replace and Emacs keyboard macros.
Not a big deal.

Where I had to spend more thinking has been in two differences, one
minor/cosmetic, the other more substantial.

On the former one, it would be nice if we could reduce the "distance"
between the new Range class and what popular drivers provide:

- "empty" vs "isempty": both asyncpg and psycopg have a "isempty"
property, we could either rename the "empty" attribute or add an
property alias

- bounds inclusiveness: both drivers expose that thru
"lower_inc"/"upper_inc" properties, while in SA we must say
`range.bounds[0] == '['`

- infinite bounds: likewise, they both provide "lower_inf"/"upper_inf"
properties, whereas we must say `range.lower is None and not
range.empty`

As you see, these are "syntactic sugar" differences, easily implemented
with some @properties.

What is more substantial, that required slightly more impacting changes
in a few cases, is that the SA Range does not have an exact notion of
its "type", i.e. there is no way to determine the data type of an empty
or infinite range.

For example, my app shows the period of validity of (say) a contract,
which storage is a daterange, either in a compact form (mm/dd/yy -
mm/dd/yy) or as a more verbose, human friendly message ("from Sun, 10
March 2022 to Fri, 15 March 2022"), in multiple languages, and taking
into account "infinite" variants. This was implemented with a generic
"strftime()" function taking either a date, a datetime, a DateRange or
DateTimeRange as argument, and depending on the kind it used different
gettext() messages to translate it to a string.

To emit different messages for "endless" date ranges and "endless"
datetime ranges, I had to modify the code path leading there, to bring
over the "original" kind of the range.

From a different perspective, we had to invent a way to distinguish the
different kind of ranges also in the recent improvements to the Range
class, to be able to determine a "discrete step" for each of them.

So I wonder if it would be reasonable/doable/affordable if SA could grow
a "family" of classes, derived from current Range, one for each data
type, so that for example a DATARANGE column would be associated with a
DateRange instance.

What do you think?

ciao, lele.
--
nickname: Lele Gaifax | Dire che Emacs è "conveniente" è come
real: Emanuele Gaifas | etichettare l'ossigeno come "utile"
le...@etour.tn.it | -- Rens Troost

[Mike Bayer]

great, we can add those.

What is more substantial, that required slightly more impacting changes

in a few cases, is that the SA Range does not have an exact notion of

its "type", i.e. there is no way to determine the data type of an empty

or infinite range.

For example, my app shows the period of validity of (say) a contract,

which storage is a daterange, either in a compact form (mm/dd/yy -

mm/dd/yy) or as a more verbose, human friendly message ("from Sun, 10

March 2022 to Fri, 15 March 2022"), in multiple languages, and taking

into account "infinite" variants. This was implemented with a generic

"strftime()" function taking either a date, a datetime, a DateRange or

DateTimeRange as argument, and depending on the kind it used different

gettext() messages to translate it to a string.

To emit different messages for "endless" date ranges and "endless"

datetime ranges, I had to modify the code path leading there, to bring

over the "original" kind of the range.

From a different perspective, we had to invent a way to distinguish the

different kind of ranges also in the recent improvements to the Range

class, to be able to determine a "discrete step" for each of them.

So I wonder if it would be reasonable/doable/affordable if SA could grow

a "family" of classes, derived from current Range, one for each data

type, so that for example a DATARANGE column would be associated with a

DateRange instance.

What do you think?

how about we add a "type" field to Range itself?    That would be an easier change and also remove the need for applications to deal with all kinds of different Range subclasses that mostly have identical behavior.

type-specific Range methods could be called off of each AbstractRange type subclass, such as how the AbstractRange._resolve_for_literal() method works.

The Range object itself should also become a typing.Generic so that we could pass around the type as Range[int] or Range[datetime], etc. 

I think that will be a good change and continue to emphasize Range as an abstract container type.

ciao, lele.

-- 

nickname: Lele Gaifax | Dire che Emacs è "conveniente" è come

real: Emanuele Gaifas | etichettare l'ossigeno come "utile"

le...@etour.tn.it      |                           -- Rens Troost

-- 

SQLAlchemy - 

The Python SQL Toolkit and Object Relational Mapper

http://www.sqlalchemy.org/

To post example code, please provide an MCVE: Minimal, Complete, and Verifiable Example.  See  http://stackoverflow.com/help/mcve for a full description.

--- 

You received this message because you are subscribed to the Google Groups "sqlalchemy" group.

To unsubscribe from this group and stop receiving emails from it, send an email to sqlalchemy+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/sqlalchemy/87pmd4lat5.fsf%40metapensiero.it.

[Lele Gaifax]

"Mike Bayer" <mik...@zzzcomputing.com> writes:

> On Wed, Nov 30, 2022, at 10:30 AM, Lele Gaifax wrote:
>> On the former one, it would be nice if we could reduce the "distance"
>> between the new Range class and what popular drivers provide:
>>
>> - "empty" vs "isempty": both asyncpg and psycopg have a "isempty"
>> property, we could either rename the "empty" attribute or add an
>> property alias
>>
>> - bounds inclusiveness: both drivers expose that thru
>> "lower_inc"/"upper_inc" properties, while in SA we must say
>> `range.bounds[0] == '['`
>>
>> - infinite bounds: likewise, they both provide "lower_inf"/"upper_inf"
>> properties, whereas we must say `range.lower is None and not
>> range.empty`
>>
>> As you see, these are "syntactic sugar" differences, easily implemented
>> with some @properties.
>
> great, we can add those.

Ok, I'll try to propose a issue/PR in the following days.

>>
>> What is more substantial, that required slightly more impacting changes
>> in a few cases, is that the SA Range does not have an exact notion of
>> its "type", i.e. there is no way to determine the data type of an empty
>> or infinite range.

>> ...

>> So I wonder if it would be reasonable/doable/affordable if SA could grow
>> a "family" of classes, derived from current Range, one for each data
>> type, so that for example a DATARANGE column would be associated with a
>> DateRange instance.

> how about we add a "type" field to Range itself? That would be an
> easier change and also remove the need for applications to deal with
> all kinds of different Range subclasses that mostly have identical
> behavior.

Yes, it could be done that way too, but from the developer perspective,
how is he going to create Range instances? Changing the constructor to
require a "type" argument seems uglier than using a specific class.

>
> type-specific Range methods could be called off of each AbstractRange
> type subclass, such as how the AbstractRange._resolve_for_literal()
> method works.

I will need to study that part.

>
> The Range object itself should also become a typing.Generic so that we
> could pass around the type as Range[int] or Range[datetime], etc.

Yes!

bye, lele.
--
nickname: Lele Gaifax | Quando vivrò di quello che ho pensato ieri
real: Emanuele Gaifas | comincerò ad aver paura di chi mi copia.
le...@metapensiero.it | -- Fortunato Depero, 1929.

[Mike Bayer]

well we could add functions to create Range objects with different types, or maybe class constructors like Range.new_daterange(x, y).    We already have a class hierarchy with AbstractRange, I dont want to make *two* class hierarchies, that increases complexity and additionally none of the DBAPIs have type-specific range objects.

>

> type-specific Range methods could be called off of each AbstractRange

> type subclass, such as how the AbstractRange._resolve_for_literal()

> method works.

I will need to study that part.

>

> The Range object itself should also become a typing.Generic so that we

> could pass around the type as Range[int] or Range[datetime], etc.

Yes!

bye, lele.

-- 

nickname: Lele Gaifax | Quando vivrò di quello che ho pensato ieri

real: Emanuele Gaifas | comincerò ad aver paura di chi mi copia.

le...@metapensiero.it  |                 -- Fortunato Depero, 1929.

-- 

SQLAlchemy - 

The Python SQL Toolkit and Object Relational Mapper

http://www.sqlalchemy.org/

To post example code, please provide an MCVE: Minimal, Complete, and Verifiable Example.  See  http://stackoverflow.com/help/mcve for a full description.

--- 

You received this message because you are subscribed to the Google Groups "sqlalchemy" group.

To unsubscribe from this group and stop receiving emails from it, send an email to sqlalchemy+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/sqlalchemy/878rjsfddq.fsf%40metapensiero.it.

[Lele Gaifax]

"Mike Bayer" <mik...@zzzcomputing.com> writes:

> On Wed, Nov 30, 2022, at 2:31 PM, Lele Gaifax wrote:
>> "Mike Bayer" <mik...@zzzcomputing.com> writes:
>>
>> > On Wed, Nov 30, 2022, at 10:30 AM, Lele Gaifax wrote:
>> >>
>> >> - "empty" vs "isempty": both asyncpg and psycopg have a "isempty"
>> >> property, we could either rename the "empty" attribute or add an
>> >> property alias
>> >

>> > great, we can add those.

Wrt "empty", what's your preference, leaving it as is and expose a
"isempty" property or rename the attribute to "isempty" (and possibly)
add a backward compat "empty" property?

>>
>> Yes, it could be done that way too, but from the developer perspective,
>> how is he going to create Range instances? Changing the constructor to
>> require a "type" argument seems uglier than using a specific class.
>
> well we could add functions to create Range objects with different
> types, or maybe class constructors like Range.new_daterange(x, y). We
> already have a class hierarchy with AbstractRange, I dont want to make
> *two* class hierarchies, that increases complexity and additionally
> none of the DBAPIs have type-specific range objects.

Agree, I'm slightly inclined toward class constructors (less symbols to
import here and there). Also, "new_date_range()" seems clearer...

Thanks&bye, lele.

[Mike Bayer]

On Thu, Dec 1, 2022, at 1:01 PM, Lele Gaifax wrote:

"Mike Bayer" <mik...@zzzcomputing.com> writes:

> On Wed, Nov 30, 2022, at 2:31 PM, Lele Gaifax wrote:

>> "Mike Bayer" <mik...@zzzcomputing.com> writes:

>> 

>> > On Wed, Nov 30, 2022, at 10:30 AM, Lele Gaifax wrote:

>> >> 

>> >> - "empty" vs "isempty": both asyncpg and psycopg have a "isempty"

>> >>   property, we could either rename the "empty" attribute or add an

>> >>   property alias

>> >

>> > great, we can add those.

Wrt "empty", what's your preference, leaving it as is and expose a

"isempty" property or rename the attribute to "isempty" (and possibly)

add a backward compat "empty" property?

well ".isempty" is not the kind of naming convention we like to use these days, so that should likely be an alias.  ".empty" might be named ".is_empty".   Did I get ".empty" from asyncpg's API ? 

>> 

>> Yes, it could be done that way too, but from the developer perspective,

>> how is he going to create Range instances? Changing the constructor to

>> require a "type" argument seems uglier than using a specific class.

>

> well we could add functions to create Range objects with different

> types, or maybe class constructors like Range.new_daterange(x, y). We

> already have a class hierarchy with AbstractRange, I dont want to make

> *two* class hierarchies, that increases complexity and additionally

> none of the DBAPIs have type-specific range objects.

Agree, I'm slightly inclined toward class constructors (less symbols to

import here and there). Also, "new_date_range()" seems clearer...

great!

Thanks&bye, lele.

-- 

nickname: Lele Gaifax | Quando vivrò di quello che ho pensato ieri

real: Emanuele Gaifas | comincerò ad aver paura di chi mi copia.

le...@metapensiero.it  |                 -- Fortunato Depero, 1929.

-- 

SQLAlchemy - 

The Python SQL Toolkit and Object Relational Mapper

http://www.sqlalchemy.org/

To post example code, please provide an MCVE: Minimal, Complete, and Verifiable Example.  See  http://stackoverflow.com/help/mcve for a full description.

--- 

You received this message because you are subscribed to the Google Groups "sqlalchemy" group.

To unsubscribe from this group and stop receiving emails from it, send an email to sqlalchemy+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/sqlalchemy/87h6yfover.fsf%40metapensiero.it.

[Lele Gaifax]

"Mike Bayer" <mik...@zzzcomputing.com> writes:

> On Thu, Dec 1, 2022, at 1:01 PM, Lele Gaifax wrote:
>> Wrt "empty", what's your preference, leaving it as is and expose a
>> "isempty" property or rename the attribute to "isempty" (and possibly)
>> add a backward compat "empty" property?
>
> well ".isempty" is not the kind of naming convention we like to use
> these days, so that should likely be an alias. ".empty" might be
> named ".is_empty". Did I get ".empty" from asyncpg's API ?

I agree with "is_empty", but both asyncpg:

https://github.com/MagicStack/asyncpg/blob/master/asyncpg/types.py#L85

and psycopg:

https://github.com/psycopg/psycopg/blob/master/psycopg/psycopg/types/range.py#L108

and psycopg2:

https://github.com/psycopg/psycopg2/blob/master/lib/_range.py#L89

call it "isempty", as a @property.

While asyncpg has an "internal" "_empty" attribute, psycopg chose to
set "_bounds" to None carry that information.

ciao, lele.

[Federico Caselli]

Postgresql does use isempty for the function that test if a range is empty 
https://www.postgresql.org/docs/current/functions-range.html#RANGE-OPERATORS-TABLE

[Lele Gaifax]

Federico Caselli <cfede...@gmail.com> writes:

> On Thursday, 1 December 2022 at 22:50:11 UTC+1 leleg...@gmail.com wrote:
>
>> "Mike Bayer" <mik...@zzzcomputing.com> writes:
>> >
>> > well ".isempty" is not the kind of naming convention we like to use
>> > these days, so that should likely be an alias. ".empty" might be
>> > named ".is_empty". Did I get ".empty" from asyncpg's API ?
>>

>> I agree with "is_empty", but both asyncpg [...] and psycopg [...] and
>> psycopg2 [...] call it "isempty", as a @property.

>>
> Postgresql does use isempty for the function that test if a range is empty
> https://www.postgresql.org/docs/current/functions-range.html#RANGE-OPERATORS-TABLE

It seems that, given we are aiming to reduce a (PG expert) dev's suprise
when using the class, "isempty" wins.

As I cannot really find a decisive advantage between renaming the
existing attribute vs providing a compatibility property over it, can
you help me please, even by tossing your own coin?

Facts: a property is surely easier, but OTOH that dual way will have to
stay there forever. Renaming is not a big deal, there are actually just
a dozen places in the source tree where it used:

$ git grep empty=
lib/sqlalchemy/dialects/postgresql/asyncpg.py: empty=value.empty,
lib/sqlalchemy/dialects/postgresql/asyncpg.py: empty=empty,
lib/sqlalchemy/dialects/postgresql/asyncpg.py: empty=value.empty,
lib/sqlalchemy/dialects/postgresql/asyncpg.py: empty=empty,
lib/sqlalchemy/dialects/postgresql/psycopg.py: empty=not value._bounds,
lib/sqlalchemy/dialects/postgresql/psycopg.py: empty=not elem._bounds,
lib/sqlalchemy/dialects/postgresql/psycopg2.py: empty=not value._bounds,
lib/sqlalchemy/dialects/postgresql/ranges.py: self, lower=None, upper=None, *, bounds="[)", empty=False
lib/sqlalchemy/dialects/postgresql/ranges.py: return Range(None, None, empty=True)
lib/sqlalchemy/dialects/postgresql/ranges.py: return Range(None, None, empty=True)
lib/sqlalchemy/dialects/postgresql/ranges.py: return Range(None, None, empty=True)
test/dialect/postgresql/test_types.py: lambda **kw: Range(empty=True),
test/dialect/postgresql/test_types.py: lambda l, r: Range(empty=True),
test/dialect/postgresql/test_types.py: lambda r, e: Range(empty=True),
test/dialect/postgresql/test_types.py: is_false(bool(Range(empty=True)))
test/dialect/postgresql/test_types.py: eq_(data, [(self._data_obj().__class__(empty=True),)])
test/dialect/postgresql/test_types.py: eq_(data, [(self._data_obj().__class__(empty=True),)])

thanks&bye, lele.

[Mike Bayer]

there should definitely be an "isempty" accessor, but from SQLA's naming conventions we prefer to have "is_empty" as the official name.   There's a bunch of precedent for this.  overall people should be able to use either one.

-- 

SQLAlchemy - 

The Python SQL Toolkit and Object Relational Mapper

http://www.sqlalchemy.org/

To post example code, please provide an MCVE: Minimal, Complete, and Verifiable Example.  See  http://stackoverflow.com/help/mcve for a full description.

--- 

You received this message because you are subscribed to the Google Groups "sqlalchemy" group.

To unsubscribe from this group and stop receiving emails from it, send an email to sqlalchemy+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/sqlalchemy/871qpfqeu8.fsf%40metapensiero.it.

[Lele Gaifax]

"Mike Bayer" <mik...@zzzcomputing.com> writes:

> there should definitely be an "isempty" accessor, but from SQLA's
> naming conventions we prefer to have "is_empty" as the official name.
> There's a bunch of precedent for this. overall people should be able
> to use either one.

Great, thank you.