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

#patch Adding support for SdkBindingData[scala.Option[_]] #308

Merged

Conversation

jschuchart-spot
Copy link
Contributor

TL;DR

Adding explicit support for scala.Option type

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

Added explicit special case for Option type when instantiating a generics literal type. Since support for any case classes in SdkLiteralTypes has been introduced, it's not immediately obvious why this works

case class Example(value: Option[String])
SdkLiteralTypes.of[Example]()

but this doesn't:

SdkLiteralTypes.of[Option[String]]()

The latter case is especially useful for input classes to workflows and SdkTransforms of the form

case class Input(
  inputStr: SdkBindingData[String],
  optional: SdkBindingData[Option[String]],
  ...
)

The Option type can't be instantiated directly like a common case class, so it needs to be instantiated differently.
I believe the common usage of Option as a field type in scala classes justifies this special handling.

Tracking Issue

NA

Follow-up issue

NA

Copy link

welcome bot commented Jul 4, 2024

Thank you for opening this pull request! 🙌
These tips will help get your PR across the finish line: - Most of the repos have a PR template; if not, fill it out to the best of your knowledge. - Sign off your commits (Reference: DCO Guide).

Comment on lines -239 to +236
// is also super class of, for example, Option and Tuple
// is also super class of, for example, Either or Try
Copy link
Contributor Author

@jschuchart-spot jschuchart-spot Jul 4, 2024

Choose a reason for hiding this comment

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

Tuple is actually also just a case class and already works with the current implementation. The problem arises when there is an abstract Product type with several specific implementations like Either or Try

Comment on lines +419 to +422
map
.get("value")
.map(valueToParamValue(_, typeOf[S].typeArgs.head))
.asInstanceOf[S]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since map.get(...) already produces an Option, we can just go with that and map the inner value if present

.map(valueToParamValue(_, typeOf[S].typeArgs.head))
.asInstanceOf[S]
else
instantiateViaConstructor(clazz)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pulled out for readability

Comment on lines -75 to +76
case t if t <:< typeOf[Product] && !(t =:= typeOf[Option[_]]) =>
case t if t <:< typeOf[Product] =>
Copy link
Contributor Author

@jschuchart-spot jschuchart-spot Jul 4, 2024

Choose a reason for hiding this comment

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

This check for avoiding Option was not successful before. It would need to use <:< instead of =:=.

@jschuchart-spot jschuchart-spot marked this pull request as ready for review July 4, 2024 09:19
Comment on lines -33 to +37
product.getClass.getDeclaredFields.map(_.getName).toList
val methodNames = product.getClass.getDeclaredMethods.map(_.getName)
product.getClass.getDeclaredFields
.map(_.getName)
.filter(methodNames.contains)
.toList
Copy link
Contributor Author

@jschuchart-spot jschuchart-spot Jul 5, 2024

Choose a reason for hiding this comment

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

This gave me ["serialVersionUUID", "value"] for Option which meant that the stored map used "serialVersionUUID" as the key for the value because this list is zipped with the productElements.
Checking against the method names solves this also for case classes.

Copy link
Collaborator

@andresgomezfrr andresgomezfrr left a comment

Choose a reason for hiding this comment

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

What do you think about add a specific of factor for Options.

ofOption(value) => SdkBindingData[Option[_]]

and

of(Option(value)) => SdkBindingData[Option[_]]

to avoid to need to use the generics

@jschuchart-spot
Copy link
Contributor Author

What do you think about add a specific of factor for Options.

ofOption(value) => SdkBindingData[Option[_]]

and

of(Option(value)) => SdkBindingData[Option[_]]

to avoid to need to use the generics

How and why do you mean to avoid generics exactly? You'd still want a SdkBindingData[Option[T]] as a result like you have for collections and maps. To avoid generics entirely would require adding ofStringOption, ofIntegerOption, etc. which may be bloating the code.
What could be useful in my opinion may be ofOption[T](value: T): SdkBindingData[Option[T]] and/or of[T : TypeTag](option: Option[T]): SdkBindingData[Option[T]]. The reason being that this of factory will otherwise only work if it is a defined Option (similar to collections and maps needing to contain at least one value), which seems an odd restriction in this case.

@andresgomezfrr andresgomezfrr merged commit e647d77 into flyteorg:master Aug 2, 2024
4 checks passed
Copy link

welcome bot commented Aug 2, 2024

Congrats on merging your first pull request! 🎉

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.

2 participants