feat(core): add SSSQL optional branch pruning MVP#550
Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR implements an MVP optional-condition pruning transformer for SSSQL-style WHERE branches, adds the PruneOptionalConditionBranches module and its types/tests, integrates it into DynamicQueryBuilder with new options, and adds documentation and a changeset entry. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DynamicQueryBuilder as DynamicQueryBuilder
participant Parser as SelectQueryParser
participant Pruner as PruneOptionalConditionBranches
participant AST as QueryAST
Client->>DynamicQueryBuilder: buildQuery(sqlTemplate, options, optionalConditionParameterStates)
DynamicQueryBuilder->>Parser: parseQuery(sqlTemplate)
Parser-->>AST: return AST
DynamicQueryBuilder->>DynamicQueryBuilder: apply filters/sort/paging
DynamicQueryBuilder->>Pruner: pruneOptionalConditionBranches(AST, pruningParameters)
Note over Pruner,AST: traverse AST, detect OR branches guarded by targeted params
Pruner->>AST: remove/mutate prunable branches
Pruner-->>DynamicQueryBuilder: return pruned AST
DynamicQueryBuilder->>DynamicQueryBuilder: apply unused-join/CTE optimizations
DynamicQueryBuilder->>DynamicQueryBuilder: serialize AST -> SQL
DynamicQueryBuilder-->>Client: return SQL
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/transformers/DynamicQueryBuilder.ts`:
- Around line 344-346: In DynamicQueryBuilder, the current
isMultiColumnDefinitionArray check treats empty arrays as multi-column metadata
and skips handling, which causes predicates like { status: [] } to be swallowed;
update the checks around the two continue sites (the block containing
isMultiColumnDefinitionArray(value) and the similar block at 387-389) to require
a non-empty array for multi-column EXISTS metadata (e.g.,
isMultiColumnDefinitionArray(value) && value.length > 0) and, for ordinary keys
where the array is empty, do not continue silently but instead reject/throw a
clear error (or pass the empty array to SqlParamInjector’s validation) so empty
filters are not dropped. Ensure you reference the same logic path used to reach
SqlParamInjector so valid multi-column defs still short-circuit but empty arrays
are handled explicitly.
- Around line 134-140: The QueryBuildOptions.jsonb flag is declared but never
used; update the serialization branch that constructs PostgresJsonQueryBuilder
(and any call sites like buildQuery(sql, { serialize, jsonb })) to pass the
jsonb value through and have PostgresJsonQueryBuilder accept and honor it (e.g.,
add a constructor/option param jsonb: boolean and use it to switch between
JSON/JSONB serialization or to throw when jsonb is false and GROUP BY would be
used), or remove QueryBuildOptions.jsonb from the public API; specifically
modify the code that instantiates PostgresJsonQueryBuilder to include the jsonb
option and implement the behavior inside PostgresJsonQueryBuilder so the flag is
actually applied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55a79867-56f9-4330-ac85-dfd7a33cbc6c
📒 Files selected for processing (9)
.changeset/issue-545-sssql-pruning-mvp.mddocs/guide/querybuilding-recipes.mddocs/guide/sssql-optional-branch-pruning.mdpackages/core/src/index.tspackages/core/src/parsers/SqlTokenizer.tspackages/core/src/tokenReaders/CommandTokenReader.tspackages/core/src/transformers/DynamicQueryBuilder.tspackages/core/src/transformers/PruneOptionalConditionBranches.tspackages/core/tests/transformers/PruneOptionalConditionBranches.test.ts
💤 Files with no reviewable changes (1)
- packages/core/src/tokenReaders/CommandTokenReader.ts
Comment on lines
+134
to
+140
| serialize?: JsonMapping | boolean; | ||
| /** | ||
| * JSONB usage setting. Must be true (default) for PostgreSQL GROUP BY compatibility. | ||
| * Setting to false will throw an error as JSON type cannot be used in GROUP BY clauses. | ||
| * @default true | ||
| */ | ||
| jsonb?: boolean; |
There was a problem hiding this comment.
jsonb is exposed but never applied.
The new QueryBuildOptions.jsonb flag is documented here, but the serialization branch never reads it. Calls like buildQuery(sql, { serialize: mapping, jsonb: false }) therefore behave exactly like the default path today, which makes this a misleading public API. Please either plumb the flag into PostgresJsonQueryBuilder or remove the option until it is implemented.
Also applies to: 304-308
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/transformers/DynamicQueryBuilder.ts` around lines 134 -
140, The QueryBuildOptions.jsonb flag is declared but never used; update the
serialization branch that constructs PostgresJsonQueryBuilder (and any call
sites like buildQuery(sql, { serialize, jsonb })) to pass the jsonb value
through and have PostgresJsonQueryBuilder accept and honor it (e.g., add a
constructor/option param jsonb: boolean and use it to switch between JSON/JSONB
serialization or to throw when jsonb is false and GROUP BY would be used), or
remove QueryBuildOptions.jsonb from the public API; specifically modify the code
that instantiates PostgresJsonQueryBuilder to include the jsonb option and
implement the behavior inside PostgresJsonQueryBuilder so the flag is actually
applied.
Comment on lines
+344
to
+346
| if (this.isMultiColumnDefinitionArray(value)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Empty array filters get swallowed by the EXISTS-metadata check.
isMultiColumnDefinitionArray([]) is true, so a normal filter like { status: [] } hits this continue path and never reaches SqlParamInjector. That silently drops the predicate. Require a non-empty array for multi-column EXISTS metadata, and reject those arrays under ordinary keys instead of skipping them.
💡 Possible fix
- if (this.isMultiColumnDefinitionArray(value)) {
- continue;
- }
+ if (this.isMultiColumnDefinitionArray(value)) {
+ throw new Error("Multi-column EXISTS definitions are only supported under '$exists' or '$notExists'.");
+ }
cleanedFilters[key] = value as StateParameterValue;
@@
- return Array.isArray(value) && value.every(entry => this.isMultiColumnDefinition(entry));
+ return Array.isArray(value) &&
+ value.length > 0 &&
+ value.every(entry => this.isMultiColumnDefinition(entry));Also applies to: 387-389
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/transformers/DynamicQueryBuilder.ts` around lines 344 -
346, In DynamicQueryBuilder, the current isMultiColumnDefinitionArray check
treats empty arrays as multi-column metadata and skips handling, which causes
predicates like { status: [] } to be swallowed; update the checks around the two
continue sites (the block containing isMultiColumnDefinitionArray(value) and the
similar block at 387-389) to require a non-empty array for multi-column EXISTS
metadata (e.g., isMultiColumnDefinitionArray(value) && value.length > 0) and,
for ordinary keys where the array is empty, do not continue silently but instead
reject/throw a clear error (or pass the empty array to SqlParamInjector’s
validation) so empty filters are not dropped. Ensure you reference the same
logic path used to reach SqlParamInjector so valid multi-column defs still
short-circuit but empty arrays are handled explicitly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/core/src/transformers/DynamicQueryBuilder.ts (2)
414-415:⚠️ Potential issue | 🟠 MajorEmpty arrays are still classified as multi-column EXISTS metadata.
Array.prototype.every()returnstruefor[], soisMultiColumnDefinitionArray([])istrue. That makes ordinary filters like{ status: [] }hit the skip path at Line 371 and disappear instead of reaching normal validation/injection. Require a non-empty array here so only real EXISTS batches short-circuit.💡 Suggested fix
private isMultiColumnDefinitionArray(value: FilterConditionValue): value is MultiColumnExistsDefinition[] { - return Array.isArray(value) && value.every(entry => this.isMultiColumnDefinition(entry)); + return Array.isArray(value) && + value.length > 0 && + value.every(entry => this.isMultiColumnDefinition(entry)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/transformers/DynamicQueryBuilder.ts` around lines 414 - 415, The predicate isMultiColumnDefinitionArray treats empty arrays as valid multi-column EXISTS metadata because Array.every([]) is true; update the check in isMultiColumnDefinitionArray to require a non-empty array (e.g., ensure Array.isArray(value) && value.length > 0 && value.every(...)) so only real MultiColumnExistsDefinition batches short-circuit; touch the isMultiColumnDefinitionArray function (and keep isMultiColumnDefinition as-is) to add the non-empty requirement for FilterConditionValue / MultiColumnExistsDefinition detection.
311-315:⚠️ Potential issue | 🟠 Major
jsonbis still exposed but not applied.This serialization path never reads
options.jsonb, sobuildQuery(sql, { serialize, jsonb: false })behaves exactly like the default path even though the public API promises different behavior. Please either plumb the flag intoPostgresJsonQueryBuilderhere or remove it until it is implemented.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/transformers/DynamicQueryBuilder.ts` around lines 311 - 315, The JSON serialization branch ignores options.jsonb, so calls to buildQuery(..., { serialize, jsonb: false }) don't change behavior; update the path to pass the jsonb flag into the PostgresJsonQueryBuilder usage (either by passing options.jsonb into PostgresJsonQueryBuilder's constructor or by forwarding it as a third argument into PostgresJsonQueryBuilder.buildJsonQuery), i.e. locate the block using options.serialize, QueryBuilder.buildSimpleQuery and PostgresJsonQueryBuilder.buildJsonQuery and ensure the jsonb boolean from options is plumbed through to PostgresJsonQueryBuilder so its behavior respects options.jsonb (or remove options.jsonb from the public API until implemented).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/transformers/PruneOptionalConditionBranches.ts`:
- Around line 26-28: The current unwrapSingleOuterParen function only strips one
ParenExpression so deeply nested parentheses (e.g. ((...))) remain and prevent
downstream checks like isBinaryOperator and getGuardedParameterName from
matching; update unwrapSingleOuterParen to repeatedly unwrap while the node is a
ParenExpression (loop or recursion) until a non-ParenExpression ValueComponent
is returned so later logic in PruneOptionalConditionBranches sees the fully
unwrapped expression.
---
Duplicate comments:
In `@packages/core/src/transformers/DynamicQueryBuilder.ts`:
- Around line 414-415: The predicate isMultiColumnDefinitionArray treats empty
arrays as valid multi-column EXISTS metadata because Array.every([]) is true;
update the check in isMultiColumnDefinitionArray to require a non-empty array
(e.g., ensure Array.isArray(value) && value.length > 0 && value.every(...)) so
only real MultiColumnExistsDefinition batches short-circuit; touch the
isMultiColumnDefinitionArray function (and keep isMultiColumnDefinition as-is)
to add the non-empty requirement for FilterConditionValue /
MultiColumnExistsDefinition detection.
- Around line 311-315: The JSON serialization branch ignores options.jsonb, so
calls to buildQuery(..., { serialize, jsonb: false }) don't change behavior;
update the path to pass the jsonb flag into the PostgresJsonQueryBuilder usage
(either by passing options.jsonb into PostgresJsonQueryBuilder's constructor or
by forwarding it as a third argument into
PostgresJsonQueryBuilder.buildJsonQuery), i.e. locate the block using
options.serialize, QueryBuilder.buildSimpleQuery and
PostgresJsonQueryBuilder.buildJsonQuery and ensure the jsonb boolean from
options is plumbed through to PostgresJsonQueryBuilder so its behavior respects
options.jsonb (or remove options.jsonb from the public API until implemented).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6c5d5deb-4322-4b08-b45f-bbde0220154b
📒 Files selected for processing (6)
.changeset/issue-545-sssql-pruning-mvp.mddocs/guide/querybuilding-recipes.mddocs/guide/sssql-optional-branch-pruning.mdpackages/core/src/transformers/DynamicQueryBuilder.tspackages/core/src/transformers/PruneOptionalConditionBranches.tspackages/core/tests/transformers/PruneOptionalConditionBranches.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- .changeset/issue-545-sssql-pruning-mvp.md
- docs/guide/querybuilding-recipes.md
- packages/core/tests/transformers/PruneOptionalConditionBranches.test.ts
- docs/guide/sssql-optional-branch-pruning.md
This was referenced Mar 26, 2026
13 tasks
6 tasks
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
1 participant
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
Verification
Summary by CodeRabbit
New Features
Documentation
Tests
Chores