Skip to content

Fix | Added ISQLServerBulkData to remove implementation details from ISQLServerBulkRecord#1099

Merged
ulvii merged 9 commits intomicrosoft:devfrom
jonfreedman:dev
Oct 4, 2019
Merged

Fix | Added ISQLServerBulkData to remove implementation details from ISQLServerBulkRecord#1099
ulvii merged 9 commits intomicrosoft:devfrom
jonfreedman:dev

Conversation

@jonfreedman
Copy link
Copy Markdown
Contributor

@jonfreedman jonfreedman commented Jun 28, 2019

New ISQLServerBulkData interface which extracts the read-only methods from ISQLServerBulkRecord

Fixes #1052

@msftclas
Copy link
Copy Markdown

msftclas commented Jun 28, 2019 •
edited
Loading

CLA assistant check
All CLA requirements met.

@jonfreedman
Copy link
Copy Markdown
Contributor Author

jonfreedman commented Jun 28, 2019

@cheenamalhotra please take a look, thanks

@jonfreedman
Copy link
Copy Markdown
Contributor Author

jonfreedman commented Jun 28, 2019

Looks like the test failure is spurious - hit a deadlock

@ulvii
Copy link
Copy Markdown
Contributor

ulvii commented Jul 2, 2019

Hi @jonfreedman,
Thanks for the contribution, the team will review the PR.

@ulvii ulvii added this to the 7.5.0 milestone Aug 12, 2019
rene-ye
rene-ye reviewed Aug 19, 2019
View reviewed changes
Comment thread src/test/java/com/microsoft/sqlserver/jdbc/bulkCopy/BulkCopyTestUtil.java Outdated
peterbae
peterbae reviewed Aug 22, 2019
View reviewed changes
Comment thread src/main/java/com/microsoft/sqlserver/jdbc/ISQLServerBulkData.java Outdated
peterbae
peterbae reviewed Aug 22, 2019
View reviewed changes
Comment thread src/main/java/com/microsoft/sqlserver/jdbc/ISQLServerBulkRecord.java
peterbae
peterbae reviewed Aug 22, 2019
View reviewed changes
Comment thread src/main/java/com/microsoft/sqlserver/jdbc/ISQLServerBulkData.java
@peterbae
Copy link
Copy Markdown
Contributor

peterbae commented Aug 22, 2019

Looks like there's some conflicts, but other than that, the PR looks good to me.

@jonfreedman
Copy link
Copy Markdown
Contributor Author

jonfreedman commented Aug 23, 2019

@peterbae I have merged from upstream

@peterbae
Copy link
Copy Markdown
Contributor

peterbae commented Aug 27, 2019

Thanks @jonfreedman, I also noticed that we have:

bulkCopy.writeToServer((ISQLServerBulkRecord) fileRecord);

in BulkCopyCSVTest.java line 169. Could you remove the cast to ISQLServerBulkRecord?

@jonfreedman
Copy link
Copy Markdown
Contributor Author

jonfreedman commented Aug 28, 2019

@peterbae I've removed the cast, didn't find any others

@jonfreedman
Copy link
Copy Markdown
Contributor Author

jonfreedman commented Sep 26, 2019

Can this be merged?

@rene-ye
Copy link
Copy Markdown
Member

rene-ye commented Sep 26, 2019

Hi @jonfreedman, the team is contemplating whether to merge this for the next preview or not. We're trying to decide whether we want to apply a bandaid fix or just break the public apis and change the interfaces completely. We will merge/close this PR after discussions.

@jonfreedman
Copy link
Copy Markdown
Contributor Author

jonfreedman commented Sep 29, 2019

It's not really clear to me why you can't do both. You can see that this pr has been open for 3 months, based on an issue from 5 months ago. At this point all it really accomplishes is that I can delete some code that I've been running in production for almost 6 months. Are you saying that if you don't merge this then the next release will be rewritten with breaking changes, or that the 'fix' will happen at some later date?

@ulvii
Copy link
Copy Markdown
Contributor

ulvii commented Oct 2, 2019

Hi @jonfreedman,
We decided to accept your suggestion, apologies for the delay. Would you mind if we make a few minor changes to your branch?

@jonfreedman
Copy link
Copy Markdown
Contributor Author

jonfreedman commented Oct 2, 2019

That's great, please go ahead and change whatever you need to

@ulvii
Copy link
Copy Markdown
Contributor

ulvii commented Oct 3, 2019

Hi @jonfreedman,
I created a PR against your fork, which updates the branch from dev, adds @deprecated tag to ISQLServerBulkRecord, formats the files and some other minor changes. Please merge it to proceed with the review process.

