Repository navigation
fix(column-stats): plugin stats follow-ups (i18n, sql_syntax.extends, sample_size, docs) - #1239
Conversation
Code Review SummaryStatus: No Issues Found | Recommendation: Merge The PR was rebased onto the
Files Reviewed (18 files)
Previous Review Summary (commit 8708332)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 8708332)Status: No Issues Found | Recommendation: Merge The changes match the PR description. Files Reviewed (18 files)
Reviewed by claude-sonnet-5.5 · Input: 256 · Output: 2K · Cached: 329.7K Review guidance: REVIEW.md from base branch |
debba
left a comment
debba
left a comment
There was a problem hiding this comment.
Thanks for wrapping up the follow-ups from #1232! The sample_size change matches the Rust side exactly (models/column_stats.rs), and the test checks each part on its own. Having sql_syntax.extends win over sql_dialect is consistent with how sqlSyntax.ts resolves the preset.
It needs a rebase onto main. #676 moved and reformatted Editor.tsx, columnStats.ts, driverCapabilities.ts and the related tests into packages/web-ui/ (4 spaces, single quotes), so git sees them as modify/delete conflicts instead of renames. The logic hasn't changed on main: the hardcoded string is still in packages/web-ui/src/pages/Editor.tsx, and speaksGenericSql is untouched. Re-applying your ~10 lines in the new files and moving the tests under packages/web-ui/tests/ should be enough. The locales and PLUGIN_GUIDE.md merge on their own.
Two nits:
- You updated the Column Statistics section of
PLUGIN_GUIDE.md, but thecolumn_statsrow in the capabilities table still says only "plugins whosesql_dialectis ...". speaksGenericSqltrustsextendseven when it isn't a valid dialect, whilesqlSyntax.tsignores an invalidextendsand falls back tosql_dialect. The impact is low, since the backend drops invalid descriptors, but it would be nice to align the two.
… sample_size, docs)
8708332 to
f683520
Compare
|
@debba Rebased onto main and re-applied the changes in the new Both nits are addressed:
|
debba
left a comment
debba
left a comment
There was a problem hiding this comment.
Thanks for redoing this on top of the new layout! Re-applying everything by hand in packages/web-ui/ with the new formatting isn't fun, and you also added tests for a bogus extends with and without sql_dialect, which is exactly the case I was worried about. Reusing isDialect from the splitter keeps speaksGenericSql in step with how sqlSyntax.ts resolves the preset. I also checked the new note about get_column_stats getting no session, and it's accurate.
I'm approving. Tests, build, lint and CI are all green on my side.
Two small things I noticed, fine as a follow-up:
plugins/manifest.schema.jsonstill describescolumn_statswith onlysql_dialect("plugins with a postgres, mysql or sqlite sql_dialect..."). It's the same wording you fixed in the guide table, and plugin authors see that text in their editor.- The JSDoc on
getColumnStatsSourceincolumnStats.tsstill mentions onlysql_dialect.
I'll run a few manual checks with a test plugin before merging.
Summary
Follow-ups from the review of #1180 (optional
get_column_statsRPC for plugin drivers, #1137). The feature itself is unchanged; this closes four small consistency gaps.Closes #1232
Changes
Translate the "unavailable" error
When a plugin declares
column_stats: truebut answers-32601,handleLoadColumnStatsthrew a hard-coded English string that the popover showed as-is. It now usesdataGrid.columnStats.unavailable, added to every locale undersrc/i18n/locales/.tis added to the callback's dependency list.sql_syntax.extendscounts as SQL-compatiblespeaksGenericSqlonly readsql_dialect, so a plugin declaringsql_syntax: { extends: "mysql" }withoutsql_dialecthad the "Column statistics" entry hidden, even though #1200 lets plugins declare their syntax that way.It now reads
sql_syntax.extends ?? sql_dialect.extendstakes precedence, matching how the syntax preset is resolved.mssql,oracleandgenericstay excluded from either field. Asql_syntaxwithoutextendsfalls back tosql_dialect, and declaring neither still doesn't count.sample_sizeconsistency with RustfromPluginColumnStatskeptsample_sizeeven when every part was excluded (for example a masked column), whilerestrict_column_statsinsrc-tauri/src/models/column_stats.rsdrops it. The TS side now drops it whendistinct,minMaxandtopValuesare all unsupported.Documentation
plugins/PLUGIN_GUIDE.md(Column Statistics):get_column_statsreceives nosessionId, so session-local state (temp tables,SET search_path) isn't visible to the plugin. The same applies toget_table_query_template.sql_syntax.extends.Tests
tests/utils/driverCapabilities.test.ts:extendsaccepted for postgres/mysql/sqlite, rejected for mssql/oracle/generic,extendswins oversql_dialect, and a syntax withoutextendsfalls back tosql_dialect.tests/utils/columnStats.test.ts:sample_sizeis dropped when all parts are excluded and kept when any one part is kept.tests/pages/EditorToolbarQuery.test.tsx: updated to expect the new i18n key.pnpm lint,pnpm typecheckandpnpm testpass.cargo testcould not be run locally because of a macOS SDK linker error, and no Rust code is touched.