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 to ignore computed columns during BulkCopy #1562

Merged
merged 3 commits into from
Oct 12, 2023

Conversation

alagrede
Copy link
Contributor

@alagrede alagrede commented Apr 8, 2021

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

ghost commented Apr 8, 2021

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
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
Contributor

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
Contributor Author

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
tkyc
tkyc previously approved these changes Sep 26, 2023
Jeffery-Wasty
Jeffery-Wasty previously approved these changes Oct 11, 2023
@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
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement An enhancement to the driver. Lower priority than bugs.
Projects
Status: Closed/Merged PRs
Development

Successfully merging this pull request may close these issues.

5 participants