Improve coll.flatten stability for large collections#61
Improve coll.flatten stability for large collections#61shahar-biron wants to merge 11 commits intomainfrom
Conversation
Co-Authored-By: Warp <agent@warp.dev>
handling toRemove null Co-authored-by: typo-app[bot] <139475626+typo-app[bot]@users.noreply.github.com>
Co-Authored-By: Warp <agent@warp.dev>
Co-Authored-By: Warp <agent@warp.dev>
Co-Authored-By: Warp <agent@warp.dev>
|
Static Code Review 📊 ✅ All quality checks passed! |
📝 WalkthroughWalkthroughIntroduces four new collection utility functions ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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 |
![typo-app[bot]](https://avatars.githubusercontent.com/in/360763?s=60&v=4){kind=small}
| // Seed the Set with `a` and copy `a` to result. | ||
| // This preserves the relative order of elements in `a`. | ||
| const set = new Set(a); | ||
| const result = a.slice(); |
There was a problem hiding this comment.
logic: union may return duplicates from the first array. Initialize result from the Set to ensure uniqueness.
| const result = a.slice(); | |
| const result = [...set]; |
![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
🧹 Nitpick comments (2)
src/collections/union.js (1)
23-53: LGTM! Well-optimized allocation-conscious implementation.The multi-branch approach effectively avoids creating intermediate concatenated arrays, reducing GC pressure. The indexed loop (lines 44-50) is more stable than spread operations for large arrays, and the clear documentation makes the optimization rationale explicit.
Optional: Lines 31-36 could be simplified
The empty-array checks (lines 31-36) could be removed since the main logic (lines 38-50) handles these cases correctly. However, the current approach avoids unnecessary Set operations and allocations for edge cases, so the current implementation is a reasonable performance tradeoff.
- const lenA = a.length; - const lenB = b.length; - - if (lenA === 0 && lenB === 0) return []; - if (lenA === 0) return [...new Set(b)]; - if (lenB === 0) return [...new Set(a)]; - // Seed the Set with `a` and copy `a` to result.src/collections/flatten.js (1)
5-15: Optional: Consider broadening the type annotation.The
@paramtype is{Array|null}, but the implementation accepts any type and returns[]for non-arrays. Consider using{*}or{any}to better reflect the defensive behavior.📝 Suggested JSDoc refinement
/** * One-level flatten of a nested list. * * - If `nested` is not an array, returns an empty list. * - For each element: * - If it is an array, its elements are concatenated. * - Otherwise, the element is included as-is. * - * @param {Array|null} nested + * @param {*} nested - The value to flatten (expected to be an array) * @returns {Array} */
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/collections/containsAll.jssrc/collections/containsAny.jssrc/collections/flatten.jssrc/collections/subtract.jssrc/collections/union.jstests/collections/subtractContainsFlatten.test.js
🧰 Additional context used
🧬 Code graph analysis (3)
src/collections/containsAll.js (1)
src/collections/containsAny.js (1)
set(21-21)
src/collections/containsAny.js (1)
src/collections/containsAll.js (1)
set(21-21)
tests/collections/subtractContainsFlatten.test.js (2)
tests/setup.js (1)
graph(12-12)src/collections/flatten.js (1)
result(19-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (6)
src/collections/subtract.js (1)
17-26: LGTM! Clean and efficient implementation.The function correctly handles all edge cases and uses a Set for efficient O(1) lookups. The shallow copy behavior for non-array
toRemoveis well-documented and intuitive.src/collections/containsAny.js (1)
16-27: LGTM! Efficient short-circuit logic.The implementation correctly uses a Set for O(1) membership checks and short-circuits on the first match. The loose equality check on line 23 appropriately handles both null and undefined as documented.
src/collections/containsAll.js (1)
16-27: LGTM! Correct implementation with proper vacuous truth handling.The function correctly implements universal quantification semantics, including returning
truefor empty candidates (vacuous truth). The Set-based implementation ensures efficient lookups.tests/collections/subtractContainsFlatten.test.js (1)
1-111: Test file is comprehensive and correctly structured.The test suite appropriately validates both graph query results and local module behavior for all four utilities (subtract, containsAll, containsAny, flatten) with good coverage of edge cases including null inputs and empty arrays. All test expectations align with the corresponding implementations.
src/collections/flatten.js (2)
36-45: Registration and export pattern looks good.The
falkor.registercall and conditional CommonJS export follow the project's established pattern for dual-environment support, consistent with the other collection utilities added in this PR.
16-34: LGTM! Solid implementation that achieves the stability goal.The indexed loop approach (lines 23-28) correctly avoids the argument explosion that would occur with
push(...item)on large arrays, addressing the core objective. The logic properly handles both array and non-array elements in the one-level flatten.
Co-Authored-By: Warp <agent@warp.dev>
typo-app
bot
added
Manual Review Time 30-60 min
and removed
Manual Review Time 30-60 min
labels
|
|
||
| const { initializeFLEX } = require('../setup'); | ||
|
|
||
| const subtractModule = require('../../src/collections/subtract'); |
There was a problem hiding this comment.
Break this test file to multiple ones,
there should be a test file for each Flex function.
Co-Authored-By: Warp <agent@warp.dev>
typo-app
bot
added
Manual Review Time 30-60 min
and removed
Manual Review Time 30-60 min
labels
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/test.yml (1)
46-49: Configuration looks correct; previous issue resolved.The inputs are correctly configured per the action's v2 documentation:
skip-step: allproperly skips install and test execution (tests already ran at line 32)github-tokenandcoverage-fileare correctly specifiedThe previous issue with the invalid
skip-stepvalue has been successfully addressed.Optional: Pin to a more specific version
For improved security and reproducibility, consider pinning to a specific commit SHA or at least a minor version instead of the major version
@v2:- uses: ArtiomTr/jest-coverage-report-action@v2 + uses: ArtiomTr/jest-coverage-report-action@v2.x.x # Replace with latest v2 minor versionOr use a commit SHA:
- uses: ArtiomTr/jest-coverage-report-action@v2 + uses: ArtiomTr/jest-coverage-report-action@abc1234... # Full commit SHA
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
Co-Authored-By: Warp <agent@warp.dev>
typo-app
bot
added
Manual Review Time 30-60 min
and removed
Manual Review Time 30-60 min
labels
Co-Authored-By: Warp <agent@warp.dev>
Co-Authored-By: Warp <agent@warp.dev>
Co-Authored-By: Warp <agent@warp.dev>
typo-app
bot
added
Manual Review Time 1+ hours
and removed
Manual Review Time 30-60 min
labels
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/collections/containsAny.test.js (1)
37-43: Consider expanding test coverage for edge cases.The local module tests correctly verify the graph query results and add an extra null check. Consider adding test cases for:
containsAny([], [])→ false (both empty)containsAny([1], [1])→ true (exact match)containsAny([1,2], [1,2,3])→ true (partial overlap)These would strengthen confidence in boundary behavior.
tests/collections/flatten.test.js (1)
37-42: Consider adding tests for deeply nested arrays.The local module tests correctly verify one-level flattening. To make the "one-level" behavior more explicit, consider adding:
flatten([[[1,2]]])→[[1,2]](demonstrates only one level is flattened)flatten([])→[](empty input)This would clarify the expected depth behavior and prevent future confusion.
tests/collections/containsAll.test.js (1)
37-43: Consider expanding edge case coverage.The local module tests correctly mirror the graph query and add symmetric null handling. Consider adding:
containsAll([], [1])→ false (empty list doesn't contain elements)containsAll([1,2], [1,2])→ true (exact match)containsAll([1,1,2], [1])→ true (duplicates in list)These would provide more comprehensive edge case coverage.
tests/collections/subtract.test.js (1)
35-41: LGTM! Consider adding more edge cases.The local module tests correctly verify the graph query results and add graceful null handling. The comment on line 39 is helpful for understanding the design decision.
Consider adding tests for:
subtract([1,2,2,3], [2])→[1,3](verify all occurrences removed)subtract([1,2,3], [4,5])→[1,2,3](no matching elements)subtract([1,2,3], [1,2,3])→[](remove all)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/test.ymltests/collections/containsAll.test.jstests/collections/containsAny.test.jstests/collections/flatten.test.jstests/collections/subtract.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test.yml
🧰 Additional context used
🧬 Code graph analysis (2)
tests/collections/containsAll.test.js (4)
tests/collections/containsAny.test.js (2)
require(5-5)db(9-9)tests/collections/flatten.test.js (2)
require(5-5)db(9-9)tests/collections/subtract.test.js (2)
require(5-5)db(9-9)tests/setup.js (1)
graph(12-12)
tests/collections/containsAny.test.js (4)
tests/collections/containsAll.test.js (2)
require(5-5)db(9-9)tests/collections/flatten.test.js (2)
require(5-5)db(9-9)tests/collections/subtract.test.js (2)
require(5-5)db(9-9)tests/setup.js (1)
graph(12-12)
🔇 Additional comments (8)
tests/collections/containsAny.test.js (2)
1-21: LGTM!The test setup and teardown are correctly structured with async lifecycle hooks and proper cleanup.
23-36: LGTM!The graph query test cases cover the essential scenarios: intersection detection, no intersection, empty candidate list, and null input handling.
tests/collections/flatten.test.js (2)
1-21: LGTM!The test structure follows the same pattern as other collection tests with proper async lifecycle management.
23-35: LGTM!The test cases effectively demonstrate one-level flatten behavior with nested arrays, mixed types, empty arrays, and null input handling.
tests/collections/containsAll.test.js (2)
1-21: LGTM!The test structure is consistent with other collection tests and properly manages the database lifecycle.
23-35: LGTM!The test cases correctly cover subset checking, partial match failure, empty candidate list (vacuous truth), and null input handling.
tests/collections/subtract.test.js (2)
1-21: LGTM!The test structure follows the established pattern with proper async lifecycle management.
23-33: LGTM!The test cases effectively verify subtract behavior: removing all occurrences, handling empty removal lists, and null input handling.
2 participants
Replace use of push(...item) with indexed loops in coll.flatten to avoid argument explosion and improve stability on large inner arrays.\n\n- Preserve existing one-level flatten semantics and behavior for non-array inputs.\n- All 27 test suites (70 tests) pass against a local FalkorDB instance.\n\nCo-Authored-By: Warp agent@warp.dev
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
PR Summary by Typo
Overview
This PR improves the stability of
coll.flattenfor large collections and enhancescoll.unionfor better performance. It also introduces new collection utility functions:containsAll,containsAny, andsubtract.Key Changes
coll.flattento manually push elements, avoiding potential argument explosion with very large inner arrays.coll.unionto be more allocation- and garbage-collection-friendly by streamlining its internal logic.coll.containsAll(checks if a list contains all specified candidates),coll.containsAny(checks if a list contains any specified candidate), andcoll.subtract(returns a list with specified elements removed).Work Breakdown
To turn off PR summary, please visit Notification settings.