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

Add | Add RowsCopied64 property, refs, and docs #2004

Merged
merged 3 commits into from
May 4, 2023

Conversation

Wraith2
Copy link
Contributor

@Wraith2 Wraith2 commented Apr 18, 2023

Closes #2001

Changes the SqlBulkCopy._rowscopied variable to a long and adds a new long SqlBulkCopy.RowsCopied64 property to the class.
The existing int SqlBulkCopy.RowsCopied behaviour of going negative is preserved. I have changed the implementation to be unchecked((int)_rowsCopied); to make it clear in the source that the unchecked behaviour is deliberate. The unchecked is not strictly required because it is the default compile option.
Is keeping the existing behaviour desiable? It could be changed to throw an overflow exception. Keeping the existing behaviour is most compatible in case someone has taken a dependency on the existing behaviour. It is not the most correct.

@codecov
Copy link

codecov bot commented Apr 18, 2023

Codecov Report

Patch coverage: 90.00% and project coverage change: -0.68 ⚠️

Comparison is base (a6e7e0d) 70.58% compared to head (171112b) 69.91%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2004      +/-   ##
==========================================
- Coverage   70.58%   69.91%   -0.68%     
==========================================
  Files         306      305       -1     
  Lines       61722    61795      +73     
==========================================
- Hits        43569    43206     -363     
- Misses      18153    18589     +436     
Flag Coverage Δ
addons 92.88% <ø> (ø)
netcore 73.38% <85.71%> (+0.02%) ⬆️
netfx 68.16% <85.71%> (-1.00%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...rc/Microsoft/Data/SqlClient/RowsCopiedEventArgs.cs 75.00% <75.00%> (+3.57%) ⬆️
...etcore/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs 70.54% <100.00%> (+0.02%) ⬆️
.../netfx/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs 70.43% <100.00%> (-0.03%) ⬇️

... and 26 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -509,9 +509,30 @@ and faster to use a Transact-SQL `INSERT … SELECT` statement to copy the data.
This value is incremented during the <xref:Microsoft.Data.SqlClient.SqlBulkCopy.SqlRowsCopied> event and does not imply that this number of rows has been sent to the server or committed.

This value can be accessed during or after the execution of a bulk copy operation.

This value can become negative if the number of rows exceelds int.MaxValue. If this may happen use the RowsCopied64 property.
Copy link
Contributor

Choose a reason for hiding this comment

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

  • exceelds -> exceeds (typo)
  • RowsCopied64 should have some kind of ref tag?
  • I feel like this language should be stronger: "This value will wrap around and become negative if... Applications should use RowsCopied64 instead".

Copy link
Contributor Author

@Wraith2 Wraith2 Apr 19, 2023

Choose a reason for hiding this comment

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

Changed to:
This value will wrap around and become negative if the number of rows exceeds int.MaxValue. Consider using the <xref:Microsoft.Data.SqlClient.SqlBulkCopy.RowsCopied64> property.

If the tone needs to be stronger than that I'll leave the exact wording to the MS team.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like your rewording. As an app developer I would just always use RowsCopied64 now that it exists since there's no cost and real upside.

@JRahnama
Copy link
Contributor

JRahnama commented May 3, 2023

LGTM, Is it possible to add some tests as well?

@JRahnama JRahnama added the 🆕 Public API Issues/PRs that introduce new APIs to the driver. label May 3, 2023
@Wraith2
Copy link
Contributor Author

Wraith2 commented May 3, 2023

Possible, yes. But why? it's a tiny property with an entirely obvious implementation.

@JRahnama
Copy link
Contributor

JRahnama commented May 3, 2023

Possible, yes. But why? it's a tiny property with an entirely obvious implementation.

We are trying to improve the code coverage and each line of new code added with no test will cause some shortage in the code coverage percentage. That is the only reason.

@Wraith2
Copy link
Contributor Author

Wraith2 commented May 3, 2023

I've altered the two existing tests which cover RowsCopied to also check RowsCopied64.

@JRahnama JRahnama merged commit 91694d8 into dotnet:main May 4, 2023
@Wraith2 Wraith2 deleted the add-rowscopied64 branch May 4, 2023 07:46
@JRahnama JRahnama changed the title Add RowsCopied64 property, refs, and docs Add | Add RowsCopied64 property, refs, and docs Jun 7, 2023
kant2002 pushed a commit to kant2002/SqlClient that referenced this pull request Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 Public API Issues/PRs that introduce new APIs to the driver.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SqlBulkCopy.RowsCopied overflows because it is an int
5 participants