Repository navigation
Conversation
Add SQL Server table paging, primary-key discovery, and cached query execution with viewer filtering and sorting. Preserve temporal and integer64 values through Arrow IPC spill storage. Add shared column projection, unknown row counts, cancellation, cleanup, recovery, and required CI dependencies. Include DBI and primary-key regression tests.
Add progressive paging, projected random access, and cached query filtering and sorting. Preserve nested and temporal values, recover failed reads, and handle unknown row counts. Cover fragment and row-group access, spill caches, recovery, and platform-independent Arrow stream ordering in the regression tests.
- shows actual fetched counts, such as 5,000+ rows. - switches to the exact total at EOF, including empty results. - fixes Arrow’s exact-batch-boundary EOF case.
renkun-ken
left a comment
renkun-ken
left a comment
There was a problem hiding this comment.
Reviewed 48bd0e6. I reproduced two DBI cursor/recovery defects, detailed inline: sharing one connection between viewers invalidates their retained results, and an interrupt between fetching and caching can silently omit a batch on retry.
Validation against this head:
- Built the extension, compiled TypeScript, and passed TypeScript lint.
- Installed the exact generated
dist/resources/sessinto an isolated R library; all 1,614 checks across the eight Data Viewer R test files passed with Arrow, DBI, dbplyr, and RSQLite available. - All 24 focused Data Viewer / viewer-session extension tests passed in VS Code.
- Reproduced both findings separately with real SQLite; the second uses deterministic interrupt injection immediately after a successful fetch.
- All five current CI jobs are green.
The existing SQLite coverage exhausts each small query before opening the next viewer, and the interruption coverage injects inside dbFetch(), so neither scenario is covered by those passing tests.
| if (is.null(cache_state$result)) { | ||
| cache_state$result <- DBI::dbSendQuery( | ||
| state$dbi$con, state$dbi$query_sql | ||
| ) |
There was a problem hiding this comment.
[P2] Coordinate retained results across viewers sharing a connection
This retains a result on the user's original connection between page requests, but supported drivers such as RSQLite allow only one active result per connection. With a SQLite table containing id = 1:12000, open View(items, title = "A"), fetch its first page, then open View(items, title = "B"). B's metadata query in dataview_dbi_source() closes A's result (Closing open result set, pending rows), and scrolling A past its cached 5,000 rows fails with Invalid result set. Retrying A opens another cursor and invalidates B in turn. An ordinary DBI::dbGetQuery() on that connection has the same effect. Please coordinate cursor lifetimes per connection or isolate viewer executions, and add a real SQLite test with two partially fetched viewers; the current 12-row SQLite fixtures complete before this conflict can occur.
| ) | ||
| dataview_dbi_result_cache_add(cache_state, block) | ||
| if (!nrow(block) || DBI::dbHasCompleted(cache_state$result)) { |
There was a problem hiding this comment.
[P2] Include cache publication in interrupt recovery
The recovery handlers cover only dbFetch(), although that call has already advanced the cursor when dataview_dbi_result_cache_add() starts. If an interrupt arrives before the fetched block is published, the cursor remains open and result_cache_rows still describes the previous position. I reproduced this with real SQLite (id = 1:12000) by injecting an interrupt condition at entry to dataview_dbi_result_cache_add() after the first successful fetch: the cache still reports zero rows, and retrying startRow = 0, endRow = 2 returns values 5001 and 5002, labeled as rows 1 and 2. This silently drops a batch after cancellation. Please protect the entire fetch/cache/completion transition with cleanup that resets the execution on an error or interrupt, and test interruption after the fetch has succeeded.
- A second viewer on the same connection is rejected before affecting the first. - Separate connections keep independent cursors and caches. - Reopening and disposal release resources correctly. - External cursor invalidation produces a clear error without silently restarting.
- Clear the cursor and all spill files so retries restart without skipping or mixing rows. - Add SQLite regression tests for failures after fetching and during cache updates.
# Conflicts: # src/session.ts # src/test/suite/dataViewerSessions.test.ts
|
Shouldn't we prioritize #1879 and close this instead? |
Oh, I thought |
|
Oh, sorry for not being clearer. |
I don't disagree. Once the current issues are resolved and the vscode-R integration is working well, it might be worth having a code level review and optimisation on the |
3 participants
Closes #1785
This PR is a clean replacement for #1804. It includes the implementation and subsequent fixes, incorporates the latest master, and reorganises the changes into two commits: DBI support with shared viewer infrastructure, followed by Arrow support.
I am submitting both implementations for review and discussion; they do not necessarily need to be adopted together.
Summary
Add on-demand Data Viewer support for Arrow datasets,
arrow_dplyr_queryobjects, and DBI-backedtbl_sqlobjects,currently targeting SQL Serverusingdbplyrfor database dialect handling.Opening and scrolling do not require collecting the complete result into an R data frame. Paging, filtering, and sorting use different strategies for datasets, bare database tables, and transformed lazy queries.
Arrow datasets
integer64, dates, datetimes, and durations. Disable filtering and sorting for nested columns.SQL ServerDatabase tablestbl_sql.Execute viewer filtering and sorting in SQL Server.Discover primary keys where available to provide stable paging order.dbplyrto generate dialect-appropriate metadata and query SQL.and use, fetching 5,000-row batches and using cached results for backward access.OFFSETwhen repositioning outside the cachearrowpackage for disk caching, including bare tables.Transformed lazy queries
For both
arrow_dplyr_queryand transformed database queries:On the first viewer filter or sort, finish caching the active query execution, then apply the viewer operation to that result.+, then the exact total at completion, including empty results.Viewer filtering and sorting therefore act as additional operations over the original Arrow query result. They do not rewrite the user's query.
Transformed database queries also requireAll supported database tables and queries require the optionalarrowpackage for disk caching.Shared viewer behaviour
Memory and current limitations
Block caches use a 20,000-row retention threshold, but this is not a strict total-memory limit. Individual Arrow batches can be larger, and Arrow filtering or sorting can require complete key columns and row indices in memory. Cached lazy-query results also consume temporary disk space.
Stable bare-table ordering depends on an available unique key.Database rows retain the order returned by the original query execution; the viewer does not add an ordering clause. Source changes require reopening the viewer.Large unsigned 64-bit integers and high-precision decimals remain a known precision limitation when converted to R numeric values, affecting display, filtering, and sorting.
Validation
Regression coverage includes sequential and random paging, fragments and row groups, projection, Arrow filtering and sorting, cache spilling, ordering, failure recovery, interruption, disposal, supported column types, and fetched-count transitions to exact totals. Arrow query tests account for platform-dependent batch arrival order.
Local Windows validation passed:
Latest local macOS validation passed:
DBI tests use simulated connections and SQLite, and verify SQL generation and cursor behaviour; they do not replace live SQL Server/ODBC integration testing. macOS and Ubuntu CI results are not claimed by this local validation.