Definition of sqlalchemy functions_ext overrides desired behaviour

[byronrthomas]

Hi,

Thanks very much for the helpful library. It got me from zero-to-one with OData very quickly which I really appreciated.

However, I was a bit concerned about some of the warnings I got after using it for parsing OData to core SQLAlchemy constructs, as it seems to override behaviour in a manner that I don't think is correct in all cases. I will explain more below.

For reference the warning that I had a concern over was:

SAWarning: Class lower will not make use of SQL compilation caching as it does not set the 'inherit_cache' attribute to `True`.  This can have significant performance implications including some performance degradations in comparison to prior SQLAlchemy versions.  Set this attribute to True if this object can make use of the cache key generated by the superclass.  Alternatively, this attribute may be set to False which will disable this warning. (Background on this error at: https://sqlalche.me/e/20/cprf)
    conn.execute(qry)

This confused me as after looking into the linked article, it implied that I would be defining the class representing the lower SQL function somewhere, whereas in fact I wasn't. I was however using the function against my PostgresQL database, where I make heavy use of range types, especially datetime ranges.

I then found that in functions_ext you define a lower function handler. Further documentation and code experimentation confirms that this causes it to be hooked into the attribute sqlalchemy.func.lower, which is what I am using to access the lower function I want to use in my non-OData relevant code. Whilst this doesn't currently cause a bug for me, it does scare me slightly as it feels like not the most future-proof solution.

Reading up on the documentation of GenericFunction - the class you subclass to create your lower it says "The primary use case for defining a .GenericFunction class is so that a function of a particular name may be given a fixed return type." But your function clashes with the base function in this sense. In the context that I am using it, lower will return a DateTime.

So I was wondering whether we could:

1. Use the package attribute such that your definitions of these functions wouldn't clash with the sqlalchemy.func.xxx versions as yours would get hooked as sqlalchemy.func.odata.lower leaving sqlalchemy.func.lower for other users. This shouldn't affect the odata code as it just uses the classes representing the functions directly
2. Also set the inheritCache attribute so that even code that uses the OData versions of the functions avoids this warning, and also allows sqlalchemy to cache compilation results of the compiling the functions - which I believe should be safe

I will raise a PR with these suggestions and link it to this ticket.

Thanks,

Byron

[OliverHofkens]

Hey, thanks for the kind words, this very thorough investigation, and the included PR! Really appreciate it!

Your changes seem very sensible, and the test suite passes, fantastic 👌
I'll merge them and create a new release.

Thank you very much!

[OliverHofkens]

Version 0.9.0 should be up, including your PR 🚀