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.

Sign up for GitHub

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: support UNRECOGNIZED types + decode BYTES columns lazily #2219

Merged
merged 10 commits into from
Feb 2, 2023
Merged

feat: support UNRECOGNIZED types + decode BYTES columns lazily #2219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9123db5
perf: decode BYTES columns lazily
olavloite Jan 5, 2023
066f6dc
feat: support unrecognized types
olavloite Jan 11, 2023
dd73f39
feat: add support for lazy bytes arrays
olavloite Jan 12, 2023
4060866
Merge branch 'main' into lazy-decode-bytes
olavloite Jan 13, 2023
617120d
chore: cleanup
olavloite Jan 13, 2023
59d12eb
fix: unrecognized types with array element type should be considered …
olavloite Jan 13, 2023
f637f1e
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] Jan 13, 2023
e59edc7
chore: address review comments
olavloite Feb 2, 2023
fd7e852
Merge branch 'main' into lazy-decode-bytes
olavloite Feb 2, 2023
c849aff
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] Feb 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,20 +49,20 @@ If you are using Maven without BOM, add this to your dependencies:
If you are using Gradle 5.x or later, add this to your dependencies:

```Groovy
implementation platform('com.google.cloud:libraries-bom:26.4.0')
implementation platform('com.google.cloud:libraries-bom:26.5.0')

implementation 'com.google.cloud:google-cloud-spanner'
```
If you are using Gradle without BOM, add this to your dependencies:

```Groovy
implementation 'com.google.cloud:google-cloud-spanner:6.35.1'
implementation 'com.google.cloud:google-cloud-spanner:6.35.2'
```

If you are using SBT, add this to your dependencies:

```Scala
libraryDependencies += "com.google.cloud" % "google-cloud-spanner" % "6.35.1"
libraryDependencies += "com.google.cloud" % "google-cloud-spanner" % "6.35.2"
```

## Authentication
Expand Down
123 changes: 116 additions & 7 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractResultSet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,14 @@
import com.google.cloud.spanner.spi.v1.SpannerRpc;
import com.google.cloud.spanner.v1.stub.SpannerStubSettings;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.AbstractIterator;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Lists;
import com.google.common.util.concurrent.Uninterruptibles;
import com.google.protobuf.ByteString;
import com.google.protobuf.ListValue;
import com.google.protobuf.NullValue;
import com.google.protobuf.Value.KindCase;
import com.google.spanner.v1.PartialResultSet;
import com.google.spanner.v1.ResultSetMetadata;
Expand All @@ -55,23 +57,29 @@
import java.math.BigDecimal;
import java.util.AbstractList;
import java.util.ArrayList;
import java.util.Base64;
import java.util.BitSet;
import java.util.Collections;
import java.util.Iterator;
import java.util.LinkedList;
import java.util.List;
import java.util.Objects;
import java.util.concurrent.BlockingQueue;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.Executor;
import java.util.concurrent.LinkedBlockingQueue;
import java.util.concurrent.TimeUnit;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.stream.Collectors;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;

