Skip to content

bulk mode insert with NULL GUID value#1778

Merged
lilgreenbird merged 4 commits intomicrosoft:mainfrom
andreitokar:bulk-insert-null-guid
Apr 15, 2022
Merged

bulk mode insert with NULL GUID value#1778
lilgreenbird merged 4 commits intomicrosoft:mainfrom
andreitokar:bulk-insert-null-guid

Conversation

@andreitokar
Copy link
Copy Markdown
Contributor

Fix for #1777

VeryVerySpicy
VeryVerySpicy previously approved these changes Apr 7, 2022
Jeffery-Wasty
Jeffery-Wasty previously approved these changes Apr 7, 2022
tkyc
tkyc previously approved these changes Apr 8, 2022
@lilgreenbird lilgreenbird added this to the 11.1.1 milestone Apr 8, 2022
Copy link
Copy Markdown
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.

hi @andreitokar
thank you for the submission however could you please add a test for this?

@andreitokar
Copy link
Copy Markdown
Contributor Author

andreitokar commented Apr 9, 2022

Hi @lilgreenbird,
The test would be a trivial addition of data to some CSV file, i.e. BulkCopyCSVTestInput.csv, but unfortunately existing testing framework does not support GUID type, and I do not feel comfortable to add such support due to a lack of knowledge about various aspects of SqlType sub-classing (coercions etc).

@VeryVerySpicy
Copy link
Copy Markdown
Contributor

Hi @andreitokar
An easier way of adding this test would be to add it to BatchExecutionWithBulkCopy.java
Either as an addition to testNullOrEmptyColumns or as it's own test case.

@VeryVerySpicy
Copy link
Copy Markdown
Contributor

Something like
@Test public void testNullGuid () throws Exception { String valid = "insert into " + AbstractSQLGenerator.escapeIdentifier(tableName) + " (c1) values (?)"; try (Connection connection = PrepUtil.getConnection(connectionString + ";useBulkCopyForBatchInsert=true;"); SQLServerPreparedStatement pstmt = (SQLServerPreparedStatement) connection.prepareStatement(valid); Statement stmt = (SQLServerStatement) connection.createStatement();) { pstmt.setNull(1, microsoft.sql.Types.GUID); pstmt.executeBatch(); } }

@VeryVerySpicy VeryVerySpicy dismissed stale reviews from tkyc, Jeffery-Wasty, and themself via f02419c April 12, 2022 20:04
@ghost
Copy link
Copy Markdown

ghost commented Apr 12, 2022

CLA assistant check
All CLA requirements met.

@andreitokar
Copy link
Copy Markdown
Contributor Author

Thank you @v-zhangw . Of course, I could have done something like that, but just did not want to blow test code needlessly, since test for a dozen of other types is already in place, so I though, that the right way would be to include GUID support into the testing framework. But either way, it is a test.

Jeffery-Wasty
Jeffery-Wasty previously approved these changes Apr 14, 2022
tkyc
tkyc previously approved these changes Apr 14, 2022
@VeryVerySpicy VeryVerySpicy dismissed stale reviews from tkyc and Jeffery-Wasty via 5db1767 April 14, 2022 20:24
@lilgreenbird lilgreenbird merged commit dc190d5 into microsoft:main Apr 15, 2022
@lilgreenbird lilgreenbird changed the title bulk mode insert with NULL GUID value (#1777) bulk mode insert with NULL GUID value Apr 15, 2022
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.

5 participants