feat: implementation for replaceWhere

[r3stl355]

Description

First/naive implementation of replaceWhere for write. Code compiles and there is a test to verify the outcome. I would appreciate any feedback on improving the structure/implementation. For example, I copied the part of code from delete operation because there is no way to call that code in delete directly from write - should I look into extracting that code from delete to somewhere central?

Seems to also works with partitions columns.

Related Issue(s)

#1957

Documentation

Added a section in docs

[github-actions]

ACTION NEEDED

delta-rs follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

[r3stl355]

It is passing the CI on my box and with added docs I'm removing the draft once CI is passing here (one of the Python tests is flaky, need to be fixed)

[r3stl355]

looks like few extra checks were introduced recently so lint is picking up changes which are not mine, e.g. crates/deltalake-core/src/delta_datafusion/mod.rs:149:37

[ion-elgreco]

looks like few extra checks were introduced recently so lint is picking up changes which are not mine, e.g. crates/deltalake-core/src/delta_datafusion/mod.rs:149:37

Could you create a separate PR to address that so main is fixed?

[r3stl355]

Could you create a separate PR to address that so main is fixed?

Ahh, fixed and pushed here before seeing your response. Can do another PR if they still break but leaving that for another day, done for today

[r3stl355]

Looks like I am close to removing the draft, only need to fix this flakytests/test_writer.py::test_replace_where_overwrite - sometimes it fails, sometimes not even on my machine

[Blajda]

I copied the part of code from delete operation because there is no way to call that code in delete directly from write - should I look into extracting that code from delete to somewhere central?

I think this should be done. Delete is one of the things we should keep DRY which will help when deletion vectors are implemented. All operations will benefit instead of needing to update code in different location.

My 2 cents is that we shouldn't modify the writer to implement this operation but rather should have a brand new operation structured in a manner similar to merge, delete, and update.

Some builder interface like below

DeltaOps(table)
    .replace_where(predicate, source_data)
    .with_check_constraint(false)
    .await?

[r3stl355]

My 2 cents is that we shouldn't modify the writer to implement this operation but rather should have a brand new operation structured in a manner similar to merge, delete, and update.

@Blajda I understand that you suggesting a standalone operator just for replaceWhere, correct? Which means:

1. Removing a predicate from write, which is fine as it wasn't implemented anyway
2. Exposing a function fromdelete which behaves similar to writer::write_execution_plan by returning a set of actions
3. [MAYBE] Possibly exposing a version of write_execution_plan function that takes a predicate from write

If this is what you were suggesting I can give it a try

[Blajda]

@r3stl355 Yes I agree with point 1. For 2. one approach is to factor out a function that returns a tuple of files to be removed and an optional stream of record batches. I'm not sure on what 3. would provide.

[r3stl355]

@r3stl355 Yes I agree with point 1. For 2. one approach is to factor out a function that returns a tuple of files to be removed and an optional stream of record batches. I'm not sure on what 3. would provide.

The 3. would allow to check if the new data conforms to the replaceWhere predicate. Current implementation of the publicwrite_execution_plan does not do that and if I don't exposed a public function checking that the new data conforms to a predicate (e.g. write_execution_plan_with_predicate) then I would have to implement it within the replaceWhere op effectively copying the code

[ion-elgreco]

I'm not sure if I agree with taking this out of write. It's still a write operation

[MrPowers]

What's the user-facing public interface for Python?

[r3stl355]

@MrPowers Good old write_deltalake with predicate parameter. It was already there before this PR but was not implemented. All should be working with this PR

I also added docs and examples in docs for both Python and Rust (https://github.com/delta-io/delta-rs/pull/1996/files#diff-681ad64170174eef8ddc2c987ca2e99af32d90a79720b50b3f575a4a00ec4a50R55)

[MrPowers]

@r3stl355 - ah, got it. Here's the delta-spark syntax:

(
    df.write.format("delta")
    .option("replaceWhere", "number > 2")
    .mode("overwrite")
    .save("tmp/my_data")
)

Looks like the proposed syntax is as follows:

write_deltalake(
    table_path,
    data,
    mode="overwrite",
    predicate="id = '1'",
    engine="rust",
)

I almost feel like the mode should be replace_where. Thoughts?

[ion-elgreco]

@MrPowers I don't think it makes sense to introduce another mode because replace where without a predicate wouldnt be possible

[ion-elgreco]

Can you add one more test cases to match the pyarrow partition filter overwrite test cases? :)

A test case where the data is not meeting the constraint. Also I wonder if you could capture that constraint error and give a better error msg

[roeap]

Sorry it took so long, but finally got around to do some reviewing.

I am a bit less happy with my code now after snapshot was made an Option

This is actually also one of my biggest concerns annoyances right now, and kind of boild down to what purpose each of these structs serve in our codebase. Previously, DeltaTableState took no opinion on if Metadata and Protocol exist. However if we cannot get these, there really is not anything meaningful we can do with a delta table. However DeltaTable has some operations that work on empty locations etc. As a result, we were raising errors all throughout our operations if the back then optionsal metadata / protocol actions were not available. While this requires some more refactoring, all operations (except the ones that can cretate a table) should always require a valid snapshot.

btw. DeltaTableState is by now also just a thin wrapper around a snapshot, which I also hope to remove soon :).

[r3stl355]

Thank you for the review @roeap

[roeap]

Thanks for your patience with this PR @r3stl355, and great work!

I'll leave it open for a bit, since @MrPowers had some questions around the APIs, but form my end passing the predicate to write_deltalake seems reasonable. While this function already takes a lot of parameters, I feel this PR does not make it worse :).

[ion-elgreco]

@roeap once we drop the Pyarrow writer the write_deltalake will be much leaner

[r3stl355]

this PR does not make it worse :).

minimum standard achieved 😁 . I'll create couple of related issues

[r3stl355]

@MrPowers do you still have those questions around API? If not then this should be ready to merge (and I squashed all my commits into one as agreed)

[roeap]

@r3stl355 - it seems we have seen a bigger rename of files before I could get that in :( - sorry oyu have to resolve once more.

Since we haven't heard back, I would just merge once we are green again ...

[r3stl355]

Thanks @roeap , should be good now

[r3stl355]

It's been back and forth few times, something that I understand and don't mind but unfortunately I don't have time to handle this in next few weeks so parking it

[ion-elgreco]

Thanks Niko! Great feature :) We are one step closer to deprecating the PyArrow writer now!