Batch INSERT statements in executemany to reduce round trips#605
Batch INSERT statements in executemany to reduce round trips#605srchilukoori wants to merge 2 commits into
Conversation
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Sadha Chilukoori.
|
35385b1 to
92069b8
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
92069b8 to
c8a8722
Compare
|
@hashhar need your review on this PR |
hashhar
left a comment
There was a problem hiding this comment.
Excellent feature and thanks for the tests.
I need to think a bit about if there are cases where this may do the wrong thing and hence should require an opt-in behaviour for a few releases maybe.
c8a8722 to
e189b92
Compare
@hashhar I added it as opt-in, with a default of false, as suggested. I'll also put together a plan for the edge cases. |
Add opt-in experimental_batch_executemany connection parameter that batches INSERT...VALUES in executemany() into multi-row statements (100 rows per request) instead of one HTTP round trip per row. Default is False to preserve existing behavior. Batching changes partial failure semantics and rowcount reporting, so it is opt-in for now.
e189b92 to
6f0c86a
Compare
|
@hashhar please review the updated PR |
| re.IGNORECASE | re.DOTALL, | ||
| ) | ||
|
|
||
| _EXECUTEMANY_BATCH_SIZE = 100 |
There was a problem hiding this comment.
this needs to be configurable. The actual limit is the query text size configured on the server. Max on the server is 16MB IIRC but default is lower.
| logger = trino.logging.get_logger(__name__) | ||
|
|
||
| _INSERT_VALUES_RE = re.compile( | ||
| r"\A(\s*INSERT\s+INTO\s+.+\bVALUES)\s*\([^)]+\)\s*\Z", |
There was a problem hiding this comment.
seems to match all parens grouped things not just things with placeholders? This is only safe to do if ONLY placeholders are present in the VALUES.
There was a problem hiding this comment.
Also tests for the false positives we want to avoid (e.g. placeholder + literal) are missing which would've caught this
| formatted = ", ".join(self._format_prepared_param(p) for p in params) | ||
| value_rows.append("(%s)" % formatted) | ||
| sql = "%s %s" % (prefix, ", ".join(value_rows)) | ||
| self._query = trino.client.TrinoQuery( |
There was a problem hiding this comment.
this bypasses the SegmentCursor.
| self._transaction = None | ||
| self.legacy_primitive_types = legacy_primitive_types | ||
| self.legacy_prepared_statements = legacy_prepared_statements | ||
| self.experimental_batch_executemany = experimental_batch_executemany |
There was a problem hiding this comment.
also update sqlalchemy _url()? Also add test for sqlalchemy.
| legacy_primitive_types=self._legacy_primitive_types) | ||
| self._iterator = iter(self._query.execute()) | ||
| if self._query.update_type is None: | ||
| raise NotSupportedError("Query must return update type") |
| finally: | ||
| trino.client.TrinoQuery = original | ||
|
|
||
| def test_connection_threads_flag_to_cursor(self): |
There was a problem hiding this comment.
weird test since we assign value ourself and then we assert. Should it not instead be testing by creating the connection with kwargs and verifying connection state?
|
|
||
| match = _INSERT_VALUES_RE.match(operation.strip()) | ||
| if match and self._experimental_batch_executemany: | ||
| return self._executemany_batch_insert(match.group(1), seq_of_params) |
There was a problem hiding this comment.
shouldn't the rowcount be accumulated across all batches and returned? this just returns value from last batch?
Description
Fixes #602.
Cursor.executemany()executes INSERT...VALUES one row at a time.For 1,000 rows that means 1,000 HTTP round trips to Trino.
This adds a regex check for INSERT...VALUES patterns and batches
rows into multi-row VALUES clauses, 100 per request. Non-INSERT
statements still use the existing row-by-row path. Parameters are
formatted with the existing
_format_prepared_param()method.Non-technical explanation
Bulk inserts via SQLAlchemy
text()orcursor.executemany()nowsend fewer queries to Trino instead of one per row.
Release notes
( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text: