Repository navigation
Destination Snowflake: Revisiting merge instead of insert+delete - #43367
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join Gireesh Sreepathi (@gisripa) and the rest of your teammates on |
Octavia Squidington III (octavia-squidington-iii)
added
area/connectors
Gireesh Sreepathi (gisripa)
changed the title
2b2e786 to
7755aaa
Compare
7755aaa to
329ee6b
Compare
Edward Gao (edgao)
left a comment
Edward Gao (edgao)
left a comment
There was a problem hiding this comment.
ngl I was only skimming the diff by the end :P largely trusting our tests to keep us safe here
+I assume you're planning to run some tests in the perf test workspace to try and validate the performance impact? I'm actually very curious what happens there - faker reemitting the whole dataset on every sync is basically the worst case, and I have no idea how merge vs insert+delete will perform there
changes seem reasonable at a glance. had some nitpicks but nothing structural. LGTM pending testing.
There was a problem hiding this comment.
why not include this in columnsForUpdateList?
There was a problem hiding this comment.
nit: why not just put whenMatchedCdcDeleteCondition on its own line + remove the \n from it? Right now it kind of looks like whenMatchedCdcDeleteCondition is part of the ON clause
There was a problem hiding this comment.
Yeah there was an empty line when this condition was missing. wasn't sure if sql compilation failed for this or something else so was trying different things
48dba70 to
0fa45a9
Compare
bd085c3 to
1aadeb0
Compare
Octavia Squidington III (octavia-squidington-iii)
added
the
area/documentation
Edward Gao (edgao)
left a comment
Edward Gao (edgao)
left a comment
There was a problem hiding this comment.
lgtm pending switching to an abstract base class for the test
1aadeb0 to
ff96664
Compare
ff96664 to
c96b2cb
Compare
|
Gireesh Sreepathi (@gisripa) This PR is a huge deal for our business. Wanted to say thank you for opening this PR. |
|
just a heads up, we had to rollback to an older version of destination-snowflake for unrelated reasons. keep an eye on #44526, which will also rerelease this feature. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This branch was successfully deployed
5 participants
What
Fixes: airbytehq/airbyte-internal-issues#9931
See the rabbit hole of linked tickets above
Resurrecting: #31683
Switching back to
MERGE INTO ... WHEN...for CDC deletions.MERGEstill expects unique records in source dataset(a.k.a table from CTE in our case) to have a deterministic behavior.How
Review guide
Experimented a lot with Kotlin's idioms to deduplicate query string snippets.
Also consolidated the
row_numberwindow function to be executed on the preceding CTE (saw few millis impact when testing in console but not a great representation by individual runs)User Impact
Users have the ability to opt-in to use
MERGE.This is intentionally left as non-default for now because of previous history of rollback.
Other details
Tested syncs with both
insert+deleteandmergeusing the toggle option. No improvement in Sync time per se but no indication that warehouse costs would have increased if time spent in query is a representation of the warehouse credits used.Query Profiles
I did not find any evidence that MERGE could increase warehouse activity if Time spent by query is a representation of warehouse credits.
Final Table seeded with 100M rows.
Raw Table generated with 500K data with some random percentage updated and cdc-deleted
Profiles for Insert+Delete+Delete
Insert
Delete from final table duplicated PKs
CDC Deletes
Profile for Merge
Can this PR be safely reverted and rolled back?