Code improvements in SQLServerDataTable > 'internalAddRow()' by cheenamalhotra · Pull Request #538 · microsoft/mssql-jdbc

cheenamalhotra · 2017-11-01T23:51:30Z

Replaced with #990

codecov-io · 2017-11-02T00:06:36Z

Codecov Report

Merging #538 into dev will decrease coverage by 0.11%.
The diff coverage is 55.55%.

@@             Coverage Diff              @@
##                dev     #538      +/-   ##
============================================
- Coverage     48.17%   48.06%   -0.12%     
+ Complexity     2585     2584       -1     
============================================
  Files           114      114              
  Lines         26578    26574       -4     
  Branches       4453     4445       -8     
============================================
- Hits          12805    12772      -33     
- Misses        11634    11677      +43     
+ Partials       2139     2125      -14

Flag	Coverage Δ	Complexity Δ
#JDBC42	`47.97% <55.55%> (-0.01%)`	`2582 <17> (+7)`
#JDBC43	`47.98% <55.55%> (-0.07%)`	`2580 <17> (+1)`

Impacted Files	Coverage Δ	Complexity Δ
...m/microsoft/sqlserver/jdbc/SQLServerDataTable.java	`64.75% <55.55%> (+6.02%)`	`25 <17> (+2)`	⬆️
...rc/main/java/com/microsoft/sqlserver/jdbc/DDC.java	`43.4% <0%> (-1.57%)`	`106% <0%> (-1%)`
...n/java/com/microsoft/sqlserver/jdbc/Parameter.java	`61.92% <0%> (-0.84%)`	`63% <0%> (ø)`
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java	`54.9% <0%> (-0.43%)`	`0% <0%> (ø)`
...oft/sqlserver/jdbc/SQLServerPreparedStatement.java	`50.44% <0%> (-0.25%)`	`157% <0%> (ø)`
...ncurrentlinkedhashmap/ConcurrentLinkedHashMap.java	`39.18% <0%> (-0.22%)`	`44% <0%> (+1%)`
...m/microsoft/sqlserver/jdbc/SQLServerResultSet.java	`33.21% <0%> (-0.2%)`	`245% <0%> (-2%)`
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java	`52.48% <0%> (-0.2%)`	`238% <0%> (-1%)`
...rc/main/java/com/microsoft/sqlserver/jdbc/dtv.java	`63.29% <0%> (-0.12%)`	`0% <0%> (ø)`
...om/microsoft/sqlserver/jdbc/ReaderInputStream.java	`44.94% <0%> (ø)`	`16% <0%> (ø)`	⬇️

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 58f4f1d...3a7f7ae. Read the comment docs.

cogman · 2017-11-02T22:02:43Z

Pretty much exactly what I was thinking for the linked issue.

…boolean test for TVP.

cheenamalhotra · 2017-11-07T20:44:40Z

Fix added for #539 - Support for Bit values (1,0) added for Boolean datatype in TVP.

alexnixon · 2017-11-09T18:48:53Z

I think this introduces regressions, see my comment on #539

