From: Mike Bayer Date: Thu, 1 Dec 2022 21:03:58 +0000 (-0500) Subject: disable setinputsizes only for true DBAPI executemany X-Git-Tag: rel_2_0_0b4~17^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e3b905be21f392b17817decff5e1e76f02ccad03;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git disable setinputsizes only for true DBAPI executemany Fixed regression caused by the combination of :ticket:`8177`, re-enable setinputsizes for SQL server unless fast_executemany + DBAPI executemany is used for a statement, along with :ticket:`6047`, implement "insertmanyvalues", which bypasses DBAPI executemany in place of a custom DBAPI execute for INSERT statements. setinputsizes would incorrectly not be used for a multiple parameter-set INSERT statement that used "insertmanyvalues" if fast_executemany were turned on, as the check would incorrectly assume this is a DBAPI executemany call. The "regression" would then be that the "insertmanyvalues" statement format is apparently slightly more sensitive to multiple rows that don't use the same types for each row, so in such a case setinputsizes is especially needed. The fix repairs the fast_executemany check so that it only disables setinputsizes if true DBAPI executemany is to be used. Fixes: #8917 Change-Id: I78895606a99848d4f92ecf38ded92dc5d6d48c6f --- diff --git a/doc/build/changelog/unreleased_20/8917.rst b/doc/build/changelog/unreleased_20/8917.rst new file mode 100644 index 0000000000..37bee0d6b9 --- /dev/null +++ b/doc/build/changelog/unreleased_20/8917.rst @@ -0,0 +1,18 @@ +.. change:: + :tags: bug, mssql + :tickets: 8917 + + Fixed regression caused by the combination of :ticket:`8177`, re-enable + setinputsizes for SQL server unless fast_executemany + DBAPI executemany is + used for a statement, along with :ticket:`6047`, implement + "insertmanyvalues", which bypasses DBAPI executemany in place of a custom + DBAPI execute for INSERT statements. setinputsizes would incorrectly not be + used for a multiple parameter-set INSERT statement that used + "insertmanyvalues" if fast_executemany were turned on, as the check would + incorrectly assume this is a DBAPI executemany call. The "regression" + would then be that the "insertmanyvalues" statement format is apparently + slightly more sensitive to multiple rows that don't use the same types + for each row, so in such a case setinputsizes is especially needed. + + The fix repairs the fast_executemany check so that it only disables + setinputsizes if true DBAPI executemany is to be used. diff --git a/lib/sqlalchemy/connectors/pyodbc.py b/lib/sqlalchemy/connectors/pyodbc.py index f920860cd8..e8fbed18eb 100644 --- a/lib/sqlalchemy/connectors/pyodbc.py +++ b/lib/sqlalchemy/connectors/pyodbc.py @@ -209,7 +209,10 @@ class PyODBCConnector(Connector): # omit the setinputsizes calls for .executemany() with # fast_executemany=True - if context.executemany and self.fast_executemany: + if ( + context.execute_style is interfaces.ExecuteStyle.EXECUTEMANY + and self.fast_executemany + ): return cursor.setinputsizes( diff --git a/test/dialect/mssql/test_engine.py b/test/dialect/mssql/test_engine.py index 6b895e3f1c..57c6274e72 100644 --- a/test/dialect/mssql/test_engine.py +++ b/test/dialect/mssql/test_engine.py @@ -419,69 +419,55 @@ class FastExecutemanyTest(fixtures.TestBase): conn.execute(t.insert(), {"id": 200, "data": "data_200"}) - @testing.fixture - def fe_engine(self, testing_engine): - def go(use_fastexecutemany, apply_setinputsizes_flag): - engine = testing_engine( - options={ - "fast_executemany": use_fastexecutemany, - "use_setinputsizes": apply_setinputsizes_flag, - } - ) - return engine - - return go - - @testing.combinations( - ( - "setinputsizeshook", - True, - ), - ( - "nosetinputsizeshook", - False, - ), - argnames="include_setinputsizes", - id_="ia", - ) - @testing.combinations( - ( - "setinputsizesflag", - True, - ), - ( - "nosetinputsizesflag", - False, - ), - argnames="apply_setinputsizes_flag", - id_="ia", - ) - @testing.combinations( - ( - "fastexecutemany", - True, - ), - ( - "nofastexecutemany", - False, - ), - argnames="use_fastexecutemany", - id_="ia", - ) - def test_insert_floats( + @testing.variation("add_event", [True, False]) + @testing.variation("setinputsizes", [True, False]) + @testing.variation("fastexecutemany", [True, False]) + @testing.variation("insertmanyvalues", [True, False]) + @testing.variation("broken_types", [True, False]) + def test_insert_typing( self, metadata, - fe_engine, - include_setinputsizes, - use_fastexecutemany, - apply_setinputsizes_flag, + testing_engine, + add_event, + fastexecutemany, + setinputsizes, + insertmanyvalues, + broken_types, ): + """tests for executemany + datatypes that are sensitive to + "setinputsizes" + + Issues tested here include: + + #6058 - turn off setinputsizes by default, since it breaks with + fast_executemany (version 1.4) + + #8177 - turn setinputsizes back **on** by default, just skip it only + for cursor.executemany() calls when fast_executemany is set; + otherwise use it. (version 2.0) + + #8917 - oops, we added "insertmanyvalues" but forgot to adjust the + check in #8177 above to accommodate for this, so + setinputsizes was getting turned off for "insertmanyvalues" + if fast_executemany was still set + + """ # changes for issue #8177 have eliminated all current expected # failures, but we'll leave this here in case we need it again - expect_failure = False + # (... four months pass ...) + # surprise! we need it again. woop! for #8917 + expect_failure = ( + broken_types and not setinputsizes and insertmanyvalues + ) - engine = fe_engine(use_fastexecutemany, apply_setinputsizes_flag) + engine = testing_engine( + options={ + "fast_executemany": fastexecutemany, + "use_setinputsizes": setinputsizes, + "use_insertmanyvalues": insertmanyvalues, + } + ) observations = Table( "Observations", @@ -489,6 +475,7 @@ class FastExecutemanyTest(fixtures.TestBase): Column("id", Integer, nullable=False, primary_key=True), Column("obs1", Numeric(19, 15), nullable=True), Column("obs2", Numeric(19, 15), nullable=True), + Column("obs3", String(10)), schema="test_schema", ) with engine.begin() as conn: @@ -499,20 +486,33 @@ class FastExecutemanyTest(fixtures.TestBase): "id": 1, "obs1": Decimal("60.1722066045792"), "obs2": Decimal("24.929289808227466"), + "obs3": "obs3", }, { "id": 2, "obs1": Decimal("60.16325715615476"), "obs2": Decimal("24.93886459535008"), + "obs3": 5 if broken_types else "obs3", }, { "id": 3, "obs1": Decimal("60.16445165123469"), "obs2": Decimal("24.949856300109516"), + "obs3": 7 if broken_types else "obs3", }, ] - if include_setinputsizes: + assert_records = [ + { + "id": rec["id"], + "obs1": rec["obs1"], + "obs2": rec["obs2"], + "obs3": str(rec["obs3"]), + } + for rec in records + ] + + if add_event: canary = mock.Mock() @event.listens_for(engine, "do_setinputsizes") @@ -543,16 +543,23 @@ class FastExecutemanyTest(fixtures.TestBase): ) .mappings() .all(), - records, + assert_records, ) - if include_setinputsizes: - if apply_setinputsizes_flag: + if add_event: + if setinputsizes: eq_( canary.mock_calls, [ # float for int? this seems wrong - mock.call([float, float, float]), + mock.call( + [ + float, + float, + float, + engine.dialect.dbapi.SQL_VARCHAR, + ] + ), mock.call([]), ], )