feat: estimates counts implementation - #311
Conversation
…Kavish022/conar into estimates-counts-implementation
…m interface with parts property
|
🚅 Deployed to the conar-pr-311 environment in Conar
|
There was a problem hiding this comment.
Pull request overview
This PR implements estimated row counts for database tables to improve performance when displaying large tables. Instead of always running expensive COUNT(*) queries, the system now fetches quick estimates from database system catalogs and allows users to click for exact counts when needed.
Changes:
- Added
exactboolean field to the page store to track whether exact or estimated counts should be fetched - Implemented database-specific estimate queries for PostgreSQL (pg_catalog.pg_class.reltuples), MySQL (information_schema.TABLES.TABLE_ROWS), and ClickHouse (system.parts)
- Updated the header UI to display estimated counts with a "~" prefix and a tooltip allowing users to click for exact counts
- Modified query keys and invalidations to include the
exactparameter for proper cache management
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/desktop/src/routes/_protected/database/$id/table/-store.ts | Added exact boolean field to store state, defaulting to false |
| apps/desktop/src/routes/_protected/database/$id/table/-components/header/header.tsx | Updated UI to show estimated/exact counts with click-to-exact functionality |
| apps/desktop/src/routes/_protected/database/$id/table/-components/header/header-actions.tsx | Updated query invalidation to include exact parameter |
| apps/desktop/src/routes/_protected/database/$id/table/-components/header/header-actions-delete.tsx | Updated query invalidation to include exact parameter |
| apps/desktop/src/entities/connection/utils/fetching.ts | Updated prefetch to use exact: false for initial estimate |
| apps/desktop/src/entities/connection/sql/total.ts | Implemented estimate queries for each database type with fallback to exact counts |
| apps/desktop/src/entities/connection/queries/total.ts | Added exact parameter to query function and query key |
| apps/desktop/src/entities/connection/dialects/postgres/schema/catalog.ts | Added reltuples field to PgClass interface |
| apps/desktop/src/entities/connection/dialects/clickhouse/schema/system.ts | Added Parts interface for system.parts table |
| apps/desktop/src/entities/connection/dialects/clickhouse/schema/index.ts | Formatting improvement for type definition |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (estimate && estimate.count >= 0) { | ||
| return { |
There was a problem hiding this comment.
In PostgreSQL, reltuples can be -1 for tables that have never been analyzed. While the check estimate && estimate.count >= 0 correctly handles negative values, it would be clearer to add an explicit null check as well: estimate && estimate.count != null && estimate.count >= 0 for defensive programming, especially since the type system might not catch all edge cases.
| className={cn('inline-flex items-center gap-1', !exact && total?.isEstimated && ` | ||
| cursor-pointer | ||
| `)} |
There was a problem hiding this comment.
The template literal with embedded newlines and indentation in the className creates unnecessary whitespace in the resulting class string. While cn() may handle this, it's cleaner to use an array or keep the condition inline. Consider refactoring to: className={cn('inline-flex items-center gap-1', !exact && total?.isEstimated && 'cursor-pointer')}
| className={cn('text-muted-foreground tabular-nums', isLoading && ` | ||
| animate-pulse | ||
| `)} |
There was a problem hiding this comment.
Similar formatting issue: the template literal with embedded newlines creates unnecessary whitespace. Consider simplifying to: className={cn('text-muted-foreground tabular-nums', isLoading && 'animate-pulse')}
| const [exact, setExact] = useState(false) | ||
| const { data: total, isLoading } = useConnectionTableTotal({ connection, table, schema, query: { filters, exact } }) |
There was a problem hiding this comment.
The exact state is managed as local component state using useState, but other parts of the application (like header-actions.tsx at line 23 and header-actions-delete.tsx at line 29) read this value from the global store using store.state.exact. This creates a state synchronization issue where clicking to get exact counts in the header won't update the store, causing query invalidations to use stale exact values.
The exact state should be read from and written to the store instead of using local state. Replace the useState with useStore to read from the store, and use store.setState to update it when clicking.
| className={cn('inline-flex items-center gap-1', !exact && total?.isEstimated && ` | ||
| cursor-pointer | ||
| `)} | ||
| onClick={() => setExact(true)} |
There was a problem hiding this comment.
The onClick handler unconditionally sets exact to true, but it should only be clickable when the count is estimated. When exact is already true or the count is not estimated, clicking should have no effect. Consider conditionally calling setExact(true) only when !exact && total?.isEstimated to prevent unnecessary state updates and re-renders.
| onClick={() => setExact(true)} | |
| onClick={() => { | |
| if (!exact && total?.isEstimated) { | |
| setExact(true) | |
| } | |
| }} |
| const { data: total, isLoading } = useConnectionTableTotal({ connection, table, schema, query: { filters, exact } }) | ||
|
|
||
| const columnsCount = columns?.length ?? 0 | ||
| const count = Number(total?.count) |
There was a problem hiding this comment.
The expression Number(total?.count) will return NaN when total is undefined during the initial loading state. This will cause the NumberFlow component to display NaN rows, which is not user-friendly. Consider using a fallback value like Number(total?.count ?? 0) or conditionally rendering based on whether total exists.
| const count = Number(total?.count) | |
| const count = Number(total?.count ?? 0) |
| {!exact && total?.isEstimated && ( | ||
| <TooltipContent side="bottom"> | ||
| Click to get the exact count. | ||
| </TooltipContent> | ||
| )} |
There was a problem hiding this comment.
When filters are applied, the code falls back to exact counts (because estimates are only used when !filters?.length). However, the UI state doesn't reflect this - the user might still see the tooltip "Click to get the exact count" even though an exact count is already being fetched due to filters. Consider updating the UI logic to show that counts are always exact when filters are active.
…options with exact filter
No description provided.