refactor(bigtable): TableShim adopts proto-native session TableAPI - #20258
Conversation
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
There was a problem hiding this comment.
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.
| if m != nil && m.isConditional { | ||
| return t.classic.Apply(ctx, row, m, opts...) | ||
| } |
There was a problem hiding this comment.
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).
| 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...) | |
| } |
There was a problem hiding this comment.
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.
| }, | ||
| { | ||
| 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)", |
There was a problem hiding this comment.
| // 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 |
There was a problem hiding this comment.
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.
| } | ||
| req := &btpb.SessionReadRowRequest{ | ||
| Key: []byte(row), | ||
| Filter: tmpReq.Filter, |
There was a problem hiding this comment.
How does tmpReq gets the filter?
There was a problem hiding this comment.
// 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.
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.
This reverts commit f77db05.
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.
Summary
Follow-up to #20257 (protoRowToRow), which now has its sole consumer.
Swaps
TableShim's session backend from the classicTableAPIshape(row string +
bigtable.Row) to the internalsession.TableAPIshape (proto-native*btpb.SessionReadRow{Request,Response}+*btpb.SessionMutateRow{Request,Response}).TableShimnow owns the proto ↔ public-types translation so theinternal/sessionpackage can stay proto-native.Behavior
ReadRow— parsesReadOptionusing the classicmakeReadSettingsshape (so filter + full-read-stats callback plumbing stays in one
place), builds a
*btpb.SessionReadRowRequest, callssession.ReadRow, feeds anyresp.Statsthrough theWithFullReadStatscallback, and convertsresp.RowviaprotoRowToRow.Apply— conditional mutations (CheckAndMutateRow) always routeto classic since the session vRPC has no
CheckAndMutateRowequivalent. Non-conditional mutations build a
*btpb.SessionMutateRowRequestand callsession.MutateRow.useSession()— nil-safe on bothsessionanddiverter, socallers can wire a
TableShimwithnilsession (asbuildDivertibledoes in #20256) and every routing decision fallsthrough to classic.
ReadRows/SampleRowKeys/ApplyBulk/ApplyReadModifyWritealways classic — no session equivalent in the vRPC.
API-visible signature change
NewTableShim'ssessionparameter type becomessession.TableAPI(aninternal-package interface). Existing in-repo callers pass
nilforsession today; nil is the zero value of any interface, so the change
is source-compatible for those. External callers writing tests can
mock
session.TableAPIdirectly — the new test file has an example(
mockSessionTable).Tests
Replaces the
mockTableAPI-as-session pattern with a proto-nativemockSessionTable. New coverage:TestTableShim_ReadRow_RoutesByDiverter— classic whenSessionLoad=0.0; session whenSessionLoad=1.0; classic fallbackwhen session is nil even with
SessionLoad=1.0; session errorpropagation (no automatic fallback to classic on failure).
TestTableShim_Apply_ConditionalAlwaysClassic— pins thatconditional mutations bypass the session path.
TestTableShim_Apply_NonConditionalRoutesByDiverter— pins thatnon-conditional mutations follow the diverter.
TestTableShim_ReadRows_AlwaysClassic,TestTableShim_SampleRowKeys_AlwaysClassic,TestTableShim_ApplyBulk_AlwaysClassic,TestTableShim_ApplyReadModifyWrite_AlwaysClassic— pin theno-session-equivalent methods.
TestTableShim_NilSession_AllMethodsFallBackToClassic— theclassic-only wiring path (what feat(bigtable): wire Diverter on Client and route Open* via TableShim #20256's
buildDivertiblewill useuntil the session backend lands).
TestTableShim_SessionErrorNotRetriedOnClassic— session-sidefailures surface as-is instead of silently falling back.
Test plan
go build ./...cleango vet ./...cleango test ./bigtable/ -run 'TestTableShim|TestProtoRowToRow' -count=1 -v— all passautonomous-mote-782 / sushanb-uc1— classic path viaclient.Open(...).Apply/ReadRowstill succeeds end-to-end (session backend not exercised on this branch since it isn't wired yet)Depends on
1297143a4f) —protoRowToRowhelper.Follows
Consumed by a future PR that wires an actual
session.TableAPIimplementationfrom the
internal/sessionpackage intoClient.buildDivertible(see#20256 for the
buildDivertibleshape).