Skip to content

spanner-jdbc: Step 13 - Add JDBC types and type mappings for Cloud Spanner - #5877

Merged
olavloite merged 4 commits into
googleapis:spanner-jdbcfrom
olavloite:spanner-jdbc-13-jdbc-types
Jul 26, 2019
Merged

spanner-jdbc: Step 13 - Add JDBC types and type mappings for Cloud Spanner#5877
olavloite merged 4 commits into
googleapis:spanner-jdbcfrom
olavloite:spanner-jdbc-13-jdbc-types

Conversation

@olavloite

Copy link
Copy Markdown
Contributor

Adds JDBC Array, Blob and Clob data types and the default type mapping between Cloud Spanner, JDBC types and Java types.

(This PR will have a build error until step 12 is merged into the Spanner client library)

@olavloite
olavloite requested a review from kolea2July 25, 2019 12:41
@googlebotgooglebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 25, 2019
}

@Override
public Type getGoogleType() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe getSpannerType()?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Changed to getSpannerType()

if (targetType.equals(String.class)) return value.toString();

try {
if (targetType.equals(Boolean.class) && type.getCode() == Code.BOOL) return value;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would simplify this a bit. First check for targetType in one if, then the codes separately.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That is certainly a good idea.


static List<java.sql.Timestamp> toSqlTimestamps(List<Timestamp> timestamps) {
List<java.sql.Timestamp> res = new ArrayList<>(timestamps.size());
for (Timestamp timestamp : timestamps) res.add(toSqlTimestamp(timestamp));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this will likely break the code formatter

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Actually it was the code formatter that put it like that, but this is really not readable. I've added curly braces to all for loops in the class and reformatted.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

huh, surprising! That sounds good to me.

@codecov

codecovBot commented Jul 25, 2019

Copy link
Copy Markdown

Codecov Report

Merging #5877 into spanner-jdbc will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@ Coverage Diff @@## spanner-jdbc #5877 +/- ##
===============================================
Coverage 46.12% 46.12% Complexity 24187 24187 ===============================================
Files 2456 2456 Lines 262236 262236 Branches 29602 29602 ===============================================
Hits 120944 120944 Misses 132179 132179 Partials 9113 9113

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 2d7750f...dbcb883. Read the comment docs.

@olavloite
olavloite requested a review from kolea2July 25, 2019 15:41
return Date.fromYearMonthDay(1970, 1, 1);
}

@SuppressWarnings("deprecation")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what's deprecated here?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Specifically in this place: java.sql.Date#getYear(), java.sql.Date#getMonth() and java.sql.Date#getDate().

More in general: Many methods in java.sql.Date and java.sql.Timestamp have been deprecated since JDK version 1.1. They made some very bad choices regarding timezone handling in these classes, but they are still used as both return types and parameter types in the JDBC interfaces.

Cloud Spanner does not support storing timestamps with timezone information in the database, so the mismatch with these classes is a little bit less of a problem for Cloud Spanner than for many other DBMS'es.

@olavloite
olavloiteforce-pushed the spanner-jdbc-13-jdbc-types branch from 73eec3d to dbcb883CompareJuly 26, 2019 06:51
@olavloite
olavloite merged commit 4c8346c into googleapis:spanner-jdbcJul 26, 2019
olavloite added a commit to olavloite/google-cloud-java that referenced this pull request Aug 5, 2019
…anner (googleapis#5877)
* add jdbc types and type mappings for Cloud Spanner
* update CR year
* changed name to getSpannerType
* simplified if statements and added curly braces to for loops
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yesThis human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@olavloite@kolea2@googlebot