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

fix bulk copy cursor issue #270

Merged
merged 9 commits into from
May 9, 2017

Conversation

xiangyushawn
Copy link
Contributor

@xiangyushawn xiangyushawn commented Apr 28, 2017

Fixes #236

@xiangyushawn xiangyushawn changed the title fBulk copy cursor issue fix Bulk copy cursor issue Apr 28, 2017
@xiangyushawn xiangyushawn changed the title fix Bulk copy cursor issue fix bulk copy cursor issue Apr 28, 2017
@codecov-io
Copy link

codecov-io commented Apr 28, 2017

Codecov Report

Merging #270 into dev will increase coverage by 2.48%.
The diff coverage is 84.61%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev     #270      +/-   ##
============================================
+ Coverage     34.64%   37.12%   +2.48%     
- Complexity     1570     1648      +78     
============================================
  Files           102      102              
  Lines         23660    23694      +34     
  Branches       3873     3882       +9     
============================================
+ Hits           8197     8797     +600     
+ Misses        13847    13301     -546     
+ Partials       1616     1596      -20
Flag Coverage Δ Complexity Δ
#JDBC41 37% <84.61%> (+2.44%) 1640 <2> (+76) ⬆️
#JDBC42 36.98% <84.61%> (+2.53%) 1646 <2> (+88) ⬆️
Impacted Files Coverage Δ Complexity Δ
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java 45.99% <ø> (+7.04%) 0 <0> (ø) ⬇️
...m/microsoft/sqlserver/jdbc/SQLServerResultSet.java 26.78% <100%> (+0.03%) 183 <1> (-6) ⬇️
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java 48.05% <84.21%> (+1.8%) 204 <1> (+9) ⬆️
...om/microsoft/sqlserver/jdbc/JaasConfiguration.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...oft/sqlserver/jdbc/SQLServerParameterMetaData.java 27.39% <0%> (+0.21%) 32% <0%> (ø) ⬇️
...oft/sqlserver/jdbc/SQLServerCallableStatement.java 14.07% <0%> (+0.25%) 39% <0%> (ø) ⬇️
...rc/main/java/com/microsoft/sqlserver/jdbc/DDC.java 30.35% <0%> (+0.66%) 56% <0%> (ø) ⬇️
...m/microsoft/sqlserver/jdbc/SQLServerStatement.java 59.08% <0%> (+0.71%) 129% <0%> (ø) ⬇️
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab62a67...655c696. Read the comment docs.

@v-nisidh v-nisidh requested a review from Suraiya-Hameed May 2, 2017 23:12
// Create and send the initial command for bulk copy ("INSERT BULK ...").
tdsWriter = command.startRequest(TDS.PKT_QUERY);
String bulkCmd = createInsertBulkCommand(tdsWriter);
tdsWriter.writeString(bulkCmd);
Copy link
Contributor

Choose a reason for hiding this comment

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

As the bulkCmd is going to be same with and w/o cursors, let this be in SQLServerBulkCopy#doInsertBulk(as before) and not inside a while loop.

Copy link
Contributor Author

@xiangyushawn xiangyushawn May 9, 2017

Choose a reason for hiding this comment

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

With cursor, we need to send data row by row, so it has to be inside the while loop. Also, to bypass MARS, the sending must happen after the reading, which is goToNextRow()

Copy link
Contributor

Choose a reason for hiding this comment

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

sending 'bulkCmd' in loop is fine, but driver will generate the same insert bulk ..... command again and again in line 3208.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the thing is, we have to reset the tdsWriter after goToNextRow(), and createInsertBulkCommand requires the new tdsWriter, so, it has to be here, otherwise it fails when column is encrypted

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it! thanks for details Shwan!

@xiangyushawn xiangyushawn merged commit 75ecbcb into microsoft:dev May 9, 2017
@xiangyushawn xiangyushawn deleted the BulkCopy_Cursor_Issue branch May 9, 2017 19:27
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.

4 participants