Skip to content

feat(bigtable): add protoRowToRow conversion helper for TableShim - #20257

Merged
sushanb merged 1 commit into
googleapis:mainfrom
sushanb:feat/bigtable-proto-row-conversion
Jul 29, 2026
Merged

feat(bigtable): add protoRowToRow conversion helper for TableShim#20257
sushanb merged 1 commit into
googleapis:mainfrom
sushanb:feat/bigtable-proto-row-conversion

Conversation

@sushanb

@sushanb sushanb commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds an unexported protoRowToRow(*btpb.Row) Row helper to
table_shim.go plus a 13-case TestProtoRowToRow that pins the
wire-shape → Row-map conversion contract.

Staged for the follow-up change that swaps TableShim.session for a
proto-native session backend whose SessionReadRowResponse carries a
*btpb.Row rather than the already-converted bigtable.Row. Keeping
the converter + its tests in a separate patch means the follow-up
introduces exactly one new call site — the conversion itself is already
reviewed and pinned by tests.

Contract pinned by the test cases

Case Expected
nil input nil
empty Families nil (matches classic not-found)
family with no columns nil
column with zero cells family drops out entirely → nil
single family/column/cell happy path → correct Row
multiple cells per column preserves wire order (versioning)
multiple columns per family concatenate into same map key in wire order
multiple families separate map keys
same family name twice in Families appended, not deduped
Labels propagate through to ReadItem
TimestampMicros = 0 preserved
empty qualifier column name is "cf:"
empty Value ([]byte{}) preserved

The nil-on-empty behavior matches classic Table.ReadRow's row-not-found
signal — callers rely on row == nil for the not-found check, so
returning an empty-but-non-nil Row would be a subtle contract break.

Files

  • bigtable/table_shim.go — protoRowToRow helper (+49 LOC, unused in this patch)
  • bigtable/table_shim_test.go — TestProtoRowToRow (+115 LOC, 13 subtests)

Test plan

  • go build ./... clean
  • go vet ./... clean
  • go test ./bigtable/ -run 'TestProtoRowToRow|TestTableShim' -count=1 -v — 13/13 subtests pass

@sushanb
feat(bigtable): add protoRowToRow conversion helper for TableShim
7edb32a 
Adds an unexported protoRowToRow(*btpb.Row) Row helper in table_shim.go
plus 13-case TestProtoRowToRow that pins the wire-shape → Row-map
conversion contract.

Staged for the follow-up change that swaps TableShim.session for a
proto-native session backend whose SessionReadRowResponse carries a
*btpb.Row rather than the already-converted bigtable.Row. Keeping the
converter + its tests in a separate patch means the follow-up
introduces exactly one new call site — the conversion itself is
already reviewed and pinned.

Contract pinned by the test cases:
- nil / empty Families / family-with-no-columns / column-with-no-cells
  all return nil, matching classic Table.ReadRow's row-not-found
  signal (callers check `row == nil`).
- Cell order within a column, column order within a family, and
  family-append semantics when the same family name appears twice in
  Families are all preserved as wire order.
- Labels, empty qualifier ("cf:"), TimestampMicros=0, and
  empty-Value ([]byte{}) all pass through unchanged.

No behavior change: protoRowToRow has no caller in this patch. Test
coverage exercises every branch.
@sushanb
sushanb requested review from a team as code owners July 29, 2026 16:23
@product-auto-label product-auto-label Bot added the api: bigtable Issues related to the Bigtable API. label Jul 29, 2026
gemini-code-assist[bot]
gemini-code-assist Bot reviewed Jul 29, 2026
View reviewed changes

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request introduces the protoRowToRow helper function in bigtable/table_shim.go to convert proto-native Row objects into the classic bigtable.Row map format, along with comprehensive unit tests in bigtable/table_shim_test.go. The feedback suggests an optimization to append directly to the target map slice in protoRowToRow instead of allocating an intermediate slice, which reduces unnecessary allocations and improves performance.

Comment thread bigtable/table_shim.go
@sushanb
sushanb merged commit 1297143 into googleapis:main Jul 29, 2026
19 checks passed
@release-please release-please Bot mentioned this pull request Jul 29, 2026
Merged
@sushanb sushanb mentioned this pull request Jul 29, 2026
Merged
4 tasks done
sushanb added a commit that referenced this pull request Jul 29, 2026
@sushanb
refactor(bigtable): TableShim adopts proto-native session TableAPI (#…
fdcefed 
…20258)

## Summary

Follow-up to #20257 (protoRowToRow), which now has its sole consumer.
Swaps `TableShim`'s session backend from the classic `TableAPI` shape
(row string + `bigtable.Row`) to the internal
`session.TableAPI` shape (proto-native
`*btpb.SessionReadRow{Request,Response}` +
`*btpb.SessionMutateRow{Request,Response}`).

