Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add array operation nodes #629

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add array operation nodes #629

wants to merge 1 commit into from

Conversation

mck
Copy link
Collaborator

@mck mck commented Jan 10, 2025

User description

New Nodes

  • Unique: Efficiently removes duplicates using Set
  • Intersection: Finds common elements between arrays
  • Union: Combines arrays with duplicate removal
  • Difference: Computes set difference (A - B)
  • Shuffle: Implements Fisher-Yates shuffle algorithm

PR Type

Enhancement, Tests


Description

  • Added five new array operation nodes: unique, intersection, union, difference, and shuffle.

  • Implemented comprehensive tests for all new nodes, covering edge cases and type validation.

  • Updated array node index to include the new nodes.

  • Documented changes in a changeset file for versioning.


Changes walkthrough 📝

Relevant files
Enhancement
6 files
difference.ts
Added `difference` node to compute array difference.         
+47/-0   
index.ts
Updated array node index to include new nodes.                     
+11/-1   
intersection.ts
Added `intersection` node to find common array elements. 
+46/-0   
shuffle.ts
Added `shuffle` node to randomly reorder array elements. 
+43/-0   
union.ts
Added `union` node to combine arrays and remove duplicates.
+45/-0   
unique.ts
Added `unique` node to remove duplicate elements from arrays.
+34/-0   
Tests
5 files
difference.test.ts
Added tests for `difference` node functionality and edge cases.
+77/-0   
intersection.test.ts
Added tests for `intersection` node functionality and edge cases.
+70/-0   
shuffle.test.ts
Added tests for `shuffle` node functionality and edge cases.
+73/-0   
union.test.ts
Added tests for `union` node functionality and edge cases.
+71/-0   
unique.test.ts
Added tests for `unique` node functionality and edge cases.
+52/-0   
Documentation
1 files
pink-roses-admire.md
Documented new array operation nodes in changeset.             
+12/-0   

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

Copy link

changeset-bot bot commented Jan 10, 2025

🦋 Changeset detected

Latest commit: 20a2a38

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@tokens-studio/graph-engine Minor

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

Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Type Validation

Ensure the type validation logic in the difference node's execute method is robust and correctly handles edge cases where input types might not match.

//Verify types
if (this.inputs.a.type.$id !== this.inputs.b.type.$id) {
	throw new Error('Array types must match');
Input Mutation

Verify that the shuffle node's logic for creating a copy of the input array before shuffling effectively prevents unintended mutations of the original input array.

// Create a copy of the array to avoid mutating the input
const shuffled = [...array];

Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible issue
Ensure inputs are validated to avoid runtime errors when they are null or undefined

Add a null or undefined check for inputs a and b in the execute method to prevent
runtime errors when inputs are not properly set.

packages/graph-engine/src/nodes/array/difference.ts [34]

 const { a, b } = this.getAllInputs();
+if (!a || !b) {
+    throw new Error('Inputs a and b must be defined');
+}
Suggestion importance[1-10]: 8

Why: Adding a null or undefined check for inputs a and b is a critical improvement to prevent runtime errors when inputs are not properly set. This ensures robustness and avoids potential crashes during execution.

8
Validate inputs to ensure they are not null or undefined before processing

Add a null or undefined check for inputs a and b in the execute method to prevent
runtime errors when inputs are not properly set.

packages/graph-engine/src/nodes/array/intersection.ts [33]

 const { a, b } = this.getAllInputs();
+if (!a || !b) {
+    throw new Error('Inputs a and b must be defined');
+}
Suggestion importance[1-10]: 8

Why: Adding a null or undefined check for inputs a and b is essential to prevent runtime errors and ensure the method operates on valid data. This enhances the reliability of the code.

8
Validate the input array to ensure it is not null or undefined before shuffling

Add a null or undefined check for the input array in the execute method to prevent
runtime errors when the input is not properly set.

packages/graph-engine/src/nodes/array/shuffle.ts [30]

 const { array } = this.getAllInputs();
+if (!array) {
+    throw new Error('Input array must be defined');
+}
Suggestion importance[1-10]: 8

Why: Adding a null or undefined check for the input array is a necessary safeguard to prevent runtime errors and ensure the method processes valid input. This improves the robustness of the code.

8

Copy link
Contributor

@AlexBxl AlexBxl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The node code looks correct, but the last test for some of the nodes fails because of .rejects.toThrow().

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants