feat(mssql): Microsoft SQL Server destination#6417
Conversation
|
This PR still required adding test workflow for destination testing + docs. Code is almost ready, though. |
yevgenypats
left a comment
There was a problem hiding this comment.
Right now the plugin incorrectly implements multiple interfaces and it needs to only implement the unmanaged plugin.
8e20e18 to
b13bca1
Compare
|
|
|
Hi @candiduslynx can we make sure to incorporate the fix from #6519? |
@erezrokah I think I already included those |
c6a42d9 to
d4a6a98
Compare
cb80ff5 to
44763c2
Compare
b8ea7a0 to
0dd4cc3
Compare
c02ba22 to
8dd0886
Compare
yevgenypats
left a comment
There was a problem hiding this comment.
Very nice! I really like the bulk-insert. In overwrite I think we can do something similar otherwise it will be quite slow / open many goroutines and so on.
I think in overwrite we should use a temptable and then do a merge on the serverside - it's quite common approach in databases that don't have good built-in batch support.
https://blog.jonschneider.com/2016/08/bulk-upsert-into-sql-server.html
https://stackoverflow.com/questions/4889123/any-way-to-sqlbulkcopy-insert-or-update-if-exists
|
@yevgenypats I tried using temporary tables and the overhead was enormous (see fae6b7a). |
251a480 to
f6f1be8
Compare
eedc504 to
aa5dcb9
Compare
aa5dcb9 to
1884152
Compare
1884152 to
e37c372
Compare
|
@erezrokah seems that some base links (not touched by this PR) were broken |
|
@yevgenypats I used table value parameters along with stored procedure to achieve better results. |
yevgenypats
left a comment
There was a problem hiding this comment.
Great job @candiduslynx !
This one is a complex one for sure as it's an "old" db :)
I played with and overall it looks good. I've a few comments but I'd like to wait for some real world usage to see if users hit any snags.
For example:
- TVP is really cool but also it adds complexity, which means harder to debug bugs. Can we achieve the same thing with temp tables? maybe and maybe it wont be better as temp tables also have their snags as not everything is happy flow in MSSQL world.
So let's put it out there and once we release tmrw we can update the users that requested that.
|
@yevgenypats thanks for the review! |
Implements #6145.
Depends on: