Add support for SQLAlchemy 1.4 #391

amotl · 2020-11-16T14:17:33Z

Summary of the changes / Why this is an improvement

~~SQLAlchemy 1.4.0 is about to be released.~~ SQLAlchemy 1.4.37 has been released. People are eagerly waiting for corresponding support.

amotl · 2020-11-16T14:22:27Z

SQLAlchemy 1.4 dropped support for Python 3.5, see sqlalchemy/sqlalchemy@d0b5ce2. However, Python 2.7 is still supported.

amotl · 2020-11-16T14:57:14Z

Observations

Tests fail with:

      File "/home/runner/work/crate-python/crate-python/eggs/SQLAlchemy-1.4.0b1-py3.8-linux-x86_64.egg/sqlalchemy/sql/compiler.py", line 508, in process
        return obj._compiler_dispatch(self, **kwargs)
      File "<string>", line 9, in _compiler_dispatch
      File "/home/runner/work/crate-python/crate-python/src/crate/client/sqlalchemy/compiler.py", line 201, in visit_insert
        crud_params = crud._get_crud_params(self, insert_stmt, **kw)
    TypeError: _get_crud_params() missing 1 required positional argument: 'compile_state'

-- https://github.com/crate/crate-python/pull/391/checks?check_run_id=1406879590#step:5:139

Root cause

The signature of _get_crud_params() has been updated through sqlalchemy/sqlalchemy@851fb8f and sqlalchemy/sqlalchemy@693938d, which decouples compiler state from DML objects. This is probably related to

Many Core and ORM statement objects now perform much of their construction and validation in the compile phase

A major initiative in the 1.4 series is to approach the model of both Core SQL statements as well as the ORM Query to allow for an efficient, cacheable model of statement creation and compilation, where the compilation step would be cached, based on a cache key generated by the created statement object, which itself is newly created for each use.
SQL Compilation Caching

New in version 1.4: SQLAlchemy now has a transparent query caching system that substantially lowers the Python computational overhead involved in converting SQL statement constructs into SQL strings across both Core and ORM. See the introduction at Transparent SQL Compilation Caching added to All DQL, DML Statements in Core, ORM.

Background

Version 1.4 is taking on a different focus than other SQLAlchemy releases in that it is in many ways attempting to serve as a potential migration point for a more dramatic series of API changes currently planned for release 2.0 of SQLAlchemy.

-- What’s New in SQLAlchemy 1.4?

amotl · 2020-11-16T22:25:42Z

Dear @chaudum and @m-kharbat,

I've tried to apply some minor updates from SQLAlchemy 1.4 to our visit_insert() and visit_update() methods.

crate-python/src/crate/client/sqlalchemy/compiler.py

Lines 187 to 193 in 7e12a49

    
               def visit_insert(self, insert_stmt, asfrom=False, **kw): 
        
                   """ 
        
                   used to compile <sql.expression.Insert> expressions. 
        
                   this function wraps insert_from_select statements inside 
        
                    parentheses to be conform with earlier versions of CreateDB. 
        
                   """

crate-python/src/crate/client/sqlalchemy/compiler.py

Lines 285 to 289 in 7e12a49

    
               def visit_update(self, update_stmt, **kw): 
        
                   """ 
        
                   used to compile <sql.expression.Update> expressions 
        
                   Parts are taken from the SQLCompiler base class. 
        
                   """

However, after I didn't have quick success when making a few adjustments and after reading the comments above, I wanted to check back with you about the further plan. Regarding the conform with earlier versions of CreateDB comment: Maybe we don't need any amendments coming from these functions anymore and drop support for CrateDB 0.x releases?

Apart from that, I also found that some bits within these methods are apparently related to still support SQLAlchemy 1.0 and 1.1. Maybe we also want to revise this and drop respective support?

With kind regards,
Andreas.

amotl

39d0101, after some other minor adjustments, magically made the test suite succeed, by adjusting the parameter mangling to/from crate_before_execute.

Other than the inline comments below, some other items could/should be addressed before finalizing this patch. Please let me know about your suggestions and if you can spot other shortcomings.

