[CXH-16] fix list tables pagination adding database and schema as constraints#104
Merged
[CXH-16] fix list tables pagination adding database and schema as constraints#104
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughList pagination now enumerates schemas in a database into a pagination.Bag (skipping INFORMATION_SCHEMA) and then pages tables per-schema (page size 200) using schema-scoped table listing. Snowflake client gains schema listing and a ListTablesInSchema API with simplified single-part cursors. Changes
sequenceDiagram
autonumber
participant Connector as Connector (pagination)
participant Bag as pagination.Bag
participant Snowflake as Snowflake Client
participant ResourceSink as Resource Iterator
Connector->>Snowflake: ListSchemasInDatabase(database)
Snowflake-->>Connector: schemas[]
Connector->>Bag: push(schemas as PageState)
loop per-schema
Connector->>Bag: pop() -> schemaPage
Connector->>Snowflake: ListTablesInSchema(database, schemaPage.ResourceID, schemaPage.Cursor, limit=200)
Snowflake-->>Connector: tables[], nextCursor
Connector->>ResourceSink: emit(table resources)
alt nextCursor != ""
Connector->>Bag: update current schemaPage cursor = nextCursor
else
Connector->>Bag: advance to next schema
end
end
Connector->>ResourceSink: return nextToken(from Bag) / finished
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/snowflake/table.go`:
- Around line 111-116: The current database-level pagination using SHOW TABLES
IN DATABASE and setting nextCursor = last.Name (see tables, nextCursor,
last.Name) is incorrect across schemas and will skip/duplicate entries; replace
it by either (A) enumerating per schema: list schemas, then call SHOW TABLES IN
SCHEMA for each schema and maintain a cursor per schema, or (B) switch to
querying INFORMATION_SCHEMA.TABLES with an explicit ORDER BY schema_name,
table_name and use LIMIT/OFFSET or a composite keyset cursor (schema_name +
table_name) instead of last.Name; update the pagination logic that builds the
query and the nextCursor value to use the composite cursor or per-schema cursors
accordingly (modify the code around where SHOW TABLES IN DATABASE is issued and
where nextCursor is computed).
luisina-santos
force-pushed
the
luisinasantos/fix-table-pagination
branch
from
31c794c to
e493bf0
Compare
luisina-santos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/connector/tables.go (1)
139-143:GetDatabaseis called on every page — consider cachingisSharedOrSystemDBin the bag state.The database's shared/system status is constant across pages. Repeating this network call for every page of a large result set wastes one round-trip per page.
One approach: store the flag in the first
PageState(e.g., inResourceTypeIDor a separate token prefix), or derive it from the parentResourceID profile if it's already stored there.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/snowflake/table.go`:
- Around line 51-79: ListSchemasInDatabase currently lacks the 422/privilege
error handling present in ListTablesInSchema; add the same privilege-check
pattern by extracting the logger with l := ctxzap.Extract(ctx) at the top of
ListSchemasInDatabase, then in the resp2 handling block (where resp2 and
response are used) detect the 422 privilege response the same way
ListTablesInSchema does, log a debug message including databaseName and
response/statement handle, and return a gRPC PermissionDenied error with
contextual schema/database info instead of the raw HTTP error; mirror the exact
checks and logging used in ListTablesInSchema for resp2/response to keep
behavior consistent.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/snowflake/table.go (1)
155-174:⚠️ Potential issue | 🟠 MajorGuard
resp1/resp2before deferringcloseResponseBody.Same nil-check risk here as above; a nil response on network error can panic when closing.
🛠️ Suggested fix
resp1, err := c.Do(req, uhttp.WithJSONResponse(&response)) -defer closeResponseBody(resp1) +if resp1 != nil { + defer closeResponseBody(resp1) +} ... resp2, err := c.Do(req, uhttp.WithJSONResponse(&response)) -defer closeResponseBody(resp2) +if resp2 != nil { + defer closeResponseBody(resp2) +}As per coding guidelines, "Always check if http.Response is not nil before attempting to close resp.Body, as resp can be nil on network errors".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/snowflake/table.go` around lines 155 - 174, The defer calls to closeResponseBody use resp1/resp2 which can be nil on network errors; update the code around the c.Do calls that populate response (the ListTablesRawResponse path using resp1 and the subsequent GetStatementResponse + c.Do using resp2) to guard before deferring by checking the response is non-nil (e.g., if resp1 != nil { defer closeResponseBody(resp1) }) and do the same for resp2 so closeResponseBody is only deferred when the http.Response exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/snowflake/table.go`:
- Around line 189-193: The current logic that sets nextCursor uses if
len(tables) >= limit which can panic when limit == 0 and tables is empty; update
the condition around the slice access in pkg/snowflake/table.go (the block that
sets nextCursor based on tables and limit) to ensure you only index tables when
limit > 0 and len(tables) > 0 (e.g. require limit > 0 && len(tables) >= limit or
check len(tables) > 0 before using tables[len(tables)-1]), so you never access
tables[len(tables)-1] on an empty slice.
- Around line 64-82: The defer calls to closeResponseBody for resp1 and resp2
can panic because resp may be nil on network errors; update the code around the
c.Do calls (where resp1 and resp2 are returned) to only defer closeResponseBody
when the response is non-nil (e.g., check resp1 != nil before deferring, or use
a short inline defer func() that checks the variable for nil before calling
closeResponseBody), keeping the surrounding error handling in
GetStatementResponse and ListSchemasRawResponse logic intact so you still return
errors as before.
---
Outside diff comments:
In `@pkg/snowflake/table.go`:
- Around line 155-174: The defer calls to closeResponseBody use resp1/resp2
which can be nil on network errors; update the code around the c.Do calls that
populate response (the ListTablesRawResponse path using resp1 and the subsequent
GetStatementResponse + c.Do using resp2) to guard before deferring by checking
the response is non-nil (e.g., if resp1 != nil { defer closeResponseBody(resp1)
}) and do the same for resp2 so closeResponseBody is only deferred when the
http.Response exists.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/snowflake/table.go`:
- Around line 51-92: In ListSchemasInDatabase, several raw errors are returned
directly; wrap each with the "baton-snowflake" prefix using fmt.Errorf("%s: %w",
...) before returning. Specifically, wrap errors returned from
c.PostStatementRequest (when err != nil), from the first c.Do call (the
non-permission-denied branch where you currently return err), from
c.GetStatementResponse (when err != nil), and from the second c.Do call
(non-permission-denied branch). Use descriptive messages (e.g.,
"baton-snowflake: failed to post statement", "baton-snowflake: failed to fetch
statement response", "baton-snowflake: failed to get statement result") and %w
to preserve error chaining when returning the error from ListSchemasInDatabase.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/connector/tables.gopkg/snowflake/table.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/connector/tables.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/connector/tables.go`:
- Around line 139-165: When seeding the pagination bag in ListTables (bag,
bag.Unmarshal, bag.Current), handle GetDatabase returning UnprocessableEntity or
nil parentDB instead of failing: call o.client.GetDatabase(ctx, databaseName)
inside a branch that treats UnprocessableEntity as "shared/system" (use
isDBSharedOrSystem or replicate its logic) and set sharedFlag = "shared" when
appropriate; if GetDatabase returns nil without error, treat as non-shared but
avoid dereferencing parentDB; only return an error for unexpected GetDatabase
failures, then continue to call o.client.ListSchemasInDatabase and seed the bag
with PageState.ResourceTypeID set correctly.
luisina-santos
requested review from
FeliLucero1,
mateoHernandez123 and
sergiocorral-conductorone
btipling
approved these changes
Feb 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Style / Messages