-
Notifications
You must be signed in to change notification settings - Fork 122
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
feat: add support for Proto Columns to Connection API #2495
feat: add support for Proto Columns to Connection API #2495
Conversation
… fix autogenerated client side statement tests
...er/src/main/java/com/google/cloud/spanner/connection/ClientSideStatementValueConverters.java
Outdated
Show resolved
Hide resolved
...er/src/main/java/com/google/cloud/spanner/connection/ClientSideStatementValueConverters.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java
Show resolved
Hide resolved
...anner/src/main/java/com/google/cloud/spanner/connection/ConnectionStatementExecutorImpl.java
Show resolved
Hide resolved
...oud-spanner/src/main/resources/com/google/cloud/spanner/connection/ClientSideStatements.json
Show resolved
Hide resolved
...er/src/main/java/com/google/cloud/spanner/connection/ClientSideStatementValueConverters.java
Outdated
Show resolved
Hide resolved
...er/src/main/java/com/google/cloud/spanner/connection/ClientSideStatementValueConverters.java
Show resolved
Hide resolved
} | ||
|
||
/** Converter for converting String that take in file path as input to String */ | ||
static class ProtoDescriptorsFileConverter implements ClientSideStatementValueConverter<String> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We supporting reading from the file here? If Yes where should file be located just on resources or local path ?
We had similar discussion on #2277 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we are supporting reading from a file path given through a client side statement. This is needed to support ORM's and other frameworks. This discussion is available in the design doc.
Just a small brief, in PR#2277, we were treating file as a resource which brings in complications as they rely on class path. However, in this method we instead used File system to read from relative or absolute path.
...er/src/main/java/com/google/cloud/spanner/connection/ClientSideStatementValueConverters.java
Show resolved
Hide resolved
} | ||
|
||
@Override | ||
public byte[] convert(String value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also add a NonNull
annotation to the argument here. Similarly in other places wherever a null value is not expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These convert methods are not used by customers and is getting called from link. We generally use this annotation to warn users when they pass null
. But here it is used in code internally and as users don't use this function, I guess it is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is being called from the same package and not meant to be public to customers. Then, should we make the methods package-protected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These methods cannot be package-private, because they implement a method from an interface. The class itself is package-private, which protects the class and all its methods from being used by 'outside' users.
} | ||
|
||
/** | ||
* @return The proto descriptor that will be used with the next DDL statement (single or batch) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @return The proto descriptor that will be used with the next DDL statement (single or batch) | |
* @return The proto descriptor that will be used only with the next DDL statement (single or batch) |
google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/DdlClient.java
Show resolved
Hide resolved
} | ||
|
||
@Override | ||
public byte[] convert(String value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These methods cannot be package-private, because they implement a method from an interface. The class itself is package-private, which protects the class and all its methods from being used by 'outside' users.
Adds support for Proto Columns feature in the Connection API. This is needed to support Proto Columns in the JDBC driver PR-1252.