alexnixon · 2017-11-13T14:13:10Z

 	        switch (jdbcType) {
 	            case BIGINT:
-	                rowValues[pair.getKey()] = (val instanceof Number ? ((Number)val).longValue() : Long.parseLong(val.toString()));
+	                rowValues[pair.getKey()] = (val instanceof Number ? (Long)val : Long.parseLong(val.toString()));


If val is an Integer this will throw a ClassCastException. Same for most of the instanceof checks below

alexnixon · 2017-11-13T15:00:16Z

I think this would benefit from some more tests, perhaps similar to the examples I gave on #539.

As for the implementation, something like this is what I had in mind:

    private static long coerceToLong(Object val, String fieldTypeName) {
        if (val instanceof Byte) {
            return ((Byte) val).longValue();
        } else if (val instanceof Short) {
            return ((Short)val).longValue();
        } else if (val instanceof Integer) {
            return ((Integer)val).longValue();
        } else if (val instanceof Long) {
            return (Long) val;
        } else if (val instanceof BigDecimal) {
            return ((BigDecimal)val).longValueExact();
        } else if (val instanceof BigInteger) {
            return ((BigInteger)val).longValueExact();
        } else if (val instanceof Float || val instanceof Double) {
            throw new IllegalArgumentException(String.format("%s fields cannot be set with floating point numbers",
                    fieldTypeName));
        } else {
            throw new IllegalArgumentException(String.format("%s fields cannot be set with object %s of type %s",
                    fieldTypeName, val, val.getClass().getName()));
        }
    }

    private static boolean coerceToBoolean(Object val, String fieldTypeName) {
        if (val instanceof Boolean) {
            return (Boolean)val;
        } else {
            long l = coerceToLong(val, fieldTypeName);
            if (l == 0) {
                return false;
            } else if (l == 1) {
                return true;
            } else {
                throw new IllegalArgumentException(String.format("Cannot coerce value %s of type %s to %s", val,
                        val.getClass().getName(), fieldTypeName));
            }
        }
    }

    private static short coerceToShort(Object val, String fieldTypeName) {
        if (val instanceof Short) {
            return (Short)val;
        } else {
            Long l = coerceToLong(val, fieldTypeName);
            if (l.shortValue() == l) {
                return l.shortValue();
            } else {
                throw new IllegalArgumentException(String.format("Cannot coerce value %s of type %s to %s", val,
                        val.getClass().getName(), fieldTypeName));
            }
        }
    }

    private static int coerceToInt(Object val, String fieldTypeName) {
        if (val instanceof Integer) {
            return (Integer)val;
        } else {
            Long l = coerceToLong(val, fieldTypeName);
            if (l.intValue() == l) {
                return l.intValue();
            } else {
                throw new IllegalArgumentException(String.format("Cannot coerce value %s of type %s to %s", val,
                        val.getClass().getName(), fieldTypeName));
            }
        }
    }

And likewise for other types. Then call these from the jdbcType switch, eg:

switch (jdbcType) {
  [ ... ]
  case BIT:
    rowValues[pair.getKey()] = coerceToBoolean(val, "BIT");
    break;
  [ ... ]
  case SMALLINT:
    rowValues[pair.getKey()] = coerceToShort(val, "SMALLINT");
    break;
  case TINYINT:
    rowValues[pair.getKey()] = coerceToShort(val, "TINYINT");
    break;
}

As a disclaimer I haven't tested this.

cheenamalhotra · 2017-11-14T20:26:27Z

Hi @alexnixon Thanks for your suggestions above. I agree to check for object instance for specific types for casting, but with this code, there are 2 downsides:

Support for String datatype will be gone for setting all numeric types. Is that intended?
(Will be a breaking change for support to String values in TVP)
If not, I would like to add another layer of conversion from String values to support String inputs.
There will be an extra data conversion to long and then to desired type again (for else cases) - can anything be done to prevent that?
(Not a major impact, just an observation)

I am still investigating on changes, and will let you know if I find some gaps. Appreciate your involvements, looking forward to your comments. Since the PR is still in progress, we do want to make sure we cover all scenarios and don't break any existing support before merging this change.

alexnixon · 2017-11-16T14:28:58Z

Thanks for the reply @cheenamalhotra

Indeed it would break support for strings. If I was designing from scratch I would omit it, but I understand you might want to keep that for backwards compatibility. So I guess that's a choice for the project maintainer (which might be you?).

But if it is to be included I would still argue for being strict, i.e. ensure it throws an exception for out-of-range values and decimals.

I think the time/memory of an extra long conversion would be negligible, especially given we're about to write to a database, I just wrote the example code that way as it's shorter.

TheConstructor · 2018-04-05T12:08:32Z

+	        int nValueLen;
+	        switch (jdbcType) {
+	            case BIGINT:
+	                rowValues[pair.getKey()] = (val instanceof Number ? (Long)val : Long.parseLong(val.toString()));


Should this really check for Number and then cast to Long? I would either change to instanceof Long or use ((Number)val).longValue() (which may introduce an unnecessary reboxing if val is Long)

Code improvements in SQLServerDataTable > 'internalAddRow()'

4fd2ed9

cheenamalhotra mentioned this pull request Nov 1, 2017

SQLServerDataTable should not call toString on all objects #531

Closed

cheenamalhotra requested a review from AfsanehR-zz November 2, 2017 22:26

gordthompson mentioned this pull request Nov 4, 2017

Setting a BIT field to 1 with TVPs results in a 0 #539

Closed

cheenamalhotra added 2 commits November 7, 2017 12:07

Adding support for 0,1 input parsing to Boolean TVP datatype + Added …

7f62d7f

…boolean test for TVP.

Fix for Test issue

5430dfa

Changes done in PR to resolve regression issues.

0de716d

alexnixon reviewed Nov 13, 2017

View reviewed changes

cheenamalhotra added the Work in Progress The pull request is a work in progress label Nov 15, 2017

Merge branch 'dev' into explicitConversions

3a33022

TheConstructor reviewed Apr 5, 2018

View reviewed changes

cheenamalhotra added 3 commits April 20, 2018 09:31

Merge branch 'dev' into explicitConversions

15d2377

Merge branch 'dev' into explicitConversions

3ee4002

Merge branch 'dev' into explicitConversions

3a7f7ae

ulvii mentioned this pull request Jun 12, 2018

Use Bulk Copy API for batch insert operation #686

Merged

cheenamalhotra force-pushed the explicitConversions branch from 1ff9ea2 to 3a7f7ae Compare March 14, 2019 19:52

cheenamalhotra mentioned this pull request Mar 14, 2019

Performance | Improve performance of SQLServerDataTable.internalAddRow() function #990

Merged

cheenamalhotra closed this Mar 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code improvements in SQLServerDataTable > 'internalAddRow()'#538

Code improvements in SQLServerDataTable > 'internalAddRow()'#538
cheenamalhotra wants to merge 8 commits intomicrosoft:devfrom
cheenamalhotra:explicitConversions

cheenamalhotra commented Nov 1, 2017 •

edited

Loading

Uh oh!

codecov-io commented Nov 2, 2017 •

edited

Loading

Uh oh!

cogman commented Nov 2, 2017

Uh oh!

cheenamalhotra commented Nov 7, 2017

Uh oh!

alexnixon commented Nov 9, 2017

Uh oh!

alexnixon Nov 13, 2017

Uh oh!

alexnixon commented Nov 13, 2017 •

edited

Loading

Uh oh!

cheenamalhotra commented Nov 14, 2017 •

edited

Loading

Uh oh!

alexnixon commented Nov 16, 2017

Uh oh!

TheConstructor Apr 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

cheenamalhotra commented Nov 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Nov 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cogman commented Nov 2, 2017

Uh oh!

cheenamalhotra commented Nov 7, 2017

Uh oh!

alexnixon commented Nov 9, 2017

Uh oh!

alexnixon Nov 13, 2017

Choose a reason for hiding this comment

Uh oh!

alexnixon commented Nov 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cheenamalhotra commented Nov 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexnixon commented Nov 16, 2017

Uh oh!

TheConstructor Apr 5, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cheenamalhotra commented Nov 1, 2017 •

edited

Loading

codecov-io commented Nov 2, 2017 •

edited

Loading

alexnixon commented Nov 13, 2017 •

edited

Loading

cheenamalhotra commented Nov 14, 2017 •

edited

Loading