`TableShim` now owns the proto ↔ public-types translation so the
`internal/session` package can stay proto-native.

## Behavior

**`ReadRow`** — parses `ReadOption` using the classic `makeReadSettings`
shape (so filter + full-read-stats callback plumbing stays in one
place), builds a `*btpb.SessionReadRowRequest`, calls
`session.ReadRow`, feeds any `resp.Stats` through the
`WithFullReadStats` callback, and converts `resp.Row` via
`protoRowToRow`.

**`Apply`** — conditional mutations (`CheckAndMutateRow`) always route
to classic since the session vRPC has no `CheckAndMutateRow`
equivalent. Non-conditional mutations build a
`*btpb.SessionMutateRowRequest` and call `session.MutateRow`.

**`useSession()`** — nil-safe on both `session` and `diverter`, so
callers can wire a `TableShim` with `nil` session (as
`buildDivertible` does in #20256) and every routing decision falls
through to classic.

**`ReadRows` / `SampleRowKeys` / `ApplyBulk` / `ApplyReadModifyWrite`
always classic** — no session equivalent in the vRPC.

## API-visible signature change

`NewTableShim`'s `session` parameter type becomes `session.TableAPI` (an
internal-package interface). Existing in-repo callers pass `nil` for
session today; nil is the zero value of any interface, so the change
is source-compatible for those. External callers writing tests can
mock `session.TableAPI` directly — the new test file has an example
(`mockSessionTable`).

## Tests

Replaces the `mockTableAPI`-as-session pattern with a proto-native
`mockSessionTable`. New coverage:

- `TestTableShim_ReadRow_RoutesByDiverter` — classic when
  `SessionLoad=0.0`; session when `SessionLoad=1.0`; classic fallback
  when session is nil even with `SessionLoad=1.0`; session error
  propagation (no automatic fallback to classic on failure).
- `TestTableShim_Apply_ConditionalAlwaysClassic` — pins that
  conditional mutations bypass the session path.
- `TestTableShim_Apply_NonConditionalRoutesByDiverter` — pins that
  non-conditional mutations follow the diverter.
- `TestTableShim_ReadRows_AlwaysClassic`,
  `TestTableShim_SampleRowKeys_AlwaysClassic`,
  `TestTableShim_ApplyBulk_AlwaysClassic`,
  `TestTableShim_ApplyReadModifyWrite_AlwaysClassic` — pin the
  no-session-equivalent methods.
- `TestTableShim_NilSession_AllMethodsFallBackToClassic` — the
  classic-only wiring path (what #20256's `buildDivertible` will use
  until the session backend lands).
- `TestTableShim_SessionErrorNotRetriedOnClassic` — session-side
  failures surface as-is instead of silently falling back.

## Test plan

- [x] `go build ./...` clean
- [x] `go vet ./...` clean
- [x] `go test ./bigtable/ -run 'TestTableShim|TestProtoRowToRow'
-count=1 -v` — all pass
- [x] Live sandbox smoke against `autonomous-mote-782 / sushanb-uc1` —
classic path via `client.Open(...).Apply/ReadRow` still succeeds
end-to-end (session backend not exercised on this branch since it isn't
wired yet)

## Depends on

- #20257 (merged as `1297143a4f`) — `protoRowToRow` helper.

## Follows

