feat: Convert SQL to Builder configs#2667
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 6198b4b The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
E2E Test Results✅ All tests passed • 242 passed • 1 skipped • 1107s
Tests ran across 4 shards in parallel. |
Greptile SummaryThis PR adds a SQL → Builder conversion path (
Confidence Score: 4/5Safe to merge for the two-way conversion wiring; the converter's silent ROLLUP/CUBE drop (flagged previously) remains unaddressed and can produce a wrong empty GROUP BY without notifying the user. The new hook logic, edit-attribution plumbing, and isUserChange propagation are all correct. The converter itself is well-tested and handles the common cases soundly. The outstanding concern is the silent ExpressionList filter in parseGroupBy: a hand-written query using GROUP BY ROLLUP(…) converts to groupBy: '' with no error, so the builder silently drops a user-authored grouping modifier. packages/common-utils/src/core/rawSqlToBuilder.ts — specifically the parseGroupBy ExpressionList filter and parseOrderBy direction interpolation noted in prior review threads. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User
participant EditTimeChartForm
participant useBuilderSqlConversion
participant renderBuilderConfigAsSqlTemplate
participant convertRawSqlToBuilderConfig
Note over useBuilderSqlConversion: lastEditedModeRef = 'builder' | 'sql' | null
User->>EditTimeChartForm: Edit builder field
EditTimeChartForm->>useBuilderSqlConversion: "setValueWithEditTracking(field, value, {isUserChange:true})"
useBuilderSqlConversion->>useBuilderSqlConversion: "lastEditedModeRef = 'builder'"
User->>EditTimeChartForm: Toggle to SQL mode
EditTimeChartForm->>useBuilderSqlConversion: "configType = 'sql'"
useBuilderSqlConversion->>useBuilderSqlConversion: "guard lastEditedModeRef === 'builder'"
useBuilderSqlConversion->>renderBuilderConfigAsSqlTemplate: convertFormState
renderBuilderConfigAsSqlTemplate-->>useBuilderSqlConversion: sql
useBuilderSqlConversion->>EditTimeChartForm: setValue('sqlTemplate', sql)
useBuilderSqlConversion->>useBuilderSqlConversion: "lastEditedModeRef = null"
User->>EditTimeChartForm: Edit SQL editor
EditTimeChartForm->>useBuilderSqlConversion: "watch fires type=change name=sqlTemplate"
useBuilderSqlConversion->>useBuilderSqlConversion: "lastEditedModeRef = 'sql'"
User->>EditTimeChartForm: Toggle to Builder mode
EditTimeChartForm->>useBuilderSqlConversion: "configType = 'builder'"
useBuilderSqlConversion->>useBuilderSqlConversion: "guard lastEditedModeRef === 'sql'"
useBuilderSqlConversion->>convertRawSqlToBuilderConfig: sqlTemplate + displayType + from
convertRawSqlToBuilderConfig-->>useBuilderSqlConversion: SqlToBuilderResult
useBuilderSqlConversion->>EditTimeChartForm: setValue(select/where/groupBy/granularity)
useBuilderSqlConversion->>useBuilderSqlConversion: "lastEditedModeRef = null"
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant User
participant EditTimeChartForm
participant useBuilderSqlConversion
participant renderBuilderConfigAsSqlTemplate
participant convertRawSqlToBuilderConfig
Note over useBuilderSqlConversion: lastEditedModeRef = 'builder' | 'sql' | null
User->>EditTimeChartForm: Edit builder field
EditTimeChartForm->>useBuilderSqlConversion: "setValueWithEditTracking(field, value, {isUserChange:true})"
useBuilderSqlConversion->>useBuilderSqlConversion: "lastEditedModeRef = 'builder'"
User->>EditTimeChartForm: Toggle to SQL mode
EditTimeChartForm->>useBuilderSqlConversion: "configType = 'sql'"
useBuilderSqlConversion->>useBuilderSqlConversion: "guard lastEditedModeRef === 'builder'"
useBuilderSqlConversion->>renderBuilderConfigAsSqlTemplate: convertFormState
renderBuilderConfigAsSqlTemplate-->>useBuilderSqlConversion: sql
useBuilderSqlConversion->>EditTimeChartForm: setValue('sqlTemplate', sql)
useBuilderSqlConversion->>useBuilderSqlConversion: "lastEditedModeRef = null"
User->>EditTimeChartForm: Edit SQL editor
EditTimeChartForm->>useBuilderSqlConversion: "watch fires type=change name=sqlTemplate"
useBuilderSqlConversion->>useBuilderSqlConversion: "lastEditedModeRef = 'sql'"
User->>EditTimeChartForm: Toggle to Builder mode
EditTimeChartForm->>useBuilderSqlConversion: "configType = 'builder'"
useBuilderSqlConversion->>useBuilderSqlConversion: "guard lastEditedModeRef === 'sql'"
useBuilderSqlConversion->>convertRawSqlToBuilderConfig: sqlTemplate + displayType + from
convertRawSqlToBuilderConfig-->>useBuilderSqlConversion: SqlToBuilderResult
useBuilderSqlConversion->>EditTimeChartForm: setValue(select/where/groupBy/granularity)
useBuilderSqlConversion->>useBuilderSqlConversion: "lastEditedModeRef = null"
Reviews (2): Last reviewed commit: "fix: Track changes from unregistered fie..." | Re-trigger Greptile |
| } | ||
|
|
||
| /** User-facing display-type labels */ | ||
| export const DISPLAY_TYPE_LABELS: Record<DisplayType, string> = { | ||
| [DisplayType.Line]: 'Time Series', | ||
| [DisplayType.StackedBar]: 'Bar', | ||
| [DisplayType.Table]: 'Table', | ||
| [DisplayType.Pie]: 'Pie', | ||
| [DisplayType.Bar]: 'Bar', | ||
| [DisplayType.Number]: 'Number', | ||
| [DisplayType.Search]: 'Search', | ||
| [DisplayType.Heatmap]: 'Heatmap', | ||
| [DisplayType.Markdown]: 'Markdown', |
There was a problem hiding this comment.
StackedBar and Bar share the same label, producing confusing error messages
Both DisplayType.StackedBar and DisplayType.Bar are mapped to 'Bar'. When validateDisplayType rejects a time bucket on a non-time-series chart (e.g. displayType === DisplayType.Bar), the error reads "Time bucketing is only supported for Time Series and Bar charts, not Bar charts." — the user sees "not Bar charts" for a Bar-chart input, which is contradictory and unhelpful. Consider a distinct label such as 'Stacked Bar' for StackedBar.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1b38012 to
6198b4b
Compare
Summary
Screenshots or video
How to test on Vercel preview
Preview routes:
Steps:
References