Skip to content

Fix to ignore computed columns during BulkCopy#1562

Merged
lilgreenbird merged 3 commits intomicrosoft:mainfrom
alagrede:bulk-insert-computed-cols
Oct 12, 2023
Merged

Fix to ignore computed columns during BulkCopy#1562
lilgreenbird merged 3 commits intomicrosoft:mainfrom
alagrede:bulk-insert-computed-cols

Conversation

@alagrede
Copy link
Copy Markdown
Contributor

@alagrede alagrede commented Apr 8, 2021 •
edited
Loading

Hello,

I'm facing a problem with the bulk API for batch insert with computed columns.
I'm using an SQL table with computed columns like

create table myTable (
  id nvarchar(100) not null, 
  json nvarchar(max) not null,
  vcol1 as json_value([json], '$.vcol1')
)

My insert query uses for bulk:
insert into myTable (id, json) values (?, ?)
The current behavior of the driver is to reject the query with:
java.sql.BatchUpdateException: The column "vcol1" cannot be modified because it is either a computed column or is the result of a UNION operator.

After investigating and testing this fix on my project, there is no specific reason to reject the batch insert. The current Bulk code simply reads and compares all the columns found in the table.
I updated the current code to ignore irrelevant computed columns when inserting in bulk and it works like a charm.

Currently, the relevant documentation does not mention any limitation on the computed columns (so the documentation is still true):
https://docs.microsoft.com/en-us/sql/connect/jdbc/use-bulk-copy-api-batch-insert-operation?view=sql-server-ver15

I also added a new unit test to verify and demonstrate this use case: BatchExecutionWithBulkCopyTest#testComputedCols

EDIT: Code updated to be compatible with versions prior than SQL Server 2016

Sincerely

@ghost
Copy link
Copy Markdown

ghost commented Apr 8, 2021 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

@alagrede alagrede force-pushed the bulk-insert-computed-cols branch from 76b9ed8 to 7282212 Compare April 8, 2021 16:36
@peterbae
Copy link
Copy Markdown
Contributor

peterbae commented Apr 8, 2021

Thanks for the contribution @alagrede, the team will test this and get back to you with the results.

@lilgreenbird lilgreenbird added the Enhancement An enhancement to the driver. Lower priority than bugs. label Nov 30, 2021
@lilgreenbird
Copy link
Copy Markdown
Contributor

lilgreenbird commented Sep 20, 2023

hi @alagrede

Apologies it took so long to get back to you on this, this had fallen through the cracks..

We had discussed this within our team we are ok to add this to the driver could you please resolve the conflicts in your branch?

Also, we would rather not add server version checks in the driver, I think it should be fine to just check for is_computed since if it was not supported that column would not be returned.

Thank you for your contribution.

@alagrede alagrede force-pushed the bulk-insert-computed-cols branch from 7282212 to 0a69e78 Compare September 20, 2023 13:58
@alagrede
Copy link
Copy Markdown
Contributor Author

alagrede commented Sep 20, 2023

Hi @lilgreenbird
As requested, I resolved the conflict and remove the server version check.
Best regards,

@lilgreenbird lilgreenbird added this to the 12.5.0 milestone Sep 21, 2023
lilgreenbird
lilgreenbird previously approved these changes Sep 26, 2023
View reviewed changes
tkyc
tkyc previously approved these changes Sep 26, 2023
View reviewed changes
Jeffery-Wasty
Jeffery-Wasty previously approved these changes Oct 11, 2023
View reviewed changes
@tkyc tkyc dismissed stale reviews from Jeffery-Wasty, lilgreenbird, and themself via c2e3b3f October 12, 2023 20:12
@tkyc tkyc force-pushed the bulk-insert-computed-cols branch from 0a69e78 to c2e3b3f Compare October 12, 2023 20:12
@lilgreenbird lilgreenbird merged commit a3178ba into microsoft:main Oct 12, 2023
@Jeffery-Wasty Jeffery-Wasty mentioned this pull request Mar 11, 2024
Closed
@atul-delphix
Copy link
Copy Markdown

atul-delphix commented Feb 11, 2025 •
edited
Loading

@lilgreenbird & @alagrede ,

This fix cause NPE with computed column (if that computed column is NOT last column as per column-id), see the exception:

java.lang.NullPointerException: Cannot read field "columnName" because the return value of "java.util.Map.get(Object)" is null
at com.microsoft.sqlserver.jdbc.SQLServerBulkCopy.validateColumnMappings(SQLServerBulkCopy.java:1907)
at com.microsoft.sqlserver.jdbc.SQLServerBulkCopy.writeToServer(SQLServerBulkCopy.java:1668)
at com.microsoft.sqlserver.jdbc.SQLServerBulkCopy.writeToServer(SQLServerBulkCopy.java:630)

@Jeffery-Wasty
Copy link
Copy Markdown
Contributor

Jeffery-Wasty commented Feb 11, 2025

Hi @atul-delphix,

Can you open a new issue describing the above problem? Thanks.

@atul-delphix
Copy link
Copy Markdown

atul-delphix commented Feb 17, 2025

@Jeffery-Wasty created new issue #2606

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@Jeffery-Wasty Jeffery-Wasty Jeffery-Wasty approved these changes

@lilgreenbird lilgreenbird lilgreenbird left review comments

@tkyc tkyc tkyc left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Enhancement An enhancement to the driver. Lower priority than bugs.

Projects

MSSQL JDBC
Status: Closed/Merged PRs

Milestone

12.5.0

Development

Successfully merging this pull request may close these issues.

6 participants

@alagrede @peterbae @lilgreenbird @atul-delphix @Jeffery-Wasty @tkyc