Special attention should be given to the new SQL Compilation Caching feature [1,2]. The CrateDialect should supports_statement_cache = True, see [3]. Also, there is a new cache_ok attribute on custom types [4], which should probably be applied to all CrateDB SQLAlchemy types listed in crate.client.sqlalchemy.types. [5] might also be related here.

Currently, we observe all those warnings referenced at SA's identify what constructs are blocking caching from being enabled documentation:

SAWarning: Dialect crate:crate-python will not make use of SQL compilation caching as it does not set the supports_statement_cache attribute to True. This can have significant performance implications including some performance degradations in comparison to prior SQLAlchemy versions. Dialect maintainers should seek to set this attribute to True after appropriate development and testing for SQLAlchemy 1.4 caching support. Alternatively, this attribute may be set to False which will disable this warning. (Background on this error at: https://sqlalche.me/e/14/cprf)

SAWarning: UserDefinedType {_Craty(),_ObjectArray(),Geopoint(),Geoshape()} will not produce a cache key because the cache_ok attribute is not set 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 type object's state is safe to use in a cache key, or False to disable this warning. (Background on this error at: https://sqlalche.me/e/14/cprf)

SAWarning: {Class Any,Class Match} 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/14/cprf)
Go through the list of significant changes [6] thoroughly and summarize/compile relevant details regarding CrateDB into the changelog file.

Test Asynchronous I/O with asyncpg, see [7].

engine = create_async_engine(
  "postgresql+asyncpg://scott:tiger@localhost/test"
)

[1] https://docs.sqlalchemy.org/en/14/core/connections.html#sql-compilation-caching
[2] https://docs.sqlalchemy.org/en/14/faq/performance.html#faq-new-caching
[3] https://docs.sqlalchemy.org/en/14/core/connections.html#caching-for-third-party-dialects
[4] https://docs.sqlalchemy.org/en/14/core/custom_types.html
[5] https://docs.sqlalchemy.org/en/14/errors.html#caching-caveats
[6] https://docs.sqlalchemy.org/en/14/changelog/migration_14.html
[7] https://docs.sqlalchemy.org/en/14/orm/extensions/asyncio.html

CHANGES.txt

amotl · 2022-05-16T21:24:52Z

src/crate/client/sqlalchemy/compiler.py

@@ -74,7 +79,24 @@ def rewrite_update(clauseelement, multiparams, params):
 def crate_before_execute(conn, clauseelement, multiparams, params):


How to properly rewrite parameters for update statements

On the topic of using events for rewriting update statements, @zzzeek suggested at sqlalchemy/sqlalchemy#5915 (comment):

For the before_execute() thing, if you want to rewrite paramaters, a much better place for a dialect to do that is in dialect.do_execute() and dialect.do_executemany() (will work in 1.3 also). See for example psycopg2 do_executemany() that does a bunch of rewriting. https://github.com/sqlalchemy/sqlalchemy/blob/master/lib/sqlalchemy/dialects/postgresql/psycopg2.py#L905

Also at sqlalchemy/sqlalchemy#5915 (comment):

You should not have to use any events, that is, no DialectEvents or ConnectionEvents at all; the code you are writing is a dialect. You would implement simple methods do_execute() and do_executemany() as methods of your class CrateDialect right here: https://github.com/crate/crate-python/blob/master/src/crate/client/sqlalchemy/dialect.py#L160 ; these override the default methods that are on DefaultDialect. dialects shouldnt use events for these kinds of things, the dialect implements the actual way that these things happen without any events riding on them.

Thank you again!

I haven't followed Mike's suggestion yet, after I was able to make the patch work with 39d0101. We will probably have to decide whether to drop support for SQLAlchemy 1.1 and 1.2? What are your opinions about this?

At sqlalchemy/sqlalchemy#5915 (reply in thread), I shared further thoughts and asked for guidance how to improve that specific topic.

I haven't followed Mike's suggestion yet, after I was able to make the patch work with 39d0101. We will probably have to decide whether to drop support for SQLAlchemy 1.1 and 1.2? What are your opinions about this?

As Mike already pointed out, there are 1) not much users using these old version 2) if the do, they probably use an older version of our driver as well.
So +1 for dropping the support of SQLAlchemy < 1.3.

All right, let's do it. I would prefer to bring in corresponding cleanups and improvements on behalf of the next iteration, if you don't have any objections. Otherwise, please let me know.

