refactor(datagrid): unify table browse query and drop SUBSTRING column truncation#1373
Merged
Conversation
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
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.
Problem
Two things looked wrong when browsing a table:
The editor query and the executed query were different. Opening a table showed
SELECT * FROM \t` LIMIT 1000;, but the first page load (and every refresh/pagination/sort/filter) silently replaced it with a different query:SELECT `id`, …, SUBSTRING(`user_agent`, 1, 256) AS `user_agent`, … FROM `t` LIMIT 1000 OFFSET 0. Two separate query builders (QueryTab.buildBaseTableQueryandTableQueryBuilder`) produced different SQL.The
SUBSTRING(col, 1, 256) AS coltruncation was an anti-pattern.ColumnExclusionPolicytruncated "very long text" columns server-side and aliased the result back as the real column, so the grid had no idea the value was partial. Grid cell edits had no lazy-load (only the sidebar single-row path did), so editing such a cell and saving wrote the 256-char value back, corrupting the column. The code already refused to do this for BLOBs for exactly this reason, but applied it to text inconsistently.No mainstream tool (TablePlus, DataGrip, DBeaver) does this. They fetch the full value and truncate at the render layer, or limit transfer while tracking the partial state and refusing to write it back.
Change
TableQueryBuilderis now the single source of truth.QueryTab.buildBaseTableQuerydelegates its SQL branch to it, so the editor query equals the executed query.TableQueryBuildernow emits the mandatoryORDER BYforOFFSET…FETCHdialects when unsorted, and the rebuild path passes the schema so qualification matches.ColumnExclusionPolicy, thecolumnExclusionsplumbing,LazyLoadColumnsService, the sidebar lazy-load path, the write-only column caches, andMultiRowEditState.isTruncated/applyFullValuesplus the "truncated" / loading UI in the field editors.Large text follows the same model already used for BLOBs: fetched in full, truncated only at render, full value available in the cell viewer.
Net ~180 lines removed; 6 files deleted.
Tests
ColumnExclusionPolicyTests,TableQueryBuilderSelectiveTests, andMultiRowEditStateTruncationTestscovered the removed behavior and were deleted.TableQueryBuilderMSSQLTests(plugin path) andTableQueryBuilderFilterTestsstill pass: the MSSQLOFFSET…FETCH+ORDER BY (SELECT NULL)behavior is preserved, andSELECT *is now always emitted.