flytekit-java icon indicating copy to clipboard operation
flytekit-java copied to clipboard

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

Open jschuchart-spot opened this issue 1 year ago • 2 comments

TL;DR

Adding explicit support for scala.Option type

Type

  • [ ] Bug Fix
  • [x] Feature
  • [ ] Plugin

Are all requirements met?

  • [x] Code completed
  • [x] Smoke tested
  • [x] Unit tests added
  • [x] 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

jschuchart-spot avatar Jul 04 '24 09:07 jschuchart-spot

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).

welcome[bot] avatar Jul 04 '24 09:07 welcome[bot]

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.

jschuchart-spot avatar Jul 26 '24 18:07 jschuchart-spot

Congrats on merging your first pull request! 🎉

welcome[bot] avatar Aug 02 '24 09:08 welcome[bot]