#424 implemented the update to drop support for corresponding EOL software components, i.e. SQLAlchemy 1.1 and 1.2.

About the original topic in this thread, the migration to hook into ExecutionContext.pre_exec() for rewriting UPDATE statements instead of using engine events, see #425.

src/crate/client/sqlalchemy/dialect.py

src/crate/client/sqlalchemy/monkey.py

seut · 2022-05-30T09:03:31Z

src/crate/client/sqlalchemy/compiler.py

+    # The rewriting logic in `rewrite_update` and `visit_update` needs
+    # adjustments here in order to prevent `sqlalchemy.exc.CompileError:
+    # Unconsumed column names: characters_name, data['nested']`
+    """
+    if parameters and stmt_parameter_tuples:
+        check = (
+            set(parameters)
+            .intersection(_column_as_key(k) for k, v in stmt_parameter_tuples)
+            .difference(check_columns)
+        )
+        if check:
+            raise exc.CompileError(
+                "Unconsumed column names: %s"
+                % (", ".join("%s" % (c,) for c in check))
+            )
+    """


The comment does not really help me in understanding the issue like why are there unconsumed column names.
Is it still valid? If so can you please comment more about the why?

Thank you for asking. It looks like through the special rewriting logic of the CrateDB driver, this sanity check by SQLAlchemy croaks. I don't actually know the "why", but I tried to improve the inline comment with 57d5941 and also added a backlog item to investigate further, see also #425.

seut · 2022-05-30T09:04:42Z

src/crate/client/sqlalchemy/compiler.py

+    # special logic that only occurs for multi-table UPDATE
+    # statements


I think the code speaks for itself here, I don't think the comment adds much value

This comment is part of the code which was vendored from upstream SA14. I didn't want to introduce any deviances to code which was copied over verbatim.

In order to test cases when views are actually _present_, some would have to be created. This is out of scope for this patch. - https://github.com/sqlalchemy/sqlalchemy/wiki/Views - https://github.com/jklukas/sqlalchemy-views - https://stackoverflow.com/questions/9766940/how-to-create-an-sql-view-with-sqlalchemy

This patch follows the documentation at [1] and adds the attributes `supports_statement_cache`, `cache_ok` and `inherit_cache` to the corresponding classes of `CrateDialect`, `UserDefinedType` and `ColumnElement`. The test suite thoroughly accepted this change of setting all attributes to `True`, effectively fully supporting SQL compilation caching. [1] https://docs.sqlalchemy.org/en/14/faq/performance.html

Beforehand, after observing errors like:: RuntimeError: number of values in row (1) differ from number of column processors (0) we decided to take the `resultproxy` out of the equation in order to work on the real gist of SQLAlchemy 1.4 compatibility. When looking at this again, it turned out that the root cause have been inaccuracies when setting up fake cursors within the test cases. SQLAlchemy <1.4 might have been more forgiving on that, but >=1.4 isn't.

SADeprecationWarning: The argument signature for the `ConnectionEvents.before_execute` event listener has changed as of version 1.4, and conversion for the old argument signature will be removed in a future release. The new signature is `def before_execute(conn, clauseelement, multiparams, params, execution_options)`.

lgtm-com · 2022-06-01T10:44:29Z

This pull request introduces 1 alert when merging 8a817fe into 67ec8e3 - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

seut

Looks good to me. Maybe wait for @mfussenegger to have a final look at it?

seut · 2022-06-01T11:52:51Z

lgtm.yml

+
+  # A module is imported with the "import" and "import from" statements.
+  # https://lgtm.com/rules/1818040193/
+  - exclude: py/import-and-import-from


does this work? #391 (comment)

No it doesn't ;[. At least, it does not work while not being merged. I hope that the additional rule will be recognized after the merge. However, it is weird because the other rule about py/not-named-self apparently seems to work well.

lgtm-com · 2022-06-01T12:28:51Z

This pull request introduces 1 alert when merging 00d016e into 67ec8e3 - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

mfussenegger

The low coverage + duplication of the visit_update_14 still doesn't feel right to me, but I guess it's good enough to move forward

mfussenegger · 2022-06-01T13:21:10Z

CHANGES.txt

