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

synapse and mssql bugfixes and improvements #1174

Merged
merged 10 commits into from
Apr 7, 2024

Conversation

jorritsandbrink
Copy link
Collaborator

@jorritsandbrink jorritsandbrink commented Apr 2, 2024

Description

This PR

  • adds chunk separation handling for the select_union insert values writer type to fix select_union insert values writer type fails with buffered writing #1173
  • sets the max_rows_per_insert property to prevent "Some part of your SQL statement is nested too deeply. Rewrite the query or break it up into smaller queries." errors
  • adds mssql and synapse to set of tested destinations in test_insert_job_client.py for better test coverage
  • removes pipeline dependency in SynapseStagingCopyJob.generate_sql by passing the job_client as argument
  • improves PyOdbcMsSqlClient.rollback_transaction such that it won't fail if there is nothing to rollback
  • reorganizes logic in some places for better code quality

Related Issues

@jorritsandbrink jorritsandbrink self-assigned this Apr 2, 2024
Copy link

netlify bot commented Apr 2, 2024

Deploy Preview for dlt-hub-docs canceled.

Name Link
🔨 Latest commit ee1910b
🔍 Latest deploy log https://app.netlify.com/sites/dlt-hub-docs/deploys/661198f3073be300089b340e

@jorritsandbrink jorritsandbrink requested a review from sh-rp April 2, 2024 15:28
Copy link
Collaborator

@rudolfix rudolfix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take a look at test_insert_job_client, you should add synapse there:

DEFAULT_SUBSET = ["duckdb", "redshift", "postgres"]

and also mssql (it also uses inserts)
you can then merge your test into it

@jorritsandbrink jorritsandbrink changed the title Add missing chunk separation handling for select_union writer type (bugfix) synapse and mssql bugfixes and improvements Apr 6, 2024
@jorritsandbrink
Copy link
Collaborator Author

@rudolfix I added mssql and synapse to test_insert_job_client.py and made some changes so the tests pass. Also removed the pipeline dependency to obtain a job_client in SynapseStagingCopyJob.generate_sql as discussed on Slack—this is done in an unsophisticated way that violates from_table_chain and generate_sql interfaces because the proper way would involve substantial code changes, type checking errors are ignored. See #1174 (comment) for an overview of the different changes contained in this PR.

Can you review?

@jorritsandbrink jorritsandbrink requested a review from rudolfix April 6, 2024 19:13
Copy link
Collaborator

@rudolfix rudolfix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! thanks for improving the tests!!

@rudolfix rudolfix merged commit d3ecc9e into devel Apr 7, 2024
49 of 53 checks passed
@rudolfix rudolfix deleted the 1173-bug-select-union-buffer-size branch April 7, 2024 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

select_union insert values writer type fails with buffered writing
2 participants