fix: remove forced table update from python writer

[ohanf]

Description

Performing (concurrent) idempotent blind-appends by comparing transaction identifiers from python is not possible. The reason for this is the internal table state is updated automatically before the actual write is performed. To perform idempotent blind appends (adapted from Spark's strategy for the same) one must load a table snapshot and compare their App ID + version to what they find in the snapshot, if the "local" version is > the one in the snapshot then the write can proceed, and any race conditions for that specific app+version are handled by the conflict checker and atomic commits. However when the table state is updated, the snapshot the app logic used for the comparison is replaced so the conflict check for updated transactions will always pass and the write will always be attempted.

This PR simply removes the automatic snapshot update from the python writer.

Running the following example script at the same time in two different windows (but same directory) should illustrate the issue:

import sys
import time
from datetime import datetime
from pathlib import Path

import pyarrow as pa

from deltalake import CommitProperties, DeltaTable, Transaction, write_deltalake

TABLE = "concurrency"
data = pa.Table.from_pylist([{"example": 1}])

if not Path(TABLE).exists():
    write_deltalake(TABLE, data)

def write(table: DeltaTable, version: int, wait: int, skip: bool) -> None:
    txn = Transaction(app_id="demo", version=version)
    props = CommitProperties(app_transactions=[txn], max_commit_retries=0)
    table.update_incremental()
    print(f"{datetime.now()}> table updated to {table.version()=}, sleeping {wait}")
    time.sleep(wait)
    rtx = table.transaction_version("demo")
    if skip and rtx and rtx >= version:
        print(f"{datetime.now()}> skipping since version already written")
        return

    write_deltalake(table, data, mode="append", commit_properties=props)
    
    print(f"{datetime.now()}> wrote {version=}, {table.version()=}")


if __name__ == "__main__":
    wait = int(sys.argv[1])
    skip = False
    if len(sys.argv) > 2:
        skip = True
    table = DeltaTable(TABLE)
    for v in range(5):
        write(table, v, wait, skip)
        print("loop", v)

screenshot from running against main

and against this branch

This should probably be considered a bug fix, but for some it may be "breaking" as writes were previously always attempted and now they may be aborted. However the scope is relatively small as the impact is for writes that included Transaction properties, if we remove the commit properties from the demo script we will see warnings on write that there is a mismatch in expected and written version, but no hard failures:

WARN deltalake_core::kernel::transaction] Attempting to write a transaction 3 but the underlying table has been updated to 4

Related Issue(s)

• Don't commit if commit has transaction identifier that has been committed already #2580

Documentation

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.21%. Comparing base (2ffaacc) to head (32ec073).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3515      +/-   ##
==========================================
- Coverage   74.25%   74.21%   -0.04%     
==========================================
  Files         150      150              
  Lines       44739    44739              
  Branches    44739    44739              
==========================================
- Hits        33220    33204      -16     
- Misses       9374     9378       +4     
- Partials     2145     2157      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[ion-elgreco]

@ohanf is it possible for you to add a test for this new behaviour?

@roeap can you also take a look, I am not an expert on Spark delta transactions

[ohanf]

@ohanf is it possible for you to add a test for this new behaviour?

@ion-elgreco done! Let me know if that covers what you wanted to see :)

FWIW running the new test cases against main has two of them fail (as expected, the reason I opened this PR):

FAILED tests/test_writer.py::test_write_concurrent_blind_appends[writer1_txn0-writer2_txn0-False-True] - Failed: DID NOT RAISE <class '_internal.CommitFailedError'>
FAILED tests/test_writer.py::test_write_concurrent_blind_appends[writer1_txn2-writer2_txn2-False-True] - Failed: DID NOT RAISE <class '_internal.CommitFailedError'>

[ion-elgreco]

It's breaking in behavior, but new behavior is more correct

[ohanf]

Hi @roeap, anything I can do to help move this along?

[roeap]

@ohanf - currently I am traveling and therefore unfortunately have no bandwidth to do proper reviews. I'll be back next week and have capacity again.

Sorry for the delay!

[ohanf]

@ohanf - currently I am traveling and therefore unfortunately have no bandwidth to do proper reviews. I'll be back next week and have capacity again.

Sorry for the delay!

no worries, safe travels!