Skip to content
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

feat: use append vs merge option from backend config #3965

Conversation

fracasula
Copy link
Collaborator

@fracasula fracasula commented Oct 11, 2023

Description

Adding support for opt-in deduplication.

Linear Ticket

< Linear Link >

E2E Warehouse run

https://github.com/rudderlabs/e2e/actions/runs/6587296799

Updated Flow Diagram

append_vs_merge drawio

Outstanding questions

  • do we want to have a shouldMerge method in mssql as well here like we do in postgres? if we add it, is it OK for existing customers that are using mssql?
  • should we review also other destinations like azure synapse and add a shouldMerge there as well?

TODOs

  • document CanAppend with a generic statement in the public docs saying that even though the user didn't opt-in with enableMerge we might merge anyway due to other conditions e.g. if they connected an extract source, we replay the data, etc...
  • check if we have live configurations for our customers that are using loadTableStrategy and replace that with enableMerge

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

@fracasula fracasula added question Further information is requested Do not merge warehouse-team labels Oct 11, 2023
fracasula

This comment was marked as resolved.

@fracasula fracasula changed the title Feature/pipe 258 use append vs merge option from backend config UI feat: use append vs merge option from backend config UI Oct 11, 2023
@fracasula fracasula changed the title feat: use append vs merge option from backend config UI feat: use append vs merge option from backend config Oct 17, 2023
@fracasula fracasula force-pushed the feature/pipe-258-use-append-vs-merge-option-from-backend-config-ui branch from 2bce617 to 367bd51 Compare October 19, 2023 13:43
@codecov
Copy link

codecov bot commented Oct 20, 2023

Codecov Report

Attention: 32 lines in your changes are missing coverage. Please review.

Comparison is base (72423dd) 71.64% compared to head (e738f57) 71.26%.
Report is 1 commits behind head on master.

❗ Current head e738f57 differs from pull request most recent head a1b912e. Consider uploading reports for the commit a1b912e to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3965      +/-   ##
==========================================
- Coverage   71.64%   71.26%   -0.38%     
==========================================
  Files         372      370       -2     
  Lines       54977    54083     -894     
==========================================
- Hits        39389    38544     -845     
+ Misses      13262    13231      -31     
+ Partials     2326     2308      -18     
Files Coverage Δ
testhelper/clone.go 100.00% <100.00%> (ø)
warehouse/integrations/mssql/mssql.go 76.96% <100.00%> (-0.07%) ⬇️
warehouse/integrations/postgres/load.go 87.27% <100.00%> (-2.85%) ⬇️
warehouse/integrations/postgres/postgres.go 75.98% <100.00%> (+0.07%) ⬆️
warehouse/internal/model/warehouse.go 100.00% <100.00%> (ø)
warehouse/router/router.go 87.52% <100.00%> (-0.05%) ⬇️
warehouse/utils/querytype.go 100.00% <100.00%> (ø)
warehouse/integrations/deltalake/deltalake.go 78.43% <88.88%> (-0.06%) ⬇️
warehouse/integrations/snowflake/snowflake.go 66.51% <88.88%> (-1.66%) ⬇️
warehouse/integrations/bigquery/bigquery.go 63.62% <72.00%> (-0.75%) ⬇️
... and 1 more

... and 53 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fracasula fracasula marked this pull request as ready for review October 20, 2023 12:41
@fracasula
Copy link
Collaborator Author

@lvrach @achettyiitr hey guys, please review also the flow diagram and the outstanding questions (see PR description).

warehouse/integrations/bigquery/bigquery.go Outdated Show resolved Hide resolved
warehouse/integrations/deltalake/deltalake.go Show resolved Hide resolved
warehouse/internal/model/warehouse.go Outdated Show resolved Hide resolved
warehouse/integrations/snowflake/snowflake.go Outdated Show resolved Hide resolved
warehouse/integrations/postgres/load.go Outdated Show resolved Hide resolved
warehouse/integrations/bigquery/bigquery_test.go Outdated Show resolved Hide resolved
@fracasula fracasula force-pushed the feature/pipe-258-use-append-vs-merge-option-from-backend-config-ui branch 2 times, most recently from 7f68b03 to a8cba84 Compare October 30, 2023 16:38
@fracasula fracasula force-pushed the feature/pipe-258-use-append-vs-merge-option-from-backend-config-ui branch from a8cba84 to a1b912e Compare October 30, 2023 16:40
@fracasula fracasula merged commit 6d2db45 into master Oct 30, 2023
33 checks passed
@fracasula fracasula deleted the feature/pipe-258-use-append-vs-merge-option-from-backend-config-ui branch October 30, 2023 17:16
@achettyiitr achettyiitr mentioned this pull request Nov 3, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants