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

655 Fix to return correct column type for spatial datatypes #657

Merged
merged 5 commits into from
Mar 23, 2018

Conversation

peterbae
Copy link
Contributor

Fixes issue #655. Also added test cases for it.

Also removed an unused import from Geometry/Geography classes.

…d return their own type code, not VARBYTE. also removing unused imports.
@peterbae
Copy link
Contributor Author

this appveyor failure is unrelated to this PR, it's a bug I found during internal testing that occurs only with SQL Server 2008. I will merge this PR after that issue has been solved.

@peterbae
Copy link
Contributor Author

Actually I'm going to use a workaround for this problem, it seems like it could take a while to solve.

@codecov-io
Copy link

codecov-io commented Mar 13, 2018

Codecov Report

Merging #657 into dev will decrease coverage by 0.05%.
The diff coverage is 57.14%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev     #657      +/-   ##
============================================
- Coverage     48.12%   48.07%   -0.06%     
+ Complexity     2578     2576       -2     
============================================
  Files           113      113              
  Lines         26574    26579       +5     
  Branches       4429     4432       +3     
============================================
- Hits          12790    12778      -12     
- Misses        11660    11671      +11     
- Partials       2124     2130       +6
Flag Coverage Δ Complexity Δ
#JDBC42 47.94% <57.14%> (ø) 2568 <0> (-2) ⬇️
#JDBC43 47.92% <57.14%> (-0.13%) 2574 <0> (-2)
Impacted Files Coverage Δ Complexity Δ
...in/java/com/microsoft/sqlserver/jdbc/Geometry.java 71.09% <ø> (ø) 33 <0> (ø) ⬇️
...n/java/com/microsoft/sqlserver/jdbc/Geography.java 69.53% <ø> (ø) 33 <0> (ø) ⬇️
...oft/sqlserver/jdbc/SQLServerResultSetMetaData.java 40.93% <57.14%> (-0.04%) 24 <0> (+1)
...om/microsoft/sqlserver/jdbc/ReaderInputStream.java 44.94% <0%> (-2.25%) 16% <0%> (-1%)
...rc/main/java/com/microsoft/sqlserver/jdbc/DDC.java 43.84% <0%> (-1.12%) 107% <0%> (ø)
...ncurrentlinkedhashmap/ConcurrentLinkedHashMap.java 38.54% <0%> (-0.86%) 42% <0%> (-1%)
...m/microsoft/sqlserver/jdbc/SQLServerResultSet.java 32.89% <0%> (-0.61%) 244% <0%> (-3%)
...rc/main/java/com/microsoft/sqlserver/jdbc/dtv.java 63.35% <0%> (-0.07%) 0% <0%> (ø)
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java 54.23% <0%> (+0.06%) 0% <0%> (ø) ⬇️
...n/java/com/microsoft/sqlserver/jdbc/DataTypes.java 78.01% <0%> (+0.16%) 5% <0%> (+1%) ⬆️
... 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 2039522...8ad8252. Read the comment docs.

@@ -125,6 +125,14 @@ public int getColumnType(int column) throws SQLServerException {
if ( SSType.SQL_VARIANT == typeInfo.getSSType()){
jdbcType = JDBCType.SQL_VARIANT;
}
if (SSType.UDT == typeInfo.getSSType()) {
if (typeInfo.getSSTypeName().equalsIgnoreCase("geometry")) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't really have a problem with this, but why are we doing type check in 3 different ways. VARIANT is being checked separately outside the switch, and now GEOMETRY/GEOGRAPHY will be checked by name. Can we maybe stick to one form? This Microsoft overview of spatial datatypes seems to imply that it's only available in SQL Server 2012 and later, so shouldn't this be in the isKatamaiOrLater() block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't stick to one form. If you debug this part of the code, you'll see that the getSSType will give SQL_VARIANT or UDT. If it's UDT, we need to look at the SSTypeName to find out if it's Geometry or Geography, so this is the only way to check for these datatypes.

The overview page mentions that new spatial features have been introduced in SQL Server 2012, but this does not mean that spatial datatypes are supported starting from 2012 - it's supported starting from 2008. so we need this check for 2008 and up.

Copy link
Member

Choose a reason for hiding this comment

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

Please create Constants for these strings and reference them here.

rene-ye
rene-ye previously approved these changes Mar 13, 2018
@cheenamalhotra cheenamalhotra added this to In progress in MSSQL JDBC Mar 14, 2018
@cheenamalhotra cheenamalhotra moved this from In progress to Under Peer Review in MSSQL JDBC Mar 14, 2018
@@ -125,6 +125,14 @@ public int getColumnType(int column) throws SQLServerException {
if ( SSType.SQL_VARIANT == typeInfo.getSSType()){
jdbcType = JDBCType.SQL_VARIANT;
}
if (SSType.UDT == typeInfo.getSSType()) {
if (typeInfo.getSSTypeName().equalsIgnoreCase("geometry")) {
Copy link
Member

Choose a reason for hiding this comment

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

Please create Constants for these strings and reference them here.

@cheenamalhotra cheenamalhotra added this to the 6.5.1 milestone Mar 23, 2018
@peterbae peterbae merged commit c1aa43b into microsoft:dev Mar 23, 2018
MSSQL JDBC automation moved this from Under Peer Review to Closed/Merged PRs Mar 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
MSSQL JDBC
  
Closed/Merged PRs
Development

Successfully merging this pull request may close these issues.

None yet

4 participants