Skip to content

Add Datatypes examples to Spanner sample.#1498

Merged
jsimonweb merged 5 commits into
masterfrom
add-spanner-datatypes-samples
Jul 10, 2019
Merged

Add Datatypes examples to Spanner sample.#1498
jsimonweb merged 5 commits into
masterfrom
add-spanner-datatypes-samples

Conversation

@jsimonweb
Copy link
Copy Markdown
Contributor

No description provided.

@jsimonweb jsimonweb requested a review from a team June 28, 2019 03:16
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 28, 2019
@jsimonweb jsimonweb assigned jsimonweb and unassigned jsimonweb Jun 28, 2019
@jsimonweb jsimonweb requested a review from lesv June 28, 2019 16:41
}

/** Class to contain venue sample data. */
static class Venue {
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'm ok w/ this for the sample, but best practice is implementing equals, hashcode, and clone.
(and yes, I rarely do that) See effective Java for details (I'll send an internal link)

Copy link
Copy Markdown
Contributor

@lesv lesv left a comment

Choose a reason for hiding this comment

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

A lot of nits on my part, nothing that should block, but a lot that you should consider.

List<Mutation> mutations = new ArrayList<>();
for (Venue venue : VENUES) {
mutations.add(
Mutation.newInsertBuilder("Venues")
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.

Ok, but it would be more readable as

.set("VenueId").to(venue.venueId)
.set("VenueName").to(venue.venueName)
...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

/** Class to contain venue sample data. */
static class Venue {

final long venueId;
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.

If the class is Venue, then why mention in on each member? Your going to type it a lot?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Leaving as is for now.

boolean exampleBool = true;
Statement statement =
Statement.newBuilder(
"SELECT VenueId, VenueName, OutdoorVenue FROM Venues \n"
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.

Does spanner require the \n ? if not, why is it here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed.

BaseEncoding.base64().encode("Hello World 1".getBytes()));
Statement statement =
Statement.newBuilder(
"SELECT VenueId, VenueName FROM Venues \n"
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.

ditto ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed.

.bind("outdoorVenue")
.to(exampleBool)
.build();
// We use a try-with-resource block to automatically release resources held by ResultSet.
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 has been in the language for a while, it really doesn't need to be called out every time it's used.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@jsimonweb jsimonweb requested a review from lesv June 29, 2019 00:17
Copy link
Copy Markdown
Contributor

@lesv lesv left a comment

Choose a reason for hiding this comment

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

LGTM - but consider testing impact.

id.getInstanceId().getInstance(),
id.getDatabase(),
Arrays.asList(
"CREATE TABLE Venues ("
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.

Probably ok for the sample, but might make testing difficult at times. (using a dynamic table name would be better)

@jsimonweb jsimonweb merged commit f62c7d9 into master Jul 10, 2019
@kurtisvg kurtisvg deleted the add-spanner-datatypes-samples branch November 15, 2019 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants