-
Notifications
You must be signed in to change notification settings - Fork 268
Move implementation of upsert from Table to Transaction #1817
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
I think since the transaction wrapper has been moved out, there should be a unit test added to do partial upsert and then throw an error and ensure the rollback occurs and we are not left in a state where a partial upsert succeeded. Example:
Just my thoughts 😃. Thanks, |
Agree! I will work on the test. With "update" you mean "delete", right? |
Hey sorry; just saw this; when i mean update, i mean it invokes an "overwrite" operation, which i believe is what delete's also trigger under the covers. 😀 |
There is a nice edgecase here.. tbl = catalog.create_table(identifier, schema=schema)
# Define exact schema: required int32 and required string
arrow_schema = pa.schema([
pa.field("id", pa.int32(), nullable=False),
pa.field("name", pa.string(), nullable=False),
])
tbl.append(pa.Table.from_pylist([{"id": 1, "name": "Alice"}], schema=arrow_schema))
df = pa.Table.from_pylist([{"id": 2, "name": "Bob"}, {"id": 1, "name": "Alicia"}], schema=arrow_schema)
with tbl.transaction() as txn:
txn.upsert(df, join_cols=["id"])
# This will re-insert Bob, instead of reading the uncommitted changes and ignore Bob
txn.upsert(df, join_cols=["id"]) @Fokko should it be possible to read uncommitted changes? |
now that #1903 is merged, could you rebase this PR? |
Rationale for this change
Previously, the upsert functionality was implemented at the table level, which meant it always initiated a new Transaction. This change moves the upsert implementation to the Transaction level while keeping
table.upsert(...)
as an entry point.With this refactor, end users now have the flexibility to call upsert in two ways:
table.upsert(...)
– which still starts a new transaction.transaction.upsert(...)
– allowing upserts within an existing transaction.Are these changes tested?
Using existing tests.
Are there any user-facing changes?
Yes. This change enables users to perform upserts within an existing transaction using
transaction.upsert(...)
, in addition to the existingtable.upsert(...)
method.