Consumed by a future PR that wires an actual `session.TableAPI`
implementation
from the `internal/session` package into `Client.buildDivertible` (see
#20256 for the `buildDivertible` shape).
sushanb added a commit to sushanb/google-cloud-go that referenced this pull request Jul 29, 2026
@sushanb
refactor(bigtable): TableShim adopts proto-native session TableAPI
6f5091a 
Folds PR googleapis#20258 into this PR. TableShim's session backend changes from
the classic-shaped TableAPI (row string + bigtable.Row) to the
internal/session.TableAPI shape (proto-native
*btpb.SessionReadRow{Request,Response} + *btpb.SessionMutateRow
{Request,Response}). TableShim owns the proto ↔ public-types
translation so the session package can stay proto-native.

- ReadRow: parses ReadOptions via classic makeReadSettings/readSettings
  so every existing option (RowFilter, WithFullReadStats, etc.) works
  on the session path; then translates
  (row, filter) → SessionReadRowRequest, calls session.ReadRow, feeds
  resp.Stats through WithFullReadStats, and converts resp.Row via
  protoRowToRow (added in googleapis#20257, now on main).
- Apply: nil mutations and conditional mutations (CheckAndMutateRow)
  route to classic — CheckAndMutateRow has no session vRPC equivalent,
  and nil-m to classic surfaces the classic client's existing
  nil-handling error rather than panicking on m.ops here.
- useSession() helper is nil-safe on both session and diverter
  (replaces the earlier pickSession); callers can wire a TableShim
  with nil session and every routing decision falls through to
  classic.

Signature change (internal-only): NewTableShim's session parameter
type is now session.TableAPI (an internal package). In-repo callers
pass nil for session today; nil is the zero value of any interface,
so the change is source-compatible for those.

Tests: replaces the mockTableAPI-as-session pattern with a
proto-native mockSessionTable. Covers routing, nil-session fallback,
session error propagation, conditional-always-classic, and the
already-existing TestProtoRowToRow contract.

Closes PR googleapis#20258 (the standalone version); folded here so PR googleapis#20256's
review scope is one coherent unit — Diverter wiring + TableShim
proto-native shape together.
sushanb pushed a commit that referenced this pull request Aug 3, 2026
@release-please
chore(main): release bigtable 1.52.0 (#20207)
f6dae8d 
🤖 I have created a release *beep* *boop*
---


##
[1.52.0](bigtable/v1.51.0...bigtable/v1.52.0)
(2026-08-03)


### Features

* **bigtable:** Add AFE picker (Simple / LeastInFlight / LeastLatency)
([#20204](#20204))
([bcbf714](bcbf714))
* **bigtable:** Add ClientConfig.DisableSession to opt out of session
backend
([#20297](#20297))
([7ee5e44](7ee5e44))
* **bigtable:** Add getClientConfigDirectAccessChecker for session pools
([#20209](#20209))
([3b8d30a](3b8d30a))
* **bigtable:** Add NoOpChannelPrimer for session channel pools
([#20208](#20208))
([d055a8a](d055a8a))
* **bigtable:** Add per-AFE sessionList for the two-tier session pool
([#20224](#20224))
([dbf0c3f](dbf0c3f))
* **bigtable:** Add protoRowToRow conversion helper for TableShim
([#20257](#20257))
([1297143](1297143))
* **bigtable:** Add Session debug surface (observability fields +
methods)
([#20211](#20211))
([d8d3e16](d8d3e16))
* **bigtable:** Add Session lifecycle (Start, Close, ForceClose,
readLoop, heartBeatLoop)
([#20215](#20215))
([b9e53c6](b9e53c6))
* **bigtable:** Add Session struct + state machine
([#20117](#20117))
([09acbb3](09acbb3))
* **bigtable:** Add session.Config.EnableDebug to gate sessionz debug
state
([#20247](#20247))
([ce74c31](ce74c31))
* **bigtable:** Add SessionClient + SessionTable + lazyPool
([#20228](#20228))
([ab2c96c](ab2c96c))
* **bigtable:** Add SessionPoolImpl (two-tier pool + scaling + debug)
([#20225](#20225))
([683eda8](683eda8))
* **bigtable:** Rename session pool display to
<resource-id>-<PERM>
([#20248](#20248))
([35e146e](35e146e))
* **bigtable:** Route Client.Open()-returned *Table through the Diverter
([#20273](#20273))
([2b81c7d](2b81c7d))
* **bigtable:** State-based classification for abnormal session close
([#20243](#20243))
([f2905b7](f2905b7))
* **bigtable:** TableShim fallback to classic on session UNIMPLEMENTED
([#20269](#20269))
([36540af](36540af))
* **bigtable:** TTL-on-idle cache for per-resource session.TableAPI
([#20263](#20263))
([00b2a49](00b2a49))
* **bigtable:** Wire Diverter on Client and route Open* via TableShim
([#20256](#20256))
([b32fbd7](b32fbd7))


### Bug Fixes

* **bigtable:** AFE picker latency signal — subtract poolWait and
compute TransportLatency = wire − backend at source
([#20281](#20281))
([bb8c4d5](bb8c4d5))
* **bigtable:** Guard NewStream OnFinish against grpc-go double-fire
([#20295](#20295))
([b51da29](b51da29))
* **bigtable:** Real per-resource pool teardown on sessionTable.Close +
cache close-race gate
([#20264](#20264))
([599aea9](599aea9))
* **bigtable:** Session.durations / session.uptime — set explicit
histogram bucket boundaries
([#20276](#20276))
([97eee22](97eee22))
* **bigtable:** SessionTableHandle self-heals across cache eviction
([#20296](#20296))
([0dd98cd](0dd98cd))
* **bigtable:** Translate ctx errors to gRPC status on session vRPC
([#20299](#20299))
([0f3b2a5](0f3b2a5))
* **bigtable:** Treat PingAndWarm NotFound as a successful prime
([#20219](#20219))
([a1557ad](a1557ad))


### Performance Improvements

* **bigtable:** Delete periodic Tick loop; sizing is event-driven
([#20285](#20285))
([2c096bd](2c096bd))
* **bigtable:** Drop pick_lost_race debug tag from CheckoutSession hot
path
([#20280](#20280))
([bd0e400](bd0e400))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
@release-please release-please Bot mentioned this pull request Sep 3, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mutianf mutianf mutianf approved these changes
+1 more reviewer
@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: bigtable Issues related to the Bigtable API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sushanb @mutianf