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

Fixed issue with SQLServerBulkCopy from CSV with setEscapeColumnDelimerts set to true #2575

Merged
merged 8 commits into from
Jan 27, 2025

Conversation

machavan
Copy link
Contributor

@machavan machavan commented Jan 6, 2025

Description:

The method SQLServerBulkCSVFileRecord::readLineEscapeDelimiters introduced for supporting newline characters as part of field values ( #2338) missed to handle the case of CSV file not having a newline character at the end of the last row in CSV file, resulting into skipping the last record when setEscapeColumnDelimerts is set to true.

Tests:

  • All existing tests units pass
  • Introduced additional tests to verify the reported scenario.

@machavan machavan linked an issue Jan 6, 2025 that may be closed by this pull request
divang
divang previously approved these changes Jan 7, 2025
tkyc
tkyc previously approved these changes Jan 7, 2025
codewatchzen
codewatchzen previously approved these changes Jan 8, 2025
@yatharthgovil
Copy link

Hi @machavan
Thanks for resolving the issue, I want to know one more thing what method Microsoft recommend to load data to the target mssql table We have seen some gain with using SqlServerBulkCopy api and using ISqlServereBulkRecord against loading data using the same class using csv file.
Would really appreciate if you could answer this query.

@machavan
Copy link
Contributor Author

Hi @machavan Thanks for resolving the issue, I want to know one more thing what method Microsoft recommend to load data to the target mssql table We have seen some gain with using SqlServerBulkCopy api and using ISqlServereBulkRecord against loading data using the same class using csv file. Would really appreciate if you could answer this query.

Hi @yatharthgovil

Yes, bulk Copy offers a significant performance advantage.

Please refer:
Bulk Copy: https://learn.microsoft.com/en-us/sql/connect/jdbc/using-bulk-copy-with-the-jdbc-driver?view=sql-server-ver16

and

Bulk Copy API for Batch Insert: https://learn.microsoft.com/en-us/sql/connect/jdbc/use-bulk-copy-api-batch-insert-operation?view=sql-server-ver16

Copy link
Contributor

@lilgreenbird lilgreenbird left a comment

Choose a reason for hiding this comment

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

see comment re test cleanup

@yatharthgovil
Copy link

Hi @machavan can you please let us know when this will be available for us to consume also whats the plan will this fix be available with the next version release? if so can you please let us know when is that planned also will it be a minor version upgrade or a major version upgrade?
We would like to know the ETA for when its avaialble for us to consume.

Jeffery-Wasty
Jeffery-Wasty previously approved these changes Jan 20, 2025
@yatharthgovil
Copy link

Hi @machavan can you please provide clarification for the concern I have mentioned in the previous comment or provide any ETA for the same?

@machavan
Copy link
Contributor Author

Hi @machavan can you please let us know when this will be available for us to consume also whats the plan will this fix be available with the next version release? if so can you please let us know when is that planned also will it be a minor version upgrade or a major version upgrade? We would like to know the ETA for when its avaialble for us to consume.

Hi @yatharthgovil

We will look to include this fix in the next release, will update you shortly on the timeline for the same.

@machavan machavan dismissed stale reviews from codewatchzen, divang, Jeffery-Wasty, and tkyc via 6adfc61 January 23, 2025 05:43
Copy link

codecov bot commented Jan 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.24%. Comparing base (08cd6fd) to head (a2620e8).
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2575      +/-   ##
============================================
+ Coverage     51.22%   51.24%   +0.02%     
- Complexity     3953     3968      +15     
============================================
  Files           147      147              
  Lines         33657    33657              
  Branches       5624     5624              
============================================
+ Hits          17241    17248       +7     
- Misses        13999    14003       +4     
+ Partials       2417     2406      -11     

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

@machavan
Copy link
Contributor Author

ADO tests passed for this PR.

@machavan machavan merged commit 03cfcfd into main Jan 27, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed/Merged PRs
Development

Successfully merging this pull request may close these issues.

Regression in mssql-jdbc version 12.8.1
9 participants