Add support for a Flatten annotation #20

gaborbarna · 2018-02-03T01:09:06Z

No description provided.

BenFradet

Looks good to me, thanks a lot! 👍

Tell me when you're happy to merge.

BenFradet · 2018-02-04T17:43:47Z

core/src/main/scala/ste/encoder.scala


 import scala.collection.generic.IsTraversableOnce

 final class Meta(val metadata: Metadata) extends StaticAnnotation


Could we have made this a case class?

Yeah I think we should do that, and remove the val keyword.

kmate

Looks OK so far, but I think we should add a schema mapping function before we can consider it complete: we must convert the flattened schema back to a nested one using Dataset.select to make .as[T] work again with this.

kmate · 2018-02-05T09:10:39Z

core/src/test/scala/ste/StructTypeEncoderSpec.scala

-    StructTypeEncoder[Foo].encode shouldBe StructType(
-      StructField("a", StringType, false) ::
-      StructField("b", IntegerType, false, metadata) :: Nil
+    case class Bar(@Flatten(2) a: Seq[Foo], @Flatten(1, Seq("x", "y")) b: Map[Symbol, Foo], @Flatten c: Foo)


I think we should use Map[String, Foo] here as the keys in flatten are Strings anyway.

kmate · 2018-02-05T09:12:25Z

core/src/main/scala/ste/encoder.scala


 import scala.collection.generic.IsTraversableOnce

 final class Meta(val metadata: Metadata) extends StaticAnnotation


Yeah I think we should do that, and remove the val keyword.

BenFradet · 2018-02-13T12:53:59Z

core/src/main/scala/ste/encoder.scala

  }

