Sqlalchemy: Session.bulk_insert_mappings needlessly iterates twice over given object list

[codingjoe]

Hi there 👋,

I stumbled upon a little inefficiency in the sqlalchemy.orm.session.Session.bulk_insert_mappings method.
https://github.com/sqlalchemy/sqlalchemy/blob/8c9537d37292459d348214fb8befa85d9cb64059/lib/sqlalchemy/orm/session.py#L2772-L2785

It generates a tuple, where a generator would be enough. This causes the method to have a complexity of O(2n) instead of just O(n).

Furthermore, the docstring might be misleading, since it states that the objects attribute should be a list where e Iterable would do.

I would suggest dropping to replace the tuple with a generator (just drop the tuple-prefix) and to adjust the docstring. Possibly in two separate commits.

Best
-Joe

[zzzeek]

we can't just change the docstring without adding test coverage that a non-sequence iterable is accepted and additionally this should be applied to all such methods on Session. do you have the resources to provide a pull request with tests? without test coverage the most I can do here is change "list" to "sequence".

[sqla-tester]

Mike Bayer has proposed a fix for this issue in the master branch:

Remove unnecessary tuple; prepare for "iterator" verbiage https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1731

[sqla-tester]

Mike Bayer has proposed a fix for this issue in the rel_1_3 branch:

Remove unnecessary tuple; prepare for "iterator" verbiage https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1732

[zzzeek]

fixed the tuple thanks for the heads up!