Add BulkResponse wrapper for improved decoding of HTTP bulk responses

[amotl]

About

CrateDB HTTP bulk responses include rowcount= items, either signalling if a bulk operation succeeded or failed.

• success means rowcount=1
• failure means rowcount=-2

https://cratedb.com/docs/crate/reference/en/latest/interfaces/http.html#error-handling

References

The code is coming from CrateDB Toolkit, but is generally usable beyond there.

• https://github.com/crate/cratedb-toolkit/blob/v0.0.27/cratedb_toolkit/io/core.py#L17-L77
• Table Loader and MongoDB: A few refinements cratedb-toolkit#270

[amotl]

tests.py is the entrypoint for zope.testing to discover test cases. Many utility functions had to be refactored away from here, in order to avoid circular imports.

[matriv]

Thx for improving the bulk response handling. Left some comments.

[matriv]

Looking at this more carefully, I don't like that BulkResponse is something you need con construct manually. Couldn't we directly return it from the insert execution, instead of a list of BulkResultItem?

[amotl]

Hi. I don't think we can do anything like this here, because the Python database driver must adhere to the Python Database API Specification, so the BulkResponse is just meant as an optional extension to it.

[amotl]

af32409, just added, provides a bit of documentation for that extension in the section about bulk operations.

[mfussenegger]

Could you elaborate a bit more on the motivation for this?

Seems to me as if getting the info without this is not really that much more difficult.

[amotl]

Hi @mfussenegger,

The code is coming from CrateDB Toolkit, but is generally usable beyond there.

• https://github.com/crate/cratedb-toolkit/blob/v0.0.27/cratedb_toolkit/io/core.py#L17-L77
• Table Loader and MongoDB: A few refinements cratedb-toolkit#270

The idea is to have a concise code representation over here, for the BulkProcessor component, by abstracting away the handling of magic numbers from user-side code.

Because CrateDB Toolkit is rather heavy, and I wanted to make it reusable, I wanted to add it elsewhere. Another option would be to use the now separate cratedb-sqlalchemy package. BulkProcessor depends on it anyway, but BulkResponse doesn't, and would provide a generic little convenience wrapper around CrateDB's special responses to bulk requests.

[mfussenegger]

The idea is to have a concise code representation over here, for the BulkProcessor component, by abstracting away the handling of magic numbers from user-side code.

That kinda confirms my point in that there's not much more code without the BulkResponse wrapper:

 cursor = self.connection.execute(statement=statement, parameters=operation.parameters)
 self.connection.commit()
 cratedb_bulk_result = getattr(cursor.context, "last_executemany_result", None)
 failed_records = 0
 success_count = 0
 for result in cratedb_bulk_result:
     if result["rowcount"] == -2:
         failed_records += 1
     else:
         success_count += 1
 self._metrics.count_success_total += success_count
 self.progress_bar and self.progress_bar.update(n=success_count)

This to me would even be more readable as it avoids the additional indirection.

Also, given that the DB API spec says:

Return values are not defined.

I think we could extend our return value - as long as we can keep it compatible with the existing one.