+SQLAlchemy 1.4 became stricter on some details. Where it was ok to use a
+textual column expression in *plain text* beforehand, a
+`SQLAlchemy literal_column`_ type should be used now. This specifically
+applies to `CrateDB system columns`_ like ``_score``.
+
+For example, when a query might have looked like this beforehand::
+
+    session.query(Character.name, '_score')
+
+it should now be written like::
+
+    session.query(Character.name, sa.literal_column('_score'))
+


```suggestion SQLAlchemy 1.4 requires to wrap system columns like ``_score`` in a `SQLAlchemy literal_column`_ type. Before it was possible to use a query like this:: session.query(Character.name, '_score') It must now be written like:: session.query(Character.name, sa.literal_column('_score'))

The text before used should but it's not an optional thing, users must change this.

187a11d improves the wording, following your suggestion. Thank you again!

amotl · 2022-06-01T14:57:06Z

The low coverage + duplication of the visit_update_14 still doesn't feel right to me, but I guess it's good enough to move forward.

Thank you, I added your suggestion to crate/sqlalchemy-cratedb#74.

lgtm.com issued an alert like:: Module 'sqlalchemy' is imported with both 'import' and 'import from' See: https://lgtm.com/projects/g/crate/crate-python/rev/pr-8ea3b257a334a41cdb531d681072752e662eee56. The corresponding rule in lgtm.yml introduced by bcf9fff does not seem to work.

lgtm-com · 2022-06-01T15:06:37Z

This pull request introduces 1 alert when merging aad1458 into 67ec8e3 - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

amotl force-pushed the amo/sqlalchemy14 branch 3 times, most recently from f598944 to 8ba3b99 Compare November 16, 2020 14:38

amotl force-pushed the amo/sqlalchemy14 branch 3 times, most recently from 78f15ec to 0b1d8cb Compare November 16, 2020 21:12

This was referenced Jun 16, 2024

Support asyncpg with SQLAlchemy 1.4 crate/sqlalchemy-cratedb#80

Open

Upgrade Postgres driver orchestracities/ngsi-timeseries-api#407

Closed

amotl mentioned this pull request Dec 17, 2020

Behavior of SQL compiler's "visit_update" on SQLAlchemy 1.4 sqlalchemy/sqlalchemy#5779

Closed

amotl force-pushed the amo/sqlalchemy14 branch 2 times, most recently from 5d8a543 to 320d369 Compare December 18, 2020 00:27

amotl mentioned this pull request Feb 26, 2021

SqlAlchemy 1.3 Support for Apache Superset 1.0.1 crate/crate#11072

Closed

seut added the complexity: 8-13 label Mar 1, 2022

amotl force-pushed the amo/sqlalchemy14 branch 2 times, most recently from 66e3308 to 8726c13 Compare May 12, 2022 15:42

crate deleted a comment from lgtm-com bot May 12, 2022

amotl force-pushed the amo/sqlalchemy14 branch from 8726c13 to 0389cf5 Compare May 12, 2022 15:56

amotl mentioned this pull request May 12, 2022

Fix 406 Client Error: Not Acceptable errors in test suite because new kind of Fastly filtering/blocking #419

Merged

amotl force-pushed the amo/sqlalchemy14 branch from 60555eb to 5d87947 Compare May 13, 2022 09:57

amotl commented May 16, 2022

View reviewed changes

amotl force-pushed the amo/sqlalchemy14 branch from 5d87947 to 76129db Compare May 18, 2022 09:39

amotl mentioned this pull request May 18, 2022

Support SQLAlchemy dialect with psycopg2 crate/sqlalchemy-cratedb#100

Closed

crate deleted a comment from lgtm-com bot May 18, 2022

crate deleted a comment from lgtm-com bot May 25, 2022

seut reviewed May 30, 2022

View reviewed changes

amotl added 5 commits June 1, 2022 12:03

SA13: Suppress specific LGTM.com warning

e58a657

amotl force-pushed the amo/sqlalchemy14 branch from a80f3e7 to 57d5941 Compare June 1, 2022 10:20

amotl mentioned this pull request Jun 16, 2024

SQLAlchemy backlog crate/sqlalchemy-cratedb#74

Open

6 tasks

amotl requested a review from seut June 1, 2022 10:35

crate deleted a comment from lgtm-com bot Jun 1, 2022

seut approved these changes Jun 1, 2022

View reviewed changes

mfussenegger approved these changes Jun 1, 2022

View reviewed changes

amotl added 6 commits June 1, 2022 16:58

SA14: Final adjustments to the changelog

40cabe3

SA14: Add test fixture for testing VIEWs

abc6a6a

Documentation: Minor edit to fix link checker

35aa890

SA14: Improve inline documentation of CrateDB dialect compiler

14479e5

SA14/CI: Update to SQLAlchemy 1.4.37

d93dfce

amotl force-pushed the amo/sqlalchemy14 branch from 187a11d to aad1458 Compare June 1, 2022 14:58

amotl merged commit 3682da5 into master Jun 1, 2022

amotl deleted the amo/sqlalchemy14 branch June 1, 2022 15:38

amotl mentioned this pull request Jun 21, 2022

Possible regression with Python 3.10 on macOS >= 11; with reproduction repository actions/runner-images#5791

Closed

10 tasks

amotl mentioned this pull request Jan 27, 2023

SQLAlchemy: Investigate CompileError: Unconsumed column names crate/sqlalchemy-cratedb#78

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for SQLAlchemy 1.4 #391

Add support for SQLAlchemy 1.4 #391

amotl commented Nov 16, 2020 •

edited

Loading

amotl commented Nov 16, 2020 •

edited

Loading

amotl commented Nov 16, 2020 •

edited

Loading

amotl commented Nov 16, 2020

amotl left a comment •

edited

Loading

amotl May 16, 2022 •

edited

Loading

amotl May 16, 2022 •

edited

Loading

amotl May 19, 2022

seut May 20, 2022

amotl May 24, 2022 •

edited

Loading

amotl May 25, 2022 •

edited

Loading

amotl Jun 1, 2022

seut May 30, 2022

amotl Jun 1, 2022

seut May 30, 2022

amotl Jun 1, 2022 •

edited

Loading

lgtm-com bot commented Jun 1, 2022

seut left a comment

seut Jun 1, 2022

amotl Jun 1, 2022 •

edited

Loading

lgtm-com bot commented Jun 1, 2022

mfussenegger left a comment

mfussenegger Jun 1, 2022

amotl Jun 1, 2022

amotl commented Jun 1, 2022

lgtm-com bot commented Jun 1, 2022

		@@ -74,7 +79,24 @@ def rewrite_update(clauseelement, multiparams, params):
		def crate_before_execute(conn, clauseelement, multiparams, params):

		# special logic that only occurs for multi-table UPDATE
		# statements

Add support for SQLAlchemy 1.4 #391

Add support for SQLAlchemy 1.4 #391

Conversation

amotl commented Nov 16, 2020 • edited Loading

Summary of the changes / Why this is an improvement

amotl commented Nov 16, 2020 • edited Loading

amotl commented Nov 16, 2020 • edited Loading

Observations

Root cause

Background

amotl commented Nov 16, 2020

amotl left a comment • edited Loading

Choose a reason for hiding this comment

amotl May 16, 2022 • edited Loading

Choose a reason for hiding this comment

How to properly rewrite parameters for update statements

amotl May 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amotl May 24, 2022 • edited Loading

Choose a reason for hiding this comment

amotl May 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amotl Jun 1, 2022 • edited Loading

Choose a reason for hiding this comment

lgtm-com bot commented Jun 1, 2022

seut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amotl Jun 1, 2022 • edited Loading

Choose a reason for hiding this comment

lgtm-com bot commented Jun 1, 2022

mfussenegger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amotl commented Jun 1, 2022

lgtm-com bot commented Jun 1, 2022

amotl commented Nov 16, 2020 •

edited

Loading

amotl commented Nov 16, 2020 •

edited

Loading

amotl commented Nov 16, 2020 •

edited

Loading

amotl left a comment •

edited

Loading

amotl May 16, 2022 •

edited

Loading

amotl May 16, 2022 •

edited

Loading

amotl May 24, 2022 •

edited

Loading

amotl May 25, 2022 •

edited

Loading

amotl Jun 1, 2022 •

edited

Loading

amotl Jun 1, 2022 •

edited

Loading