Add query results pagination support - #942
Dharshini-RS03 wants to merge 7 commits into
Conversation
|
rebase upstream, and can you fix CI error |
|
Sure, I’ll rebase my branch onto the latest upstream/main, investigate the CI failures, and push the fixes. Thanks! |
218407f to
b9055b5
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Updated the implementation based on the review feedback for #816.
All CI checks are now passing. |
cevheri
left a comment
There was a problem hiding this comment.
Thanks, the backbone of this is right and you found the seam the design asks for. Four things are correct and need no further work: hasMore now requires prepared.wasLimited in src/app/api/db/query/route.ts:139, both handleLoadMore copies reuse result.pagination.limit instead of the hardcoded 500, supportsResultPagination follows the supportsInlineRowEdit precedent in src/lib/db/types.ts and is gated on === true in the UI, and the preview cap has left the generated SQL text.
I ran the branch against a PostgreSQL table of 2000 rows to see the result on screen. Opening the table generates SELECT * FROM public.orders;, the grid shows 500 rows with the AUTO-LIMITED badge, Load More appends 500 at a time to 2000, and one further click returns an empty page and retires the control. So the mechanism works. What follows is where it does not yet match the design in #816, with the acceptance criterion for each item.
1. The preview page size has to travel as the limit execution option
The design removes the bound from the SQL text and sends 50 as an execution option. This PR does the first half only, so the first page is now DEFAULT_QUERY_LIMIT, 500 rows, on every engine. That is a tenfold change to what a click on a table costs, and it also makes the "Select Top 50" label at src/components/schema-explorer/TableItem.tsx:114 untrue.
The seam is handleTableClick in src/hooks/use-tab-manager.ts:330, whose executeQueryFn parameter is typed (query: string, tabId: string) => void and therefore cannot carry options today. It is called at src/workspace/StudioWorkspace.tsx:416 with queryExec.executeQuery, which already accepts an options argument.
Acceptance criteria:
- Opening a table from the object tree returns 50 rows, and the response reports
pagination.limit: 50. - Load More then fetches 50 more, not 500: page two is the size of page one.
- A hook test covers the option reaching
executeQuery, and the same behaviour holds in the standalone app and inStudioWorkspace, since both render this path.
2. The fallback statement still carries a bound
src/hooks/use-tab-manager.ts:341 still emits `SELECT * FROM ${path.join(".")} LIMIT 50;` when capabilities is absent. The design names this string explicitly. As it stands a tab opened before metadata arrives is still pinned to page one.
Acceptance criterion: that string carries no row bound, and the same execution option from item 1 applies to it.
3. Every provider has to declare the flag, and six do not
Ten providers were set to true and I agree with all ten. Six have no value at all, which the design rules out: the flag is optional in the type only because the interface is published through src/exports/, and every provider in this repo declares it. Each value below comes from that provider's own prepareQuery, which is what the design asks you to read.
| Provider | Value | Why, from its own code |
|---|---|---|
sql/cassandra/index.ts:224 |
false |
prepareQuery:355 throws on any SELECT with offset > 0 |
sql/clickhouse/index.ts:504 |
true |
Inherits the sql-base offset form. Its trailing FORMAT / SETTINGS path returns wasLimited: false, which your own hasMore conjunct now covers |
sql/search/index.ts:680 |
see below | prepareQuery:827 throws unless this.product.acceptsOffsetClause |
document/mongodb.ts:623 |
false |
prepareQuery:694 pins offset: 0 |
keyvalue/redis.ts:158 |
false |
prepareQuery:865 pins offset: 0 |
embedded/libredb.ts:321 |
false |
No override, so base-provider.ts:366 never sets wasLimited, and page two can never be offered |
The search provider is the one that cannot take a literal. One class serves two type-ids (#424), and the two products differ exactly here: acceptsOffsetClause is false for Elasticsearch at search/index.ts:291 and true for OpenSearch at search/index.ts:300. So the declaration has to be supportsResultPagination: this.product.acceptsOffsetClause. Writing true there would offer a control that makes Elasticsearch throw; writing false would hide a control OpenSearch can serve.
Acceptance criteria:
supportsResultPaginationappears in everygetCapabilities()undersrc/lib/db/providers/, each with a one-line comment giving the reason, in the style of the flags around it.tests/integration/db/<type-id>-provider.test.tsasserts the value for its own type-id, including one assertion per product for elasticsearch and opensearch.
4. The regression this leaves behind
Items 1 and 3 together produce a loss on three engines. ClickHouse, Elasticsearch and OpenSearch fall into the default branch of generateTableQuery, so they lost their LIMIT 50, and with no flag declared they show no Load More. Their table preview goes from 50 rows to 500 and gains nothing in return. Cassandra avoided this only because a port 9042 branch was added to keep its LIMIT 50.
Acceptance criterion: for every type-id, opening a table either offers the preview page size with a working Load More, or keeps a bound in the generated statement. No engine is left with a larger preview and no control.
5. The unordered-pages notice is missing
The design accepts that an unordered query can return overlapping or skipped pages, and requires that the grid say so once rather than let it pass silently, next to the existing wasLimited badge at src/components/results-grid/StatsBar.tsx:92. That file is untouched in this PR.
Acceptance criteria:
- When pagination is offered and the executed statement has no
ORDER BY, the grid states the condition once, beside AUTO-LIMITED. - It is stated once, not per page, and it is a statement of fact and not a warning that blocks anything.
- A component test covers both the ordered and the unordered case.
6. Docs and tests have to move with the code
CLAUDE.md requires code, docs and tests in step in the same PR, and this diff changes code only.
src/lib/db/types.ts:247has no docblock, while every flag around it does.- The capability table in
docs/ADDING_A_PROVIDER.mdand the per provider capability tables indocs/providers/*.mddo not mention the new flag. - The generated statement is documented with its old bound in
docs/providers/elasticsearch.md:580-586,docs/providers/opensearch.md:606,docs/providers/couchbase.md:276,docs/providers/druid.md:886anddocs/API_DOCS.md:443,:483,:520. All of those are now wrong. - The ClickHouse test named for #264, "the trailing LIMIT is the last clause, so a user-appended FORMAT stays legal", was deleted. Please re-aim it at whatever still carries that invariant rather than removing the guard.
7. One item to confirm rather than change
The design asks that a failed page behave the same in both hooks and that neither advances currentOffset on failure. Reading the branch, use-query-adapter.ts raises a "Load More Error" toast and use-query-execution.ts reaches its own error toast through executeQuery, and neither advances the offset. Please confirm that on both paths and say so in the PR, so it is measured rather than assumed.
Out of scope, so please do not add it here
Automatic fetch on scroll stays out, as #816 says, and the manual control stays. The footer button's own placement and styling is a separate concern that predates this work (StatsBar.tsx, from #810), and I will open its own issue. Nothing about it belongs in this PR.
|
One optional extra on top of the review above. This is a UI change rather than a correctness one, so treat it as a nice to have: take it if you want, and it will not hold the PR up either way. The Load More footer ( So if you want to fold the two together: make that Acceptance criteria:
|
|
Thanks for the detailed review. I went through all the requested changes for #816 and I understand what needs to be updated. I’ll make the changes to pass the 50-row preview limit as an execution option, update the fallback query, add the pagination capability for all the required providers, and make sure ClickHouse, Elasticsearch, and OpenSearch still keep the 50-row preview with Load More. I’ll also add the unordered-results message, update the related docs and tests, and verify the failed-page behavior in both query execution paths. As requested, I’ll keep automatic infinite scroll out of this PR. I’ll work on these changes and run the relevant tests before updating the PR. |
thanks for explanation |
Description
Adds query results pagination support for supported database providers. Generated table queries no longer impose a preview limit where the pagination infrastructure can handle it, allowing subsequent result pages to load correctly.
Type of Change
Related Issue
Closes #816
Changes Made
Testing
Checklist
Additional Notes
The existing pagination infrastructure was reused rather than rebuilt. Pagination remains unavailable for providers that do not support the required offset behavior.