@jonfreedman
Merge pull request #1 from ulvii/PR1099
8b05d47 
Added @deprecated tag to ISQLServerBulkRecord
@ulvii ulvii changed the title New ISQLServerBulkData interface Fix | Added ISQLServerBulkData to remove implementation details from ISQLServerBulkRecord (#1099) Oct 4, 2019
@ulvii ulvii changed the title Fix | Added ISQLServerBulkData to remove implementation details from ISQLServerBulkRecord (#1099) Fix | Added ISQLServerBulkData to remove implementation details from ISQLServerBulkRecord Oct 4, 2019
@ulvii ulvii merged commit 02da869 into microsoft:dev Oct 4, 2019
@ulvii
Copy link
Copy Markdown
Contributor

ulvii commented Oct 4, 2019

The PR is merged. Thanks for the contribution @jonfreedman.

ulvii added a commit that referenced this pull request Oct 16, 2019
@ulvii
Release | Merge dev to master for v8.1.0-preview (#1161)
e88884c 
* Release | Update SNAPSHOT for 7.5.0-preview release (#1129)

* Release | Update driver version

* Release| Add pom and gradle files

* Fix | Remove extra spaces in SQLServerDatabaseMetaData.getNumericFunctions()/SQLServerDatabaseMetaData.getStringFunctions() return values (#1117)

* Tests | Improve assertions in ComparisonUtil (#1100)

* ComparisonUtil : replace assertTrue with assertEquals

Failure message should include the actual/expected values and that is what assertEquals does.

* Corrected BigDecimal comparison

Scale difference should be ignored.

* Fix | Add list of trusted endpoints for AKV (#1130)

* added list of trusted endpoints

* rename

* Update issue templates (#1148)

* Fix getImportedKeys() returning duplicate rows if multiple FKs have the same name  (#1092)

* fixed for github #1091 dup rows

* Fix STAsBinary returning null for a single point (#1074)

* populate wkb for point

* fix stasbinary issue

* change variable name

* apply same variable name change

* Performance | Improved performance of column name string lookups (#1066)

* Fix | Made column name lookup more performant

* Fix | Spacing

* Fix | Changed arraylist to maps

* Fix | Add newline

* Add | Caching for previously retrieved columns

* Fix | Formatting

* Fix | Variable issue

* Fix | null order

* Fix | part 2

* Revert | Integer changes

* Fix | Trim retrieved column name

* Fix | Clear column names whenever ResultSet cursor is moved

* Revert "Fix | Clear column names whenever ResultSet cursor is moved"

This reverts commit db42d44.

* Add | Implement hashCode() and equals() APIs for SQLServerDataTable and SQLServerDataColumn (#1146)

* Add | Add hashCode()/equals() methods to SQLServerDataColumn and SQLServerDataTable

* Fix issue with truststore password being removed too early for XA transaction (#1133)

Fix | Fix issue with truststore password being removed too early for XA transaction

* Fix | SQLServerDatabaseMetada.getColumns not escaping wildcard (#1138)

* Feature | Introduce JAVA 13 Support (#1151)

* JDK 13 | Remove jre12 from pom file

* JDK 13 | Update Gradle build file

* JDK 13 | Update Azure-pipelines

* Test | Updated SQL Server from 2008R2 to 2012 in CI (#1153)

* Performance | Disabled pattern matching when using CallableStatements and SQLServerParameterMetaData (#1149)

* Fix | Added ISQLServerBulkData to remove implementation details from ISQLServerBulkRecord (#1099)

* Feature | Added support for Always Encrypted with Secure Enclaves (#1155)

* Release | 8.1.0-preview release changes (#1158)
@ulvii ulvii mentioned this pull request Oct 16, 2019
Merged
ulvii pushed a commit that referenced this pull request Mar 3, 2020
@jonfreedman @ulvii
Fix | Added ISQLServerBulkData to remove implementation details from …
b6f188d 
…ISQLServerBulkRecord (#1099)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@rene-ye rene-ye rene-ye approved these changes

@lilgreenbird lilgreenbird lilgreenbird approved these changes

@peterbae peterbae peterbae approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@rene-ye rene-ye

@lilgreenbird lilgreenbird

Labels

None yet

Projects

None yet

Milestone

7.5.0

Development

Successfully merging this pull request may close these issues.

[FEATURE REQUEST] Remove set / add methods from ISQLServerBulkRecord

6 participants

@jonfreedman @msftclas @ulvii @peterbae @rene-ye @lilgreenbird