/** Implementation of {@link ResultSet}. */
abstract class AbstractResultSet<R> extends AbstractStructReader implements ResultSet {
private static final Tracer tracer = Tracing.getTracer();
private static final com.google.protobuf.Value NULL_VALUE =
com.google.protobuf.Value.newBuilder().setNullValue(NullValue.NULL_VALUE).build();

interface Listener {
/**
Expand Down Expand Up @@ -353,6 +361,79 @@ private boolean isMergeable(KindCase kind) {
}
}

static final class LazyByteArray implements Serializable {
private static final Base64.Encoder ENCODER = Base64.getEncoder();
private static final Base64.Decoder DECODER = Base64.getDecoder();
private final String base64String;
private transient AbstractLazyInitializer<ByteArray> byteArray;

LazyByteArray(@Nonnull String base64String) {
this.base64String = Preconditions.checkNotNull(base64String);
this.byteArray = defaultInitializer();
}

LazyByteArray(@Nonnull ByteArray byteArray) {
this.base64String =
ENCODER.encodeToString(Preconditions.checkNotNull(byteArray).toByteArray());
this.byteArray =
new AbstractLazyInitializer<ByteArray>() {
@Override
protected ByteArray initialize() {
return byteArray;
}
};
}

private AbstractLazyInitializer<ByteArray> defaultInitializer() {
return new AbstractLazyInitializer<ByteArray>() {
@Override
protected ByteArray initialize() {
return ByteArray.copyFrom(DECODER.decode(base64String));
}
};
}

private void readObject(java.io.ObjectInputStream in)
rajatbhatta marked this conversation as resolved.
Show resolved Hide resolved
throws IOException, ClassNotFoundException {
in.defaultReadObject();
byteArray = defaultInitializer();
}

ByteArray getByteArray() {
try {
return byteArray.get();
} catch (Throwable t) {
throw SpannerExceptionFactory.asSpannerException(t);
}
}

String getBase64String() {
return base64String;
}

@Override
public String toString() {
return getBase64String();
}

@Override
public int hashCode() {
return base64String.hashCode();
}

@Override
public boolean equals(Object o) {
if (o instanceof LazyByteArray) {
return lazyByteArraysEqual((LazyByteArray) o);
}
return false;
}

private boolean lazyByteArraysEqual(LazyByteArray other) {
return Objects.equals(getBase64String(), other.getBase64String());
}
}

static class GrpcStruct extends Struct implements Serializable {
private final Type type;
private final List<Object> rowData;
Expand Down Expand Up @@ -395,7 +476,11 @@ private Object writeReplace() {
builder.set(fieldName).to(Value.pgJsonb((String) value));
break;
case BYTES:
builder.set(fieldName).to((ByteArray) value);
builder
.set(fieldName)
.to(
Value.bytesFromBase64(
value == null ? null : ((LazyByteArray) value).getBase64String()));
break;
case TIMESTAMP:
builder.set(fieldName).to((Timestamp) value);
Expand Down Expand Up @@ -431,7 +516,17 @@ private Object writeReplace() {
builder.set(fieldName).toPgJsonbArray((Iterable<String>) value);
break;
case BYTES:
builder.set(fieldName).toBytesArray((Iterable<ByteArray>) value);
builder
.set(fieldName)
.toBytesArrayFromBase64(
value == null
? null
: ((List<LazyByteArray>) value)
.stream()
.map(
element ->
element == null ? null : element.getBase64String())
.collect(Collectors.toList()));
break;
case TIMESTAMP:
builder.set(fieldName).toTimestampArray((Iterable<Timestamp>) value);
Expand Down Expand Up @@ -511,7 +606,7 @@ private static Object decodeValue(Type fieldType, com.google.protobuf.Value prot
return proto.getStringValue();
case BYTES:
checkType(fieldType, proto, KindCase.STRING_VALUE);
return ByteArray.fromBase64(proto.getStringValue());
return new LazyByteArray(proto.getStringValue());
case TIMESTAMP:
checkType(fieldType, proto, KindCase.STRING_VALUE);
return Timestamp.parseTimestamp(proto.getStringValue());
Expand All @@ -526,6 +621,8 @@ private static Object decodeValue(Type fieldType, com.google.protobuf.Value prot
checkType(fieldType, proto, KindCase.LIST_VALUE);
ListValue structValue = proto.getListValue();
return decodeStructValue(fieldType, structValue);
case UNRECOGNIZED:
return proto;
default:
throw new AssertionError("Unhandled type code: " + fieldType.getCode());
}
Expand Down Expand Up @@ -634,7 +731,11 @@ protected String getPgJsonbInternal(int columnIndex) {

@Override
protected ByteArray getBytesInternal(int columnIndex) {
return (ByteArray) rowData.get(columnIndex);
return getLazyBytesInternal(columnIndex).getByteArray();
}

LazyByteArray getLazyBytesInternal(int columnIndex) {
return (LazyByteArray) rowData.get(columnIndex);
}

@Override
Expand All @@ -647,6 +748,10 @@ protected Date getDateInternal(int columnIndex) {
return (Date) rowData.get(columnIndex);
}

protected com.google.protobuf.Value getProtoValueInternal(int columnIndex) {
return (com.google.protobuf.Value) rowData.get(columnIndex);
}

@Override
protected Value getValueInternal(int columnIndex) {
final List<Type.StructField> structFields = getType().getStructFields();
Expand All @@ -671,13 +776,16 @@ protected Value getValueInternal(int columnIndex) {
case PG_JSONB:
return Value.pgJsonb(isNull ? null : getPgJsonbInternal(columnIndex));
case BYTES:
return Value.bytes(isNull ? null : getBytesInternal(columnIndex));
return Value.internalBytes(isNull ? null : getLazyBytesInternal(columnIndex));
case TIMESTAMP:
return Value.timestamp(isNull ? null : getTimestampInternal(columnIndex));
case DATE:
return Value.date(isNull ? null : getDateInternal(columnIndex));
case STRUCT:
return Value.struct(isNull ? null : getStructInternal(columnIndex));
case UNRECOGNIZED:
return Value.unrecognized(
isNull ? NULL_VALUE : getProtoValueInternal(columnIndex), columnType);
case ARRAY:
final Type elementType = columnType.getArrayElementType();
switch (elementType.getCode()) {
Expand Down Expand Up @@ -785,9 +893,10 @@ protected List<String> getPgJsonbListInternal(int columnIndex) {
}

@Override
@SuppressWarnings("unchecked") // We know ARRAY<BYTES> produces a List<ByteArray>.
@SuppressWarnings("unchecked") // We know ARRAY<BYTES> produces a List<LazyByteArray>.
protected List<ByteArray> getBytesListInternal(int columnIndex) {
return Collections.unmodifiableList((List<ByteArray>) rowData.get(columnIndex));
return Lists.transform(
(List<LazyByteArray>) rowData.get(columnIndex), l -> l == null ? null : l.getByteArray());
}

@Override
Expand Down
1 change: 0 additions & 1 deletion google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractStructReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,6 @@ public Date getDate(String columnName) {

@Override
public Value getValue(int columnIndex) {
checkNonNull(columnIndex, columnIndex);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a breaking change? We are modifying the contract here, I wonder if any customer is relying on this? I would imagine not, but double checking.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm... That's a good point. I don't think anyone would be relying on it, but you're right that it is a change of the contract. In this case we will always be returning a non-null value, even if the database value is NULL (we're returning a Value instance whose internal value is null and whose type is set to the type of the column), so it also a safe value to return. I think there are two (three) options here:

  1. Accept the small change in contract.
  2. Add an extra method named something like 'getValueOrNull`
  3. Do a major version bump (but I don't feel like this feature is worth that)

@thiagotnunes @rajatbhatta @ansh0l Any specific thoughts on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

1 makes sense to me as well. As discussed offline, the documentation for getValue also indicates that this method can be used with columns having a null value.

return getValueInternal(columnIndex);
}

Expand Down
Loading