Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -318,10 +318,39 @@ public TargetId getTargetId() {
return targetId;
}

/**
* Returns true if this query identifies a single row that can be served by a point read. Supports
* two shapes: exactly one row key and no row ranges, or exactly one closed-closed row range whose
* start key equals its end key.
*/
@InternalApi
public boolean isSinglePointQuery() {
RowSet rows = this.builder.getRows();
int keyCount = rows.getRowKeysCount();
int rangeCount = rows.getRowRangesCount();
if (keyCount == 1 && rangeCount == 0) {
return true;
}
if (keyCount == 0 && rangeCount == 1) {
RowRange range = rows.getRowRanges(0);
return range.hasStartKeyClosed()
&& range.hasEndKeyClosed()
&& range.getStartKeyClosed().equals(range.getEndKeyClosed());
}
return false;
}

@InternalApi
public SessionReadRowRequest toSessionPointProto() {
Preconditions.checkState(
isSinglePointQuery(),
"Query must be a single-point read (one row key, or one closed-closed row range whose"
+ " start equals its end)");
RowSet rows = this.builder.getRows();
ByteString key =
rows.getRowKeysCount() > 0 ? rows.getRowKeys(0) : rows.getRowRanges(0).getStartKeyClosed();
return SessionReadRowRequest.newBuilder()
.setKey(this.builder.getRows().getRowKeysList().get(0))
.setKey(key)
.setFilter(this.builder.getFilter())
.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import com.google.api.gax.retrying.BasicResultRetryAlgorithm;
import com.google.api.gax.retrying.ExponentialRetryAlgorithm;
import com.google.api.gax.retrying.RetryAlgorithm;
import com.google.api.gax.retrying.RetrySettings;
import com.google.api.gax.retrying.RetryingExecutorWithContext;
import com.google.api.gax.retrying.ScheduledRetryingExecutor;
import com.google.api.gax.retrying.SimpleStreamResumptionStrategy;
Expand All @@ -37,6 +38,7 @@
import com.google.api.gax.rpc.RequestParamsExtractor;
import com.google.api.gax.rpc.ServerStreamingCallSettings;
import com.google.api.gax.rpc.ServerStreamingCallable;
import com.google.api.gax.rpc.StatusCode;
import com.google.api.gax.rpc.UnaryCallSettings;
import com.google.api.gax.rpc.UnaryCallable;
import com.google.api.gax.tracing.SpanName;
Expand Down Expand Up @@ -97,6 +99,7 @@
import com.google.cloud.bigtable.data.v2.stub.mutaterows.MutateRowsRetryingCallable;
import com.google.cloud.bigtable.data.v2.stub.readrows.FilterMarkerRowsCallable;
import com.google.cloud.bigtable.data.v2.stub.readrows.LargeReadRowsResumptionStrategy;
import com.google.cloud.bigtable.data.v2.stub.readrows.MaybePointReadCallable;
import com.google.cloud.bigtable.data.v2.stub.readrows.ReadRowsBatchingDescriptor;
import com.google.cloud.bigtable.data.v2.stub.readrows.ReadRowsResumptionStrategy;
import com.google.cloud.bigtable.data.v2.stub.readrows.ReadRowsRetryCompletedCallable;
Expand All @@ -119,6 +122,7 @@
import java.time.Duration;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import java.util.function.Function;
import javax.annotation.Nonnull;
Expand Down Expand Up @@ -259,11 +263,20 @@ public <RowT> ServerStreamingCallable<Query, RowT> createReadRowsCallable(
bigtableClientContext.getClientContext().getTracerFactory(),
span);

return traced.withDefaultCallContext(
bigtableClientContext
.getClientContext()
.getDefaultCallContext()
.withRetrySettings(perOpSettings.readRowsSettings.getRetrySettings()));
ServerStreamingCallable<Query, RowT> classic =
traced.withDefaultCallContext(
bigtableClientContext
.getClientContext()
.getDefaultCallContext()
.withRetrySettings(perOpSettings.readRowsSettings.getRetrySettings()));

return new MaybePointReadCallable<>(
classic,
createPointReadCallable(
rowAdapter,
"ReadRows",
perOpSettings.readRowsSettings.getRetrySettings(),
perOpSettings.readRowsSettings.getRetryableCodes()));
}

/**
Expand All @@ -281,13 +294,25 @@ public <RowT> ServerStreamingCallable<Query, RowT> createReadRowsCallable(
* </ul>
*/
public <RowT> UnaryCallable<Query, RowT> createReadRowCallable(RowAdapter<RowT> rowAdapter) {
return createPointReadCallable(
rowAdapter,
"ReadRow",
perOpSettings.readRowSettings.getRetrySettings(),
perOpSettings.readRowSettings.getRetryableCodes());
}

