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

[WX-963] Unzip Engine Function #7368

Merged
merged 26 commits into from
Feb 27, 2024
Merged

[WX-963] Unzip Engine Function #7368

merged 26 commits into from
Feb 27, 2024

Conversation

THWiseman
Copy link
Contributor

Implemented the unzip() engine function, new with WDL 1.1.

As a new WDL 1.1 feature, it was implemented for Biscayne and Cascades, but not earlier versions (e.g. draft-2, draft-3).

@THWiseman THWiseman requested a review from a team as a code owner February 21, 2024 20:57
Copy link
Contributor

@aednichols aednichols left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Contributor

@jgainerdewar jgainerdewar left a comment

Choose a reason for hiding this comment

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

Looks great!

Reviewing this made me think about how we don't seem to have much unit testing of failure modes (my suffix PR suffered from the same issue) - what if I run a WDL with unzip([("banana", 5), (6, "apple"])? Is that valid, or does it fail because we can't find good types for the output arrays?

@@ -64,4 +64,12 @@ class CascadesTypeEvaluatorSpec extends AnyFlatSpec with CromwellTimeoutSpec wit
e.evaluateType(Map.empty) shouldBeValid WomArrayType(WomStringType)
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

There's another test in the Biscayne version, should it be added here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we're still missing a test here that is is Biscayne, no? Once that's in I think this is good to go!

@THWiseman
Copy link
Contributor Author

Looks great!

Reviewing this made me think about how we don't seem to have much unit testing of failure modes (my suffix PR suffered from the same issue) - what if I run a WDL with unzip([("banana", 5), (6, "apple")])? Is that valid, or does it fail because we can't find good types for the output arrays?

Agree with this sentiment! I added a few unit tests for basic failure modes.

To your more specific question: unzip([("banana", 5), (6, "apple")]) will succeed, because putting stuff into arrays homogenizes the type, and the integers are coercible to strings. For that to fail, I think we would want arrays to throw errors instead of attempting to homogenize types, which is a big and breaking change that we probably shouldn't make.

However, unzip ([(1, 2), ([1,2,3], 3)]) will fail because there's no way to coerce between int and array[int]. Added a test case to assert this.

@THWiseman THWiseman merged commit 5e59b02 into develop Feb 27, 2024
33 of 34 checks passed
@THWiseman THWiseman deleted the WX-963 branch February 27, 2024 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants