Skip to content

spanner-jdbc: Step 16 - JdbcResultSet - #5891

Merged
olavloite merged 9 commits into
googleapis:spanner-jdbcfrom
olavloite:spanner-jdbc-16-resultset
Jul 30, 2019
Merged

spanner-jdbc: Step 16 - JdbcResultSet#5891
olavloite merged 9 commits into
googleapis:spanner-jdbcfrom
olavloite:spanner-jdbc-16-resultset

Conversation

@olavloite

@olavloiteolavloite commented Jul 26, 2019

Copy link
Copy Markdown
Contributor

Adds java.sql.ResultSet implementation for Cloud Spanner. This is basically a wrapper around a com.google.cloud.spanner.ResultSet.

(The branch of this PR is based on step 13. Step 13 has been merged and this PR has been rebased.)

@googlebotgooglebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 26, 2019
@olavloite
olavloite requested a review from kolea2July 26, 2019 06:56

/** Extract {@link java.sql.Types} code from Spanner {@link Type}. */
static int extractColumnType(Type type) {
if (type.equals(Type.bool()))

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 either flip the equality here, or check for null and throw first.

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.

Same applies to further if statements in this file.

}

private static final String RESULTSET_NOT_SUPPORTED =
"Getting a resultset from an array is not supported";

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.

'ResultSet'

JdbcPreconditions.checkArgument(start >= 1, "Start position must be >= 1");
JdbcPreconditions.checkArgument(searchstr != null, "searchstr may not be null");
checkPosition(start);
int res = value.indexOf(searchstr, (int) start - 1);

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.

'searchStr'

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.

same applies to below usage of the parameter

@@ -0,0 +1,267 @@
/*

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 think this was already done in step 13. Does it need a rebase?

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.

I based this PR on step 13 to avoid build errors. Once 13 is merged, this one should be rebased.

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.

It has now been rebased.

@@ -0,0 +1,274 @@
/*

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.

think this was again already added

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.

I based this PR on step 13 to avoid build errors. Once 13 is merged, this one should be rebased.


@Test
public void testToString() {
assertNotNull(subject.toString());

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.

let's also assert what the string value is

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.

done

@codecov

codecovBot commented Jul 26, 2019

Copy link
Copy Markdown

Codecov Report

Merging #5891 into spanner-jdbc will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@ Coverage Diff @@## spanner-jdbc #5891 +/- ##
==================================================
- Coverage 46.12% 46.12% -0.01% 
Complexity 24189 24189 ==================================================
Files 2456 2456 Lines 262236 262236 Branches 29602 29602 ==================================================
- Hits 120952 120949 -3 - Misses 132173 132176 +3 
Partials 9111 9111
Impacted FilesCoverage ΔComplexity Δ
...gle/cloud/storage/testing/RemoteStorageHelper.java61.98% <0%> (-2.48%)9% <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 042a43e...ba8c626. Read the comment docs.

@olavloite
olavloiteforce-pushed the spanner-jdbc-16-resultset branch from d463224 to 68a099cCompareJuly 28, 2019 20:43
@olavloite
olavloite requested a review from kolea2July 28, 2019 21:06
@olavloite
olavloite merged commit c10fa70 into googleapis:spanner-jdbcJul 30, 2019
olavloite added a commit to olavloite/google-cloud-java that referenced this pull request Aug 5, 2019
* add jdbc types and type mappings for Cloud Spanner
* update CR year
* add jdbc ResultSet
* added null checks and javadoc and fixed formatting
* added missing checks for negative overflow and added test cases
* fixed camel case
* refactored if-statements into one, added actual check for toString
* removed formatter hints
* extracted to variable + added extra documentation
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