-  implicit def recordEncoder[A, H <: HList, HA <: HList](
+  private def flattenFields(fields: Seq[StructField], dt: DataType, prefix: String, flatten: Flatten) =


could we specify the return type?

BenFradet · 2018-02-13T12:59:36Z

core/src/main/scala/ste/selector.scala

@@ -0,0 +1,197 @@
+package ste


could we add the license header in this file and the associated spec?

kmate · 2018-02-13T13:33:37Z

core/src/main/scala/ste/selector.scala

+}
+
+object DFUtils {
+  implicit class EnhancedDF(df: DataFrame) {


I know that these are names of implicits, but I would rename at least the class. EnhancedDF should be something like FlattenedDataFrame. Also, I would split the method, and create a public wrapper around the select only, like selectNested: DataFrame. Then asNested can be implemented with selectNested and as.

BenFradet

Some nits but it looks great 👍 . Also, could we, in a separate PR, document those new features (Metadata and Flatten) in the readme?

BenFradet · 2018-02-13T13:04:05Z

core/src/main/scala/ste/selector.scala

+    tSelector.select(dfNested, parentPrefixes, flatten.tail)
+  }
+
+  private def getChildPrefixes(prefixes: Seq[Prefix], flatten: Option[Flatten]) =


we're also missing the return type here

BenFradet · 2018-02-13T13:05:09Z

core/src/main/scala/ste/selector.scala

+  }
+
+  private def getChildPrefixes(prefixes: Seq[Prefix], flatten: Option[Flatten]) =
+    flatten.map(_ match {


we can just do

flatten.map { case ... }

BenFradet · 2018-02-13T13:05:53Z

core/src/main/scala/ste/selector.scala

+      }
+    }(breakOut)
+
+  private def orderedSelect(df: DataFrame, nestedCols: Map[Prefix, Column], fields: Seq[Prefix]) = {


missing return type here too

BenFradet · 2018-02-13T13:07:19Z

core/src/main/scala/ste/selector.scala

+
+  private def orderedSelect(df: DataFrame, nestedCols: Map[Prefix, Column], fields: Seq[Prefix]) = {
+    @tailrec
+    def loop(nestedCols: Map[Prefix, Column], fields: Seq[Prefix], cols: Seq[Column]): Seq[Column] = fields match {


could we make fields and cols lists instead of seqs?

BenFradet · 2018-02-13T13:10:23Z

core/src/test/scala/ste/StructTypeEncoderSpec.scala

-    StructTypeEncoder[Foo].encode shouldBe StructType(
-      StructField("a", StringType, false) ::
-      StructField("b", IntegerType, false, metadata) :: Nil
+    case class Bar(@Flatten(2) a: Seq[Foo], @Flatten(1, Seq("x", "y")) b: collection.Map[String, Foo], @Flatten c: Foo)


can't we have Map instead of collection.Map?

Unfortunately not, this fix hasn't been backported to spark 2.1 apache/spark#16161

ok, would you be against an upgrade to 2.2.1 in release 0.2.0?

I was wrong, they backported it to 2.1.x, I'm upgrading the patch version, I think we should do the minor update in a separate PR

BenFradet · 2018-02-13T14:10:14Z

core/src/test/scala/ste/StructTypeSelectorSpec.scala

+
+object StructSelectorSpec {
+  case class Foo(a: Int, b: String)
+  case class Bar(@Flatten(1, Seq("asd", "qwe")) foo: collection.Map[String, Foo], c: Int)


same remark for the collection.Map

gaborbarna · 2018-02-13T14:51:53Z

@BenFradet I'm going to write some documentation for these new features today hopefully

kmate · 2018-02-14T11:33:44Z

core/src/main/scala/ste/selector.scala

+}
+
+object DFUtils {
+  def selectNested[A](df: DataFrame)(implicit s: StructTypeSelector[A]): DataFrame = s.select(df, None)


I would move this into FlattenedDataFrame .

BenFradet · 2018-02-14T18:00:05Z

core/src/main/scala/ste/selector.scala

+    @tailrec
+    def loop(nestedCols: Map[Prefix, Column], fields: List[Prefix], cols: List[Column]): List[Column] = fields match {
+      case Nil => cols.reverse
+      case hd +: tail => nestedCols.find { case (p, _) => p.isParentOf(hd) } match {


since you have lists you can do hd :: tail, same thing below

good idea, unfortunately shapeless overrides the :: definition

ah, too bad :(

BenFradet · 2018-02-14T18:04:11Z

core/src/test/scala/ste/StructTypeSelectorSpec.scala

+  case class Bar(@Flatten(1, Seq("asd", "qwe")) foo: Map[String, Foo], c: Int)
+  case class Baz(@Flatten(2) bar: Seq[Bar], e: Int)
+  case class Asd(@Flatten foo: Foo, x: Int)
+}


could we move this at the top of the spec, this make things easier to understand

I moved the companion object up, it needs to be in an object for spark encoder to access the scope that those case classes were defined in.

yeah I don't have an issue with encapsulating them in an object, it's just that if we move them at the top it makes understanding the tests easier 👍

BenFradet

Looks great, two nits and I'll merge 👍 Thanks a lot for all your effort

BenFradet · 2018-02-15T10:54:42Z

Merging, thanks a lot! 👍

gaborbarna added 4 commits February 3, 2018 02:08

flattened annotation support

36d5eb5

match on DataType when flattening fields

d94f27b

cleanup prefixing

eeb2ffc

s/flattened/flatten/g

ec68115

BenFradet approved these changes Feb 4, 2018

View reviewed changes

kmate reviewed Feb 5, 2018

View reviewed changes

gaborbarna added 8 commits February 12, 2018 18:37

struct selector for df-s

6cc0220

cleanup

b3e7555

naming

e3f84b0

s/Symbol/String/

c3b66de

reorder

e55ea9d

rm unnecessary import

291f9ae

cleanup

b468719

no need for apply

3a230fc

gaborbarna changed the title ~~[WIP] feat: flattened annotation support~~ feat: flattened annotation support Feb 13, 2018

BenFradet reviewed Feb 13, 2018

View reviewed changes

add return type

924104d

BenFradet reviewed Feb 13, 2018

View reviewed changes

kmate suggested changes Feb 13, 2018

View reviewed changes

add license

b829140

BenFradet reviewed Feb 13, 2018

View reviewed changes

gaborbarna added 3 commits February 13, 2018 15:23

add selectNested convenience method

62188a6

cleanup

c230e97

disable parallelExecution in tests

df8bcb0

gaborbarna added 2 commits February 13, 2018 16:10

updating spark version to 2.1.2

f9eb2e6

use immutable.Map

a771b36

kmate approved these changes Feb 14, 2018

View reviewed changes

kmate reviewed Feb 14, 2018

View reviewed changes

move selectNested to FlattenedDataFrame

f876e91

BenFradet reviewed Feb 14, 2018

View reviewed changes

BenFradet approved these changes Feb 14, 2018

View reviewed changes

move up definitions

a783f15

BenFradet changed the title ~~feat: flattened annotation support~~ Add support for a Flatten annotation Feb 15, 2018

BenFradet merged commit 449e6c5 into BenFradet:master Feb 15, 2018


		import scala.collection.generic.IsTraversableOnce

		final class Meta(val metadata: Metadata) extends StaticAnnotation

Add support for a Flatten annotation #20

Add support for a Flatten annotation #20

Uh oh!

Conversation

gaborbarna commented Feb 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenFradet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmate left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenFradet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaborbarna commented Feb 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenFradet left a comment

Choose a reason for hiding this comment

Uh oh!

BenFradet commented Feb 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gaborbarna commented Feb 3, 2018 •

edited

Loading