feat: use UNNEST to bypass parameter limit #226
Conversation
WalkthroughThe changes introduce the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
src/Query/Builder.php (1)
129-139: Excellent enhancement to handle large number of parameters!The changes to the
whereInmethod provide a robust solution to handle queries with a high number of parameters by leveraging theUNNESTfunction. This will significantly improve the performance and usability of the query builder when working with large datasets.Consider extracting the default threshold value (900) into a constant for better readability and maintainability. For example:
private const DEFAULT_UNNEST_THRESHOLD = 900; // ... $unnestThreshold = $this->connection->getConfig('parameter_unnest_threshold') ?? self::DEFAULT_UNNEST_THRESHOLD;
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- README.md (2 hunks)
- src/Query/Builder.php (4 hunks)
- tests/Query/BuilderTest.php (2 hunks)
Additional comments not posted (8)
src/Query/Builder.php (3)
36-36: LGTM!The addition of the
PARAMETER_LIMITconstant is approved. It provides a clear threshold for the maximum number of parameters allowed in a query.
167-173: LGTM!The change to the
whereInUnnestmethod signature to accept either astringor anExpressionfor the$columnargument is approved. This enhances the method's flexibility and usability by allowing it to handle different types of column specifications.
22-22: LGTM!The addition of the
usestatement for theExpressionclass is approved. It is required to correctly reference theExpressionclass in the updatedwhereInUnnestmethod signature.CHANGELOG.md (2)
4-4: LGTM!The change is appropriately documented in the changelog and is consistent with the PR summary.
5-5: LGTM!The change is appropriately documented in the changelog and is consistent with the PR summary.
README.md (1)
102-104: LGTM!The documentation clearly explains the limitation of binding more than 950 parameters in a single query and the driver's behavior to handle it by using
Query\Builder::whereInUnnest(...)when the parameter count exceeds theparameter_unnest_thresholdconfig value. The default value and the option to turn off the feature are also documented.tests/Query/BuilderTest.php (2)
1089-1094: LGTM!The test correctly verifies the behavior of
whereInwhen the unnest overflow flag is turned on. Passing 1000 values towhereInshould return an empty result set as expected.
1097-1105: LGTM!The test correctly verifies the behavior of
whereInwhen the unnest overflow flag is turned off. Passing 1000 values towhereInshould throw aQueryExceptionwith the expected message.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- phpstan.neon (1 hunks)
- tests/Query/BuilderTest.php (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/Query/BuilderTest.php
Additional comments not posted (1)
phpstan.neon (1)
51-53: Ensure that the code is handling the mixed type correctly.The change ignores the error message related to the constructor of the
Colopl\Spanner\Query\Nestedclass, which expects an array or anArrayableobject as the first parameter, but a mixed type was given.Ignoring the error message might be necessary if the code is intentionally passing a mixed type to the constructor. However, it's important to ensure that the code is handling the mixed type correctly to avoid runtime errors.
Please verify that the code is correctly handling the mixed type passed to the constructor. If not, consider updating the code to handle the mixed type or enforce the expected type.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/Query/Builder.php (4 hunks)
Additional comments not posted (5)
src/Query/Builder.php (5)
22-22: LGTM!The import statement is necessary for using the
Expressiontype in thewhereInUnnestmethod signature.
36-37: LGTM!The constants are used to define the parameter limit and the default threshold for using the
UNNESTfunction. The constant names are clear and follow the naming convention.
130-140: LGTM!The changes introduce a workaround to bypass the parameter limit by utilizing the
UNNESTfunction. The threshold is configurable using theparameter_unnest_thresholdconfig option, which is a good practice. The method falls back to the parent method if the threshold is not exceeded, which is a good fallback mechanism.
168-174: LGTM!The change broadens the method's flexibility in handling different types of column specifications. The change is consistent with the import statement at line 22.
Line range hint
189-193:
Tried running WHERE IN UNNEST with 1000 parameters and here is the result.
Index scan is used and everything seems to be working fine.
Summary by CodeRabbit
New Features
Documentation
Tests