private <RowT> UnaryCallable<Query, RowT> createPointReadCallable(
RowAdapter<RowT> rowAdapter,
String spanName,
RetrySettings retrySettings,
Set<StatusCode.Code> retryableCodes) {
ClientContext clientContext = bigtableClientContext.getClientContext();

ServerStreamingCallable<ReadRowsRequest, RowT> readRowsCallable =
createReadRowsBaseCallable(
ServerStreamingCallSettings.<ReadRowsRequest, Row>newBuilder()
.setRetryableCodes(perOpSettings.readRowSettings.getRetryableCodes())
.setRetrySettings(perOpSettings.readRowSettings.getRetrySettings())
.setRetryableCodes(retryableCodes)
.setRetrySettings(retrySettings)
Comment thread
mutianf marked this conversation as resolved.
.setIdleTimeoutDuration(Duration.ZERO)
.setWaitTimeoutDuration(Duration.ZERO)
.build(),
Expand All @@ -302,16 +327,20 @@ public <RowT> UnaryCallable<Query, RowT> createReadRowCallable(RowAdapter<RowT>
BigtableUnaryOperationCallable<Query, RowT> classic =
new BigtableUnaryOperationCallable<>(
readRowCallable,

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.

high

There is a bug here where the locally created readRowsCallable (which is configured with the correct retrySettings, retryableCodes, and rowAdapter) is ignored. Instead, the class field readRowCallable is passed to BigtableUnaryOperationCallable. This causes the custom retry settings and the generic rowAdapter to be bypassed. Please update this to use readRowsCallable.

Suggested change
readRowCallable,
readRowsCallable,

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.

I don't think this review is right. readRows callable is converted to readRow callable with Transforming callable.

clientContext
.getDefaultCallContext()
.withRetrySettings(perOpSettings.readRowSettings.getRetrySettings()),
clientContext.getDefaultCallContext().withRetrySettings(retrySettings),
clientContext.getTracerFactory(),
getSpanName("ReadRow"),
getSpanName(spanName),
/* allowNoResponse= */ true);

UnaryCallSettings<?, ?> shimSettings =
perOpSettings.readRowSettings.toBuilder()
.setRetrySettings(retrySettings)
.setRetryableCodes(retryableCodes)
.build();

return bigtableClientContext
.getSessionShim()
.decorateReadRow(classic, rowAdapter, perOpSettings.readRowSettings);
.decorateReadRow(classic, rowAdapter, shimSettings);
}

private <ReqT, RowT> ServerStreamingCallable<ReadRowsRequest, RowT> createReadRowsBaseCallable(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
/*
* Copyright 2026 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.google.cloud.bigtable.data.v2.stub.readrows;

import com.google.api.core.ApiFuture;
import com.google.api.core.ApiFutureCallback;
import com.google.api.core.ApiFutures;
import com.google.api.core.InternalApi;
import com.google.api.gax.rpc.ApiCallContext;
import com.google.api.gax.rpc.ResponseObserver;
import com.google.api.gax.rpc.ServerStreamingCallable;
import com.google.api.gax.rpc.StreamController;
import com.google.api.gax.rpc.UnaryCallable;
import com.google.cloud.bigtable.data.v2.models.Query;
import com.google.common.util.concurrent.MoreExecutors;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicReference;

/**
* Routes ReadRows calls whose query identifies a single row through a unary point-read callable,
* letting them benefit from the same session-shim diversion as {@code ReadRow}. Queries that cannot
* be reduced to a point read fall through to the classic {@code ReadRows} callable.
*/
@InternalApi
public class MaybePointReadCallable<RowT> extends ServerStreamingCallable<Query, RowT> {
Comment thread
mutianf marked this conversation as resolved.
private final ServerStreamingCallable<Query, RowT> classic;
private final UnaryCallable<Query, RowT> pointReader;

public MaybePointReadCallable(
ServerStreamingCallable<Query, RowT> classic, UnaryCallable<Query, RowT> pointReader) {
this.classic = classic;
this.pointReader = pointReader;
}

@Override
public void call(Query request, ResponseObserver<RowT> responseObserver, ApiCallContext context) {
if (!request.isSinglePointQuery()) {
classic.call(request, responseObserver, context);
return;
}

AtomicBoolean cancelled = new AtomicBoolean();
AtomicReference<ApiFuture<RowT>> futureRef = new AtomicReference<>();

responseObserver.onStart(
new StreamController() {
@Override
public void cancel() {
cancelled.set(true);
ApiFuture<RowT> f = futureRef.get();
if (f != null) {
f.cancel(false);
}
}

@Override
public void disableAutoInboundFlowControl() {}

@Override
public void request(int count) {}
});

ApiFuture<RowT> future;
try {
future = pointReader.futureCall(request, context);
} catch (Throwable t) {
if (!cancelled.get()) {
responseObserver.onError(t);
}
return;
}
futureRef.set(future);
if (cancelled.get()) {
future.cancel(false);
}

ApiFutures.addCallback(
future,
new ApiFutureCallback<RowT>() {
@Override
public void onSuccess(RowT row) {
if (cancelled.get()) {
return;
}
if (row != null) {
try {
responseObserver.onResponse(row);
} catch (Throwable t) {
responseObserver.onError(t);
return;
}
}
responseObserver.onComplete();
}

@Override
public void onFailure(Throwable t) {
if (cancelled.get()) {
return;
}
responseObserver.onError(t);
}
},
MoreExecutors.directExecutor());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.google.bigtable.v2.RowFilter;
import com.google.bigtable.v2.RowRange;
import com.google.bigtable.v2.RowSet;
import com.google.bigtable.v2.SessionReadRowRequest;
import com.google.cloud.bigtable.data.v2.internal.ByteStringComparator;
import com.google.cloud.bigtable.data.v2.internal.NameUtil;
import com.google.cloud.bigtable.data.v2.internal.RequestContext;
Expand Down Expand Up @@ -966,4 +967,113 @@ public void testQueryReversed() {
assertThat(query.toProto(requestContext))
.isEqualTo(expectedReadFromTableProtoBuilder().setReversed(true).build());
}

@Test
public void isSinglePointQuery_singleRowKey() {
assertThat(Query.create(TABLE_ID).rowKey("k").isSinglePointQuery()).isTrue();
}

@Test
public void isSinglePointQuery_singleClosedRange() {
assertThat(
Query.create(TABLE_ID)
.range(ByteStringRange.unbounded().startClosed("k").endClosed("k"))
.isSinglePointQuery())
.isTrue();
}

@Test
public void isSinglePointQuery_emptyQuery() {
assertThat(Query.create(TABLE_ID).isSinglePointQuery()).isFalse();
}

@Test
public void isSinglePointQuery_multipleRowKeys() {
assertThat(Query.create(TABLE_ID).rowKey("a").rowKey("b").isSinglePointQuery()).isFalse();
}

@Test
public void isSinglePointQuery_rowKeyAndRange() {
assertThat(
Query.create(TABLE_ID)
.rowKey("a")
.range(ByteStringRange.unbounded().startClosed("a").endClosed("a"))
.isSinglePointQuery())
.isFalse();
}

@Test
public void isSinglePointQuery_multipleRanges() {
assertThat(
Query.create(TABLE_ID)
.range(ByteStringRange.unbounded().startClosed("a").endClosed("a"))
.range(ByteStringRange.unbounded().startClosed("b").endClosed("b"))
.isSinglePointQuery())
.isFalse();
}

@Test
public void isSinglePointQuery_closedOpenRange() {
assertThat(
Query.create(TABLE_ID)
.range(ByteStringRange.unbounded().startClosed("k").endOpen("k"))
.isSinglePointQuery())
.isFalse();
}

@Test
public void isSinglePointQuery_unequalClosedRange() {
assertThat(
Query.create(TABLE_ID)
.range(ByteStringRange.unbounded().startClosed("a").endClosed("b"))
.isSinglePointQuery())
.isFalse();
}

@Test
public void isSinglePointQuery_prefixRange() {
assertThat(Query.create(TABLE_ID).prefix("k").isSinglePointQuery()).isFalse();
}

@Test
public void toSessionPointProto_fromRowKey() {
Query query = Query.create(TABLE_ID).rowKey("the-key");
assertThat(query.toSessionPointProto())
.isEqualTo(
SessionReadRowRequest.newBuilder()
.setKey(ByteString.copyFromUtf8("the-key"))
.setFilter(RowFilter.getDefaultInstance())
.build());
}

@Test
public void toSessionPointProto_fromClosedRange() {
Query query =
Query.create(TABLE_ID)
.range(ByteStringRange.unbounded().startClosed("the-key").endClosed("the-key"));
assertThat(query.toSessionPointProto())
.isEqualTo(
SessionReadRowRequest.newBuilder()
.setKey(ByteString.copyFromUtf8("the-key"))
.setFilter(RowFilter.getDefaultInstance())
.build());
}

@Test
public void toSessionPointProto_preservesFilter() {
RowFilter filter = FILTERS.key().regex("regex").toProto();
Query query = Query.create(TABLE_ID).rowKey("the-key").filter(FILTERS.key().regex("regex"));
assertThat(query.toSessionPointProto())
.isEqualTo(
SessionReadRowRequest.newBuilder()
.setKey(ByteString.copyFromUtf8("the-key"))
.setFilter(filter)
.build());
}

@Test
public void toSessionPointProto_rejectsNonSinglePointQuery() {
Query query = Query.create(TABLE_ID).rowKey("a").rowKey("b");
assertThrows(IllegalStateException.class, query::toSessionPointProto);
}
}
Loading
Loading