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 creating SQLServerDataTable being O(n^2) issue #514

Merged
merged 6 commits into from
Oct 11, 2017

Conversation

peterbae
Copy link
Contributor

Addresses issue #497.

@codecov-io
Copy link

codecov-io commented Sep 27, 2017

Codecov Report

Merging #514 into dev will increase coverage by 0.04%.
The diff coverage is 72.72%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev     #514      +/-   ##
============================================
+ Coverage     46.51%   46.56%   +0.04%     
+ Complexity     2218     2213       -5     
============================================
  Files           108      108              
  Lines         25312    25304       -8     
  Branches       4181     4176       -5     
============================================
+ Hits          11775    11782       +7     
+ Misses        11501    11494       -7     
+ Partials       2036     2028       -8
Flag Coverage Δ Complexity Δ
#JDBC41 46.32% <72.72%> (-0.06%) 2202 <3> (-7)
#JDBC42 46.39% <72.72%> (+0.14%) 2209 <3> (+3) ⬆️
Impacted Files Coverage Δ Complexity Δ
...c/main/java/com/microsoft/sqlserver/jdbc/Util.java 60.75% <100%> (+1.57%) 87 <2> (-1) ⬇️
...rc/main/java/com/microsoft/sqlserver/jdbc/TVP.java 45.22% <33.33%> (+0.03%) 27 <0> (ø) ⬇️
...m/microsoft/sqlserver/jdbc/SQLServerDataTable.java 58.73% <75%> (+0.66%) 23 <1> (ø) ⬇️
...va/com/microsoft/sqlserver/jdbc/ThreePartName.java 71.42% <0%> (-14.29%) 7% <0%> (-1%)
...om/microsoft/sqlserver/jdbc/ReaderInputStream.java 44.94% <0%> (-2.25%) 16% <0%> (ø)
...m/microsoft/sqlserver/jdbc/SQLServerResultSet.java 32.57% <0%> (-0.69%) 239% <0%> (-6%)
...m/microsoft/sqlserver/jdbc/SQLServerStatement.java 59.47% <0%> (+0.1%) 131% <0%> (+1%) ⬆️
...rc/main/java/com/microsoft/sqlserver/jdbc/dtv.java 63.24% <0%> (+0.12%) 0% <0%> (ø) ⬇️
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java 52.7% <0%> (+0.19%) 239% <0%> (ø) ⬇️
...ncurrentlinkedhashmap/ConcurrentLinkedHashMap.java 42.42% <0%> (+0.21%) 46% <0%> (ø) ⬇️
... and 3 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 05d0388...adf10ea. Read the comment docs.

Copy link

@alexnixon alexnixon left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple of minor comments.

import java.util.UUID;

public final class SQLServerDataTable {

int rowCount = 0;
int columnCount = 0;
Map<Integer, SQLServerDataColumn> columnMetadata = null;
Set<String> columnList = null;

Choose a reason for hiding this comment

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

I think naming this columNames would be clearer

* when a duplicate column exists
*/
private void checkDuplicateColumnName(String columnName) throws SQLServerException {
if (null != columnList) {

Choose a reason for hiding this comment

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

I'd rather an NPE than silently fail to check dupes if someone messes up null handling.

Better IMO would be defining the local variable like Set<String> columnNames = new HashSet<>(); and omitting this check, but I don't know if that'd violate the project's coding style.

@peterbae
Copy link
Contributor Author

peterbae commented Oct 5, 2017

@alexnixon Thanks for the review. I added the same logic for TVP since it was also applicable there, and made minor changes according to your suggestions.

@AfsanehR-zz
Copy link
Contributor

let's also add a test for checking duplicate column

@peterbae peterbae merged commit 7a0fe66 into microsoft:dev Oct 11, 2017
@peterbae peterbae deleted the SQLServerDataTableImprovement-497 branch October 11, 2017 15:59
@peterbae peterbae restored the SQLServerDataTableImprovement-497 branch October 11, 2017 16:14
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