[discuss] `Transaction` API's `autocommit`

[kevinjqliu]

Porting over from #1246

Can be a potential footgun (#1246 (comment))
Autocommit usage (#1246 (comment))

However, there may still be some concerns around this since Transaction is a public class. If this is the case, I think we can start from making the parameter "private" (autocommit -> _autocommit) and/or adding some doc to explain the usage.

[kevinjqliu]

The idea is to make the code simpler if we only want to evolve schema/spec/...
i.e.

with table.update_schema() as update:
    update.add_column("some_field", IntegerType(), "doc")

instead of another with..transaction wrapper

with table.transaction() as transaction:
    with transaction.update_schema() as update_schema:
        update.add_column("some_other_field", IntegerType(), "doc")

....

What about moving the autocommit logic out of Transaction and into the class that uses it instead?

For example, UpdateSchema can implement __enter__ and __exit__ to commit the transaction automatically?

Currently, these classes use autocommit=True:

• ManageSnapshots
• UpdateSchema
• UpdateSpec

[jiakai-li]

Just raise another possibility for discussion. We could also modify the _transaction._autocommit when calling UpdateSchema.__enter__ method, which indicates multiple updates is coming and we would like to avoid the autocommit. Something like:

class UpdateSchema(UpdateTableMetadata["UpdateSchema"]):
    ...
    def __enter__(self):
        self._transaction._autocommit = False
        return self
    ...

The reason behind my head is, to my understanding, autocommit is still somewhat a related concept with transaction.

[jiakai-li]

Adding an example for the issue:

from pyiceberg.catalog.sql import SqlCatalog
from pyiceberg.schema import Schema
from pyiceberg.types import IntegerType, NestedField, StringType

WAREHOUSE_PATH = "/tmp/warehouse"
catalog = SqlCatalog(
    "default",
    uri=f"sqlite:///{WAREHOUSE_PATH}/pyiceberg_catalog.db", warehouse=f"file://{WAREHOUSE_PATH}",
)
catalog.create_namespace_if_not_exists("default")

try:
    catalog.drop_table("default.test")
except:
    pass

schema = Schema(
    NestedField(1, "field1", StringType(), required=False)
)

table = catalog.create_table("default.test", schema)

with table.update_schema() as update:
    update.add_column("field_should_not_exist", IntegerType())  # <--- This field should not be committed
    raise Exception("Error!")

[kevinjqliu]

The reason behind my head is, to my understanding, autocommit is still somewhat a related concept with transaction.

I think its fair to ask if we consider UpdateSchema to be a transaction. If so, we cannot allow autocommit since that'll break the atomicity of the transaction.

We could also modify the _transaction._autocommit when calling UpdateSchema.enter method, which indicates multiple updates is coming and we would like to avoid the autocommit. Something like:

I feel like in this case, we can just get rid of autocommit entirely

cc @Fokko / @HonahX from the original conversation

[github-actions]

This issue has been automatically marked as stale because it has been open for 180 days with no activity. It will be closed in next 14 days if no further activity occurs. To permanently prevent this issue from being considered stale, add the label 'not-stale', but commenting on the issue is preferred when possible.