Repository navigation
fix(CEMAS-103): stop leading comma when a dimension fails to resolve - #308
Merged
johnson-abraham merged 5 commits intoSep 23, 2026
Merged
johnson-abraham merged 5 commits into
johnson-abraham merged 5 commits into
Conversation
addDimensionToSQLProjection gated the separator on loop index, so a dimension that isn't registered in tableSchemas (e.g. a hidden or stale field pulled in by an upstream caller) got skipped via continue but still left the next resolvable dimension with a leading comma, breaking the generated SQL with a DuckDB parser error. Gate on whether a dimension was actually emitted instead. work-item: CEMAS-103
johnson-abraham
requested review from
itsTalwar,
ujaval403,
vpbs2 and
zaidjan-devrev
as code owners
September 19, 2026 06:53
zaidjan-devrev
left a comment
zaidjan-devrev
left a comment
Contributor
There was a problem hiding this comment.
Please bump the package.json versions too
Address review feedback: - Bump meerkat-core, meerkat-browser, meerkat-node to 0.0.136. - Clarify why a dimension can go unemitted: `dimensions` is caller-supplied and not guaranteed to match `tableSchemas` (e.g. a caller passing a field deliberately excluded from the schema). work-item: CEMAS-103
Contributor
Author
|
Bumped meerkat-core, meerkat-browser, and meerkat-node to 0.0.136 (matching the convention in prior PRs of bumping all three together, e.g. #299). |
…p state Per review feedback: build an array of resolved entries and join them with the separator, rather than tracking loop index / an emitted-flag to decide when to prepend a comma. Removes the need to reason about what happened in a previous iteration, and applies the same pattern to cubeMeasureToSQLSelectString for consistency. work-item: CEMAS-103
The prior commit's comment trim removed three comments that predate this change and document unrelated behavior: - the source-table resolution note above dimensionSchema lookup - the safeKey replacement note above the forEach - the alias-key note above the dimension push Restore all three. work-item: CEMAS-103
…ring Per review feedback: cubeMeasureToSQLSelectString was never part of the reported bug and shouldn't be structurally changed in this PR. Restored it to its original loop-based form, byte-identical to main. addDimensionToSQLProjection keeps the collect-and-join fix, which is what the regression test covers. work-item: CEMAS-103
zaidjan-devrev
approved these changes
Sep 23, 2026
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
2 participants
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.
Summary
addDimensionToSQLProjectiongated the,separator on loop index (i > 0) rather than on whether a dimension was actually emitted. When an earlier dimension in the list isn't registered intableSchemas(e.g. a hidden/stale field pulled in transitively by an upstream caller) and gets skipped viacontinue, the next resolvable dimension still received a leading comma before it — producingSELECT, <col>, ...and a DuckDBParser Error: syntax error at or near ",".ui.isHiddenfield in the FDLdimensionsarray; that field is legitimately absent from meerkat'stableSchemasbecause the schema builder filters hidden fields). This fix makesapplyProjectionToSQLQueryresilient to that mismatch regardless of which caller produces it.DevRev
work-item: CEMAS-103
Test plan
should not emit a leading comma when an earlier dimension fails to resolve) that fails on the pre-fix code and passes with the fix.meerkat-coretest suite passes (564 passed, 11 pre-existing skips, 0 failures).nx lint meerkat-core— 0 errors (36 pre-existing warnings, none in touched files).Before fix:
Screen.Recording.2026-09-19.at.1.34.35.PM.mov
After fix:
Screen.Recording.2026-09-19.at.1.35.43.PM.mov