Skip to content

refactor(bigtable): TableShim adopts proto-native session TableAPI - #20258

Merged
sushanb merged 4 commits into
googleapis:mainfrom
sushanb:feat/bigtable-table-shim-proto-native
Jul 29, 2026
Merged

refactor(bigtable): TableShim adopts proto-native session TableAPI#20258
sushanb merged 4 commits into
googleapis:mainfrom
sushanb:feat/bigtable-table-shim-proto-native

Conversation

@sushanb

@sushanb sushanb commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

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 feat(bigtable): wire Diverter on Client and route Open* via TableShim #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

  • go build ./... clean
  • go vet ./... clean
  • go test ./bigtable/ -run 'TestTableShim|TestProtoRowToRow' -count=1 -v — all pass
  • 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

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).

Follow-up to the protoRowToRow helper in this PR (which now has its
sole consumer). Swaps TableShim's session backend from the classic
TableAPI shape (which took the caller's row string and returned a
bigtable.Row directly) to the internal/session.TableAPI shape (which
takes/returns *btpb.SessionReadRow{Request,Response} +
*btpb.SessionMutateRow{Request,Response}).

TableShim now owns the proto ↔ public-types translation:
- ReadRow: parses ReadOptions 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, then
  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() helper is nil-safe on both session and diverter, so
  callers can wire a TableShim with nil session (as buildDivertible
  does in the diverter-open PR) and every routing decision falls
  through to classic.

Signature change (API-visible): NewTableShim's session parameter type
becomes session.TableAPI (an internal package). Existing callers
inside this repo pass nil for session today; external callers writing
tests can construct their own session.TableAPI mock the same way the
new tests here do (see mockSessionTable in table_shim_test.go).

Tests: replaces the mockTableAPI-as-session pattern with a proto-native
mockSessionTable. Covers:
- ReadRow / Apply routing by diverter ratio (0.0 → classic, 1.0 → session)
- Fallback to classic when session is nil even with SessionLoad=1.0
- Session error propagation (no automatic fallback to classic on failure)
- Conditional Apply always classic
- ReadRows / SampleRowKeys / ApplyBulk / ApplyReadModifyWrite always classic
- All 13 protoRowToRow subtests (from the previous commit) still green

Test plan:
- go test ./bigtable/ -run 'TestTableShim|TestProtoRowToRow' -count=1 -v — all pass
- go vet ./... clean
@sushanb
sushanb requested review from a team as code owners July 29, 2026 16:53
@product-auto-label product-auto-label Bot added the api: bigtable Issues related to the Bigtable API. label Jul 29, 2026

@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 refactors TableShim to route requests between a classic gRPC data-plane and a proto-native session data-plane, updating the routing logic for ReadRow and Apply while delegating unsupported operations to the classic path. The test suite has been updated to verify routing, nil-safety, and error propagation. Feedback is provided regarding a potential nil pointer dereference in Apply when the mutation is nil, as well as a minor typo in a test name.

Comment thread bigtable/table_shim.go Outdated
Comment on lines +88 to +90
if m != nil && m.isConditional {
return t.classic.Apply(ctx, row, m, opts...)
}

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

If m is nil and t.useSession() is true, accessing m.ops on line 96 will cause a nil pointer dereference panic. Since m != nil is checked on line 88, it implies m can be nil. We should guard against m == nil and route it to the classic path (which preserves the classic client's error handling or behavior for nil mutations).

Suggested change
if m != nil && m.isConditional {
return t.classic.Apply(ctx, row, m, opts...)
}
if m == nil || m.isConditional {
return t.classic.Apply(ctx, row, m, opts...)
}

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.

Done in 32291cd — widened the gate to if m == nil || m.isConditional so nil mutations route to classic. The classic Apply path already handles nil-m by returning an error rather than panicking, so this preserves that error shape rather than introducing a new one here.

Comment thread bigtable/table_shim_test.go Outdated
},
{
name: "TimestampMicros=0 preserved (server-set-to-now semantic is caller's job)",
name: "TimestampMicros=0 preserved (server-server-set-to-now semantic is caller's job)",

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.

medium

There is a typo in the test name: server-server-set-to-now instead of server-set-to-now.

Suggested change
name: "TimestampMicros=0 preserved (server-server-set-to-now semantic is caller's job)",
name: "TimestampMicros=0 preserved (server-set-to-now semantic is caller's job)",

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.

Fixed in 32291cd.

Comment thread bigtable/table_shim.go
// conversion contract; the test suite pins every branch of that
// contract today (see TestProtoRowToRow).
//
// Contract: preserves wire order for cells within a column and columns

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.

keep this comment.

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.

Restored in 32291cd — the Contract: block (wire-order, nil-on-empty, same-family append-not-dedup, TestProtoRowToRow pointer) is back on the protoRowToRow doc.

Three review comments from PR googleapis#20258:

- gemini (high) — TableShim.Apply would NPE on m.ops (line 96) if the
  caller passed m == nil AND useSession() returned true. Widen the
  gate to `m == nil || m.isConditional` so nil mutations route to the
  classic path, where the classic client's existing nil-handling
  surfaces the same error it always has.
- sushanb — restore the "Contract:" doc block on protoRowToRow that
  documented wire-order preservation, nil-on-empty semantics, and the
  same-family-name append-not-dedup rule. My shortened docstring in the
  swap commit dropped it.
- gemini (medium) — typo in TestProtoRowToRow case name:
  "server-server-set-to-now" → "server-set-to-now".

No behavior change from the guard: m == nil never got past the classic
path before because Apply eventually dereferences m.ops there too, but
the classic path's dereference happens after a nil-check inside
Table.applyClassic → t.apply. Routing nil to classic just preserves
that exact error shape.
Comment thread bigtable/table_shim.go
}
req := &btpb.SessionReadRowRequest{
Key: []byte(row),
Filter: tmpReq.Filter,

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.

How does tmpReq gets the filter?

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.

// stats callback plumbing stays in one place.
tmpReq := &btpb.ReadRowsRequest{}
settings := makeReadSettings(tmpReq, 0)
for _, opt := range opts {
	opt.set(&settings)
}
req := &btpb.SessionReadRowRequest{
	Key:    []byte(row),
	Filter: tmpReq.Filter,
}

via the opts setting we use in classic path.

sushanb added 2 commits July 29, 2026 17:08
The tmpReq + settings pattern in ReadRow uses the classic-side
readSettings shape so every existing ReadOption works without
per-option branching. The mechanic isn't obvious from the code alone
— readSettings.req is a POINTER to tmpReq, so when RowFilter's set
method writes settings.req.Filter it's writing into tmpReq. That's
what makes the tmpReq.Filter copy on the next line meaningful.

Also flags the silent-drop behavior for ReadOptions whose target
field doesn't exist on SessionReadRowRequest (LimitRows → RowsLimit
being the concrete example today). Not a correctness issue for
ReadRow since it's single-row by construction, but a future
session-specific option would need to be read off `settings`
directly rather than through the proto.
@sushanb
sushanb merged commit fdcefed into googleapis:main Jul 29, 2026
19 checks passed
sushanb added a commit to sushanb/google-cloud-go that referenced this pull request Jul 29, 2026
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: bigtable Issues related to the Bigtable API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants