feat(api): implement upsert() using MERGE INTO

[deepyaman]

Description of changes

Implement Backend.upsert() using sqlglot.expressions.merge() under the hood. Upsert support is very important, especially for data engineering use cases.

Starting with a most basic implementation, including only supporting one join column. I think this could be expanded to support a list without much effort.

MERGE INTO support is limited. DuckDB only added support for MERGE statements earlier today in 1.4.0, and many other backends don't support it. However, it seems like the more standard/correct approach for supporting upserts, and it doesn't require merge keys defined ahead of time on tables.

Backends that work:

• DuckDB (from 1.4.0)
• Flink
• Oracle (currently using a hack to work around "AS" getting added to MERGE statement)
• MS SQL (currently throwing a ; onto the end of every statement 😅)
• Postgres

Should work, need help to test:

• Databricks
• Snowflake
• BigQuery

Backends that don't work:

• PySpark ("MERGE INTO TABLE is not supported temporarily.")
• Clickhouse
• DataFusion
• SQLite (supports the nonstandard UPSERT statement)
• Impala ("The MERGE statement is only supported for Iceberg tables.")
• MySQL
• Polars
• RisingWave
• Athena ("MERGE INTO is transactional and is supported only for Apache Iceberg tables in Athena engine version 3.")
• Trino ("connector does not support modifying table rows")

Issues closed

• Resolves Add Backend.upsert #5391

[deepyaman]

@cpcloud Requesting review for initial feedback while I try to improve backend support; I assume I could always mark a lot of them notyet if the approach is correct but some translations just need work.

[deepyaman]

I'm honestly not sure how to better do this; I wasn't able to figure out how to add a semicolon to an expression in SQLGlot.

[NickCrews]

This isn't ideal but I think is fine with me. The "cleaner" way would be to override the _build_upsert_from_table method, but the amount of boilerplate for that feels not worth it.

[NickCrews]

Or actually, is it not possible to just always stick on a ; at the end of _build_upsert_from_table() for every backend? or does that break on some backends?

[deepyaman]

Or actually, is it not possible to just always stick on a ; at the end of _build_upsert_from_table() for every backend?

I don't actually know how to stick a semicolon on the end of a SQLGlot expression. 😅 If _build_upsert_from_table() was handling the conversion to SQL, that would have been simple enough.

[NickCrews]

Ah, I understand.

huh, is this a bug in the upstream mssql engine? Can you confirm that only merge statements require trailing semicolons, but other sql queries/statements do not?

[deepyaman]

Yes, semicolons are only enforced for certain statements, including MERGE.

There's also a link in that answer to the official documentation stating that, at some point in the future, they will require terminators for every statement, but that clearly hasn't happened in the nearly 10 years since that answer was given.

[deepyaman]

@cpcloud I'm temporarily cherry-picked the changes from #11636 and the updates to xfail/xpass two DuckDB tests in order to get DuckDB and Oracle tests working (need the newer DuckDB and SQLGlot releases). All of these changes are in the last 3 commits:

• 7fddcb9
• 0ca5379
• adc70e7

With that, all of the functionality to implement Backend.upsert() is working and ready for review. The remaining issues all also exist in #11637, so I won't duplicate solving them. Whenever you do merge #11637, I should be able to back out the last 3 commits and rebase this on top.

[deepyaman]

@cpcloud I'm temporarily cherry-picked the changes from #11636 and the updates to xfail/xpass two DuckDB tests in order to get DuckDB and Oracle tests working (need the newer DuckDB and SQLGlot releases). All of these changes are in the last 3 commits:

• 7fddcb9
• 0ca5379
• adc70e7

With that, all of the functionality to implement Backend.upsert() is working and ready for review. The remaining issues all also exist in #11637, so I won't duplicate solving them. Whenever you do merge #11637, I should be able to back out the last 3 commits and rebase this on top.

@cpcloud FYI I've done this and everything is passing! Should be ready to go.

[deepyaman]

@NickCrews Addressed all your comments (except didn't add an xfail test, but replied to your suggestion). Please feel free to take another look!

[NickCrews]

Assuming an existing table with columns {i: int64, s: string, f: float64}, can you add a tests for upserting objects (using condition i=i) with schemas

• {i: int64, s: string, f: float64} (works)
• {s: !string, f: float32, i: uint8} (different order and flavors, but dtypes still compatible, works)
• {i: int64} (success, but nothing is updated)
• {i: int64, s: string} (only s is updated)
• {s: string} (error, i not present)
• {i:int64, b: boolean} (error, b is not in dest table)

[NickCrews]

Then, depending on how the "should we fallback to positional ordering" decision goes, we should add tests for that too. But I think all the above tests should still be valid regardless of what we decide there.

[deepyaman]

@NickCrews I've implemented all of the above, with the following differences:

• {i: int64} (success, but nothing is updated) (error, no cols to insert)
• {i:int64, b: boolean} (error, b is not in dest table cols to insert includes columns not in table)

The latter relates to your questions around the columns to insert behavior, I think. The former is somewhat related; seems it could be nice to support the no-update case, but I also don't know how important it is.

[NickCrews]

Thanks @deepyaman, those tweaks all look great. I came up with several more test cases I'd love to see. Sorry for the continually moving target.

[NickCrews]

Oof, I see what you mean how you are inheriting this logic from insert. I think it is essential that they use the same logic.

But, I think this is a footgun waiting to happen. I think we should make a breaking change to .insert() and require that source be a subset of target. What do you think of this change @cpcloud ?

[NickCrews]

I don't think there is anywhere else in ibis where we rely on positional ordering of columns, is there? I think we should keep that in mind, if we make this change here, then I think we should wipe out all other instances of relying on positional ordering.

[NickCrews]

Asking copilot Is there anywhere in this codebase where we rely on the position of columns in tables/relations? Or do we always match by column name? gives:

Looking at the codebase, Ibis primarily relies on column names rather than positions, but there are some specific cases where positional ordering is used:

Column Name-Based Operations (Majority)

Most operations use column names:

• Table.join() matches columns by name (e.g., "movieId")
• Table.select() references columns by name
• Schema.name_at_position() exists but is used to look up names by position, then uses names
• Field operations match by column name

Position-Based Operations (Specific Cases)

However, there are cases where position matters:

1. Positional table access: Table.__getitem__ supports t[0] for first column, t[1] for second, etc.

2. Positional joins: There's a "positional" join kind mentioned in JoinKind that joins tables by row position rather than column values. See test_positional_join.

3. Schema comparison: Schema.equals() explicitly states that "The order of fields in the schema is taken into account when computing equality."

4. Column insertion during insert(): In SQLBackend._build_insert_from_table, columns are matched by position when source columns are not a subset of target columns.

5. Info operations: Table.info() includes a pos field tracking column position.

So the answer is: primarily name-based, but position is significant for schema equality, positional joins, and some insertion scenarios.

[deepyaman]

I'm going to put this aside for now, since I think at the very least such a change should come in a commit separate from the Backend.upsert() implementation.

[deepyaman]

Thanks @deepyaman, those tweaks all look great. I came up with several more test cases I'd love to see. Sorry for the continually moving target.

@NickCrews No worries, thanks for the revised review! I believe I've largely addressed your updated feedback.