fix(s3): restore working test for DynamoDb log store repair log on read

[dispanser]

Description

Make sure the read path for delta table commit entries passes through the log store, enabling it to ensure the invariants and potentially repair a broken commit in the context of S3 / DynamoDb log store implementation.

This also adds another test in the context of S3 log store: repairing a log store on load was not implemented previously.

Note that this a stopgap and not a complete solution: it comes with a performance penalty as we're triggering a redundant object store list operation just for the purpose of "triggering" the log store functionality.

fixes #2109

[ion-elgreco]

@dispanser how about the python S3 round trip test?

[dispanser]

@dispanser how about the python S3 round trip test?

This test isn't related to the changes in this PR. @roeap , is this some known regression related to your changes?

[dispanser]

For this test, test_roundtrip_s3_env, there's no dynamodb-based locking configured, so it falls back to the DefaultLogStore implementation. Here's what happens:

first write_deltalake(..) called by the test:

1. commit_with_retries prepares a temporary commit entry (json file)
2. S3StorageBackend::rename_if_not_exists is called to rename the commit into its final destination, 0.json
3. since self.allow_unsafe_rename is not set, it throws an error as there's no safe way to atomically rename

second write_deltalake(..) called by the test, after config change to inject setenv("AWS_S3_ALLOW_UNSAFE_RENAME", "true"):

1. verify that the table doesn't yet exist. This fails, because apparently our condition to see if the table already exists is to check that table_root is not empty!

The underlying problem is that we detect the attempt to write to a table that can't be written to in a very late stage of writing. As a result, there's already some files (both parquet files and a temporary commit entry) present, which makes the next incarnation of delta lake think that the table already exists.

Both commit_with_retries and DefaultLogStore are entirely unaware that the requested write won't work, so the json commit entries are there, and in any case, parquet files are also present. I wonder how that test ever worked, because even if detecting the problem earlier, at least the parquet files will already be present. Also, there's not really any harm done - some files are present, but since there's no proper commit entry, everything is ignored.

Potential fixes:

1. a better way to test if a table already exists
2. have some method LogStore::can_write() or similar that signals if a write is even possible. In a different universe, we might have separate traits for writeable and non-writeable tables and constructing a writeable table would fail long before a single byte enters any parquet file
3. ignore the test, or make it less picky :-).

I tend to think that it's valuable to avoid accidental overwriting of an existing table, and I do think our existing method to detect the presence of an existing table is too limiting - so in my opinion, option 1 seems most sensible to me.

Opionions? @ion-elgreco @rtyler @roeap ?

[roeap]

This test isn't related to the changes in this PR. @roeap , is this some known regression related to your changes?

Yes, that failure was introduced, when we moved to the arrow backend.

Also thanks for digging into it and figuring out why the round-trip test fails. I agree with your assessment, that a better way to see if we have a table is the way to go. The current approach is to check if there is a _delta_log folder (albeit in a very inefficient way via a list due to some quirks we had ion windows machines that prevented a simple head). This check would indeed consider a location with a failed commit 0 as an existing table location. IIRC, sprak also just checks if _delta_log exists.

Maybe we could just try to create a new LogSegment and see if that is non-empty, as it already includes checks for valid files?

[roeap]

Thanks for fixing this @dispanser - this brings us much closer to a releasable state again 👍.

[roeap]

should we add a TODO here o.a. saying why this statement exists and the we want to get rid of it?

[roeap]

Note that this a stopgap and not a complete solution: it comes with a performance penalty as we're triggering a redundant object store list operation just for the purpose of "triggering" the log store functionality.

For most of our users this should not be too bad, but for some tables this can be very expensive. Particularly if you have streaming scenarios we sometimes see A LOT of versions ...

Ultimately I hope that we can converge on some consolidated storage trait, but I hope that we can find out wht the write abstractions will look like in a post-kernel world.

[roeap]

@dispanser, when looking into this, I though that updating this line

delta-rs/crates/deltalake-core/src/kernel/snapshot/log_segment.rs

Line 247 in 354fda3

async move { store.get(&meta.location).await?.bytes().await }

to use LogStore would resolve the issue. Would that have worked as well and would that avoid the extra list?

[dispanser]

@dispanser, when looking into this, I though that updating this line

delta-rs/crates/deltalake-core/src/kernel/snapshot/log_segment.rs

Line 247 in 354fda3

async move { store.get(&meta.location).await?.bytes().await }

to use LogStore would resolve the issue. Would that have worked as well and would that avoid the extra list?

Yeah, that would also work. LogStore::read_commit_entry(..) would touch DynamoDb and then retrieve the bytes of the commit using the underlying object store. However, this also means that for each individual version we retrieve, DynamoDb interaction is necessary. This is wasteful and not useful, especially if we are going to read multiple versions consecutively.

To avoid this repeated interaction with DynamoDb, it would be good to replace read_commit_entry(version) with something that accepts a version range instead. This is sort of what LogSegment::try_new_slice(..) does but at a lower level. Or maybe a log-aware replacement for list_log_files(..), that should be all we need.

[roeap]

@dispanser and i discussed we'll wait and explore an alternative before merging this one ...

[rtyler]

I interpret @roeap 's last comment to mean that this is no longer approved. Please dismiss this review when it's mergeable so nobody hits the tempting green Merge button 😄