[SPARK-23938][SQL] Add map_zip_with function #22017

mn-mikke · 2018-08-06T23:50:20Z

What changes were proposed in this pull request?

This PR adds a new SQL function called map_zip_with. It merges the two given maps into a single map by applying function to the pair of values with the same key.

How was this patch tested?

Added new tests into:

DataFrameFunctionsSuite.scala
HigherOrderFunctionsSuite.scala

mn-mikke · 2018-08-06T23:51:11Z

cc @ueshin @mgaido91 @hvanhovell

SparkQA · 2018-08-07T04:04:20Z

Test build #94325 has finished for PR 22017 at commit ef56011.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class ArrayDataMerger(elementType: DataType)
case class ArrayUnion(left: Expression, right: Expression) extends ArraySetLike
abstract class GetMapValueUtil extends BinaryExpression with ImplicitCastInputTypes
case class MapZipWith(left: Expression, right: Expression, function: Expression)

…ng confilicts.

ueshin · 2018-08-07T10:42:05Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala

+    }
+  }
+
+  private def getMapType(expr: Expression) = expr.dataType match {


I'd like you to use the same util method. I suggested to introduce object HigherOrderFunction at #21986 (comment).

ueshin · 2018-08-07T10:42:18Z

...yst/src/test/scala/org/apache/spark/sql/catalyst/expressions/HigherOrderFunctionsSuite.scala

+    nullable2: Boolean,
+    dt3: DataType,
+    nullable3: Boolean,
+    f: (Expression, Expression, Expression) => Expression): Expression = {


nit: indent

mgaido91 · 2018-08-07T10:48:45Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala

-    case BinaryType => false
-    case _: AtomicType => true
-    case _ => false
+  @transient protected lazy val elementTypeSupportEquals = {


nit: I think we can avoid the braces

mgaido91 · 2018-08-07T11:05:33Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala

+    val keys = arrayDataUnion(mapData1.keyArray(), mapData2.keyArray())
+    val values = new GenericArrayData(new Array[Any](keys.numElements()))
+    keys.foreach(keyType, (idx: Int, key: Any) => {
+      val v1 = GetMapValueUtil.getValueEval(mapData1, key, keyType, leftValueType, ordering)


This approach is very inefficient. The computational complexity is very high (N^2 in the size of the biggest map). I think here can implement something more efficient avoid also the changes for the code refactoring. I'd propose to get also the index where a key has been found in each map, so that we can access the values by index. In this way the overall complexity would be O(N).

Thanks for mentioning this! I'm not happy with the current complexity either. I've assumed that the implementation of maps will change into something with O(1) element access in future. By then, the complexity would be O(N) for types supporting equals as well and we would safe a portion of duplicated code.

If you think that maps will remain like this for a long time, really like your suggestion with indexes.

@ueshin What's your view on that?

I think there is no plan to have a different map implementation and anyway there is a lot of code which depends on having the array based version of MapData. Regarding the duplicated code, to be honest, I think that avoiding the refactoring introduced by that would also make this PR cleaner...

Ok, I will change it. Thanks a lot!

mgaido91 · 2018-08-07T11:06:53Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameFunctionsSuite.scala

+  }
+
+  test("map_zip_with function - invalid")
+  {


nit: please move the brace at the end of the previous line (there are other place where this should be done, please update them too)

SparkQA · 2018-08-07T12:30:54Z

Test build #94362 has finished for PR 22017 at commit ec583eb.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class ArrayDataUnion(elementType: DataType) extends ((ArrayData, ArrayData) => ArrayData)
case class ArrayUnion(left: Expression, right: Expression) extends ArraySetLike

xuanyuanking · 2018-08-07T14:39:14Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala

-
-abstract class GetMapValueUtil extends BinaryExpression with ImplicitCastInputTypes {
+object GetMapValueUtil
+{


nit: brace should in previous line.

xuanyuanking · 2018-08-07T14:46:14Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala

+
+  override def functions: Seq[Expression] = function :: Nil
+
+  override def nullable: Boolean = left.nullable || right.nullable


left.nullable && right.nullable? Because if one side is empty map, NULL will be passed as the value for each key in other side.

nullable flag is rather related to the cases when the whole map is null. The case that you are referring to is handled by valueContainsNull flag of MapType (see the line 496).

# Conflicts: # sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala

SparkQA · 2018-08-08T01:40:23Z

Test build #94394 has finished for PR 22017 at commit 12ad8b2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-08-08T01:48:43Z

Test build #94393 has finished for PR 22017 at commit 89a3da4.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

mgaido91 · 2018-08-08T08:48:54Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala

+  }
+
+  private def getKeysWithIndexesFast(keys1: ArrayData, keys2: ArrayData) = {
+    val arrayBuffer = new mutable.ArrayBuffer[(Any, Array[Option[Int]])]


why Array[Option[Int]] instead of (Option[Int], Option[Int])? Moreover, I can't understand why we need this at all. As we have the HashMap, we can just add there the indexes and return it as an array..

If we changed it to (Option[Int], Option[Int]), wouldn't we need two similar i loops instead of one?

My motivation for using also the ArrayBuffer is preserve the order of keys. A random order would break map comparison in tests. Maybe you will come with idea how to compare maps in tests better :-)

If we changed it to (Option[Int], Option[Int]), wouldn't we need two similar i loops instead of one?

I really don't think so, it would be the same as now I think

well, maybe we can fix map comparison in tests... :)

I really don't think so, it would be the same as now I think

Let's assume that indexes are tuple for now. indexes(z).isEmpty could replace with indexes.productElement(z).isEmpty, but how to replace indexes(z) = Some(i)? Since tuple is immutable, I don't see how to replace ith element with copy function. Maybe we could implement a dedicated class to hold indexes, but is it worth doing that?

since the HashMap is mutable, you can just: hashMap += key -> newTuple

The array based solution is 20% faster indeed according to my benchmark, but I think it is not critical as I run the benchmark performing the operation 1.000.000 times and the absolute difference was 2 ms. So I prefer the cleaner solution (that is using tuples). @ueshin what do you think?

I generally prefer the cleaner solution, but actually I'd prefer the previous approach in this case for 2 reasons:

We shouldn't ignore 20% of performance difference.

I'm not sure we can modify the comparison of MapType in ExpressionEvalHelper here. We might need another pr to make sure the modification is valid.

We still need to add comments what the arrays are for and the reason, though.

@mgaido91 Are you comfortable with reverting back to the previous version?

@mn-mikke we can use LinkedHashMap in order to preserve key order.

Like this idea, thanks!

mgaido91 · 2018-08-08T08:53:27Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala

+  private def getKeysWithIndexesFast(keys1: ArrayData, keys2: ArrayData) = {
+    val arrayBuffer = new mutable.ArrayBuffer[(Any, Array[Option[Int]])]
+    val hashMap = new mutable.OpenHashMap[Any, Array[Option[Int]]]
+    val keys = Array(keys1, keys2)


maybe better to do something like for (arr <- Seq(keys1, keys2))?

mgaido91 · 2018-08-08T08:58:32Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala

+          j += 1
+        }
+        if (!found) {
+          assertSizeOfArrayBuffer(arrayBuffer.size)


shall we check this only once at the end in order to avoid the overhead at each iteration?

The purpose of this line is to avoid OutOfMemoryError exception when max array size is exceeded and throw something more accurate. Maybe I'm missing something, but wouldn't we break it we checked this only once at the end? The max size could be exceeded in any iteration.

I see, because you are using an ArrayBuffer....makes sense, thanks

mgaido91 · 2018-08-08T09:01:40Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala

+    arrayBuffer
+  }
+
+  private def getValue(valueData: ArrayData, eType: DataType, index: Option[Int]) = index match {


do we really need this? It can be index.map(valueData.get(_, eType)).getOrElse(null) and we are using it only in one place (twice, but in the same place)...

SparkQA · 2018-08-09T00:26:12Z

Test build #94458 has finished for PR 22017 at commit 38ce4e7.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

ueshin · 2018-08-09T04:27:59Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala

+
+  @transient lazy val functionForEval: Expression = functionsForEval.head
+
+  @transient lazy val (keyType, leftValueType, _) =


keyType should be TypeCoercion.findTightestCommonType(leftKeyType, rightKeyType)?

You are right, thanks!

WDYT about introducing a coercion rule handling different key types? For cases like (IntType, LongType) might be handy.

Sounds good. Thanks!

ueshin · 2018-08-09T04:28:05Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala

+    var i = 0
+    while (i < keys1.numElements) {
+      val key = keys1.get(i, keyType)
+      if(!hashMap.contains(key)) hashMap.put(key, (Some(i), None))


nit: let's use brackets:

if (!hashMap.contains(key)) { hashMap.put(key, (Some(i), None)) }

ueshin · 2018-08-09T04:28:10Z

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala

-          if (unsafeRow != expectedRow) {
+          val field = StructField("field", expression.dataType)
+          val dataType = StructType(field :: field :: Nil)
+          if (!checkResult(unsafeRow, expectedRow, dataType)) {


What's this for?

UnsafeRows are compared based on equality of backing arrays. This approach doesn't work well when ignoring order in unsafe representation of maps.

This reverts commit 6aeaaa8

…indexes represented as arrays.

SparkQA · 2018-08-09T20:49:16Z

Test build #94508 has finished for PR 22017 at commit 5d2a78e.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-08-10T03:00:02Z

Test build #94526 has finished for PR 22017 at commit 3c849cb.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

ueshin · 2018-08-10T06:31:00Z

LGTM.
@mgaido91 Do you have any other comments on this?

mgaido91

LGTM too, apart from one question

mgaido91 · 2018-08-10T06:52:26Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala

+    HigherOrderFunction.mapKeyValueArgumentType(right.dataType)
+
+  @transient lazy val keyType =
+    TypeCoercion.findTightestCommonType(leftKeyType, rightKeyType).getOrElse(NullType)


why do we need this? We are enforcing that the two maps have the same key type, can't we just get one?

Even though there is a coercion rule for unification of key types. The key types may differ in nullability flags if they are complex. In theory, we could use == and findTightestCommonType in the coercion rule since there is no codegen to be optimized for null checks. But unfortunatelly, bind gets called once before execution of coercion rules, so findTightestCommonType is important for setting up a correct input type for lamda function.

Maybe, we could play with order of analysis rules, but I'm not sure about all the consequences. @ueshin could shad some light on analysis rules ordering?

Yeah, the current analysis rules order might cause some problem. Let me think about it for a while.

I submitted a pr to fix analysis rules order to fix argument types before bind #22075.
Btw, we should use findCommonTypeDifferentOnlyInNullFlags for this because after the type coercion, the difference between two key types must be only nullabilities. Sorry for confusing you.

IMHO, if checkInputDataTypes was executed before bind, findTightestCommonType could play the same role. But yeah, findCommonTypeDifferentOnlyInNullFlags will be semantically more accurate. Thanks!

After #22075, checkArgumentDataType() introduced in it will be executed before bind, so the key types should be "sameType" and we will be able to use findCommonTypeDifferentOnlyInNullFlags. We still need checkInputDataTypes to be executed after bind to check the whole data types are valid.

Oh, I see. We also need to check the output data type of lambda functions for the expressions like ArrayFilter.

Good catch! Year, we need it.

ueshin · 2018-08-10T16:01:20Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala

+  /**
+   * Similar to [[findTightestCommonType]] but with string promotion.
+   */
+  def findWiderTypeForTwoExceptDecimals(t1: DataType, t2: DataType): Option[DataType] = {


Why except Decimals?

If we have maps with decimals of different precision as keys. Cast will fail in analysis phase since it can't cast a key to nullable (potential lost of precision). IMHO, the type mismatch exception from this function will be more accurate. WDYT?

Ah, I see, good catch! But it led me to another issue. We can't choose those types possibly to be null as a map key. Instead of adding the method, how about modifying findTypeForComplex as something like:

private def findTypeForComplex( t1: DataType, t2: DataType, findTypeFunc: (DataType, DataType) => Option[DataType]): Option[DataType] = (t1, t2) match { ... case (MapType(kt1, vt1, valueContainsNull1), MapType(kt2, vt2, valueContainsNull2)) => findTypeFunc(kt1, kt2) .filter(kt => !Cast.forceNullable(kt1, kt) && !Cast.forceNullable(kt2, kt)) .flatMap { kt => findTypeFunc(vt1, vt2).map { vt => MapType(kt, vt, valueContainsNull1 || valueContainsNull2) } } ... }

We might need to have another pr to discuss this.

cc @cloud-fan @gatorsmile

On second thoughts, do we really need those? Seems like the current coercions rules don't contain possibly cast to null?

ok, I see that this is a matter of findTypeForComplex. I'll submit another pr later. Maybe we can go back to findWiderTypeForTwo in TypeCoercion and findCommonTypeDifferentOnlyInNullFlag for keyType.

I submitted a pr #22086.

Thanks for both your PRs! I will submit changes once they get in.

SparkQA · 2018-08-10T20:17:30Z

Test build #94570 has finished for PR 22017 at commit 595161f.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

mn-mikke · 2018-08-10T20:29:44Z

retest this please

SparkQA · 2018-08-10T23:41:39Z

Test build #94585 has finished for PR 22017 at commit 595161f.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-08-13T10:05:58Z

retest this please

SparkQA · 2018-08-13T16:47:27Z

Test build #94685 has finished for PR 22017 at commit 595161f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

…NullFlags + coercion rule refactoring

SparkQA · 2018-08-14T01:49:13Z

Test build #94712 has finished for PR 22017 at commit 2b7e9e5.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

ueshin

LGTM.

ueshin · 2018-08-14T02:58:51Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala

+case class MapZipWith(left: Expression, right: Expression, function: Expression)
+  extends HigherOrderFunction with CodegenFallback {
+
+  @transient lazy val functionForEval: Expression = functionsForEval.head


nit: shall we use def here to follow the comment #21954 (comment)?

ueshin · 2018-08-14T03:36:37Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala

+  }
+
+  // Nothing to check since the data type of the lambda function can be anything.
+  override def checkInputDataTypes(): TypeCheckResult = TypeCheckResult.TypeCheckSuccess


I'd call checkArgumentDataTypes() here again.

ueshin · 2018-08-14T03:40:25Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameFunctionsSuite.scala

+        Row(null)))
+  }
+
+  test("map_zip_with function - map of complex types") {


nit: non-primitive instead of complex?

mgaido91

LGTM

mgaido91 · 2018-08-14T08:10:59Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala

+  @transient lazy val MapType(rightKeyType, rightValueType, rightValueContainsNull) = right.dataType
+
+  @transient lazy val keyType =
+    TypeCoercion.findCommonTypeDifferentOnlyInNullFlags(leftKeyType, rightKeyType).get


shouldn't the null flag be false for both them?

If leftKeyType is ArrayType(IntegerType, false) and rightKeyType is ArrayType(IntegerType, true) for instance, the coercion rule is not executed leftKeyType.sameType(rightKeyType) == true.

An array with nulls seems to be a valid key.:

scala> spark.range(1).selectExpr("map(array(1, 2, null), 12)").show() +---------------------------------------+ |map(array(1, 2, CAST(NULL AS INT)), 12)| +---------------------------------------+ | [[1, 2,] -> 12]| +---------------------------------------+

I see, thanks

SparkQA · 2018-08-14T11:56:07Z

Test build #94737 has finished for PR 22017 at commit bcd4e0f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

ueshin · 2018-08-14T12:12:39Z

Thanks! merging to master.

…should be true. ## What changes were proposed in this pull request? This is a follow-up pr of #22017 which added `map_zip_with` function. In the test, when creating a lambda function, we use the `valueContainsNull` values for the nullabilities of the value arguments, but we should've used `true` as the same as `bind` method because the values might be `null` if the keys don't match. ## How was this patch tested? Added small tests and existing tests. Closes #22126 from ueshin/issues/SPARK-23938/fix_tests. Authored-by: Takuya UESHIN <ueshin@databricks.com> Signed-off-by: Takuya UESHIN <ueshin@databricks.com>

## What changes were proposed in this pull request? - Revert [SPARK-23935][SQL] Adding map_entries function: #21236 - Revert [SPARK-23937][SQL] Add map_filter SQL function: #21986 - Revert [SPARK-23940][SQL] Add transform_values SQL function: #22045 - Revert [SPARK-23939][SQL] Add transform_keys function: #22013 - Revert [SPARK-23938][SQL] Add map_zip_with function: #22017 - Revert the changes of map_entries in [SPARK-24331][SPARKR][SQL] Adding arrays_overlap, array_repeat, map_entries to SparkR: #21434 ## How was this patch tested? The existing tests. Closes #22827 from gatorsmile/revertMap2.4. Authored-by: gatorsmile <gatorsmile@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

[SPARK-23938][SQL] Add map_zip_with function

ef56011

mn-mikke added 2 commits August 7, 2018 11:14

[SPARK-23938][SQL] Merging master into the feature branch and resolvi…

34cdf0d

…ng confilicts.

[SPARK-23938][SQL] Merging master into the feature branch and resolvi…

ec583eb

…ng confilicts.

ueshin reviewed Aug 7, 2018

View reviewed changes

mgaido91 reviewed Aug 7, 2018

View reviewed changes

xuanyuanking reviewed Aug 7, 2018

View reviewed changes

mn-mikke added 4 commits August 7, 2018 23:11

[SPARK-23938][SQL] Addressing review comments

be2e10d

Merge remote-tracking branch 'spark/master' into SPARK-23938

a9020a4

# Conflicts: # sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala

[SPARK-23938][SQL] Addressing review comments

89a3da4

[SPARK-23938][SQL] Adding new lines back

12ad8b2

mgaido91 reviewed Aug 8, 2018

View reviewed changes

mn-mikke added 2 commits August 8, 2018 23:30

[SPARK-23938][SQL] Map comparison in tests ignoring ordering

6aeaaa8

[SPARK-23938][SQL] Addressing review comments

38ce4e7

ueshin reviewed Aug 9, 2018

View reviewed changes

mn-mikke added 2 commits August 9, 2018 16:31

Revert "[SPARK-23938][SQL] Map comparison in tests ignoring ordering"

562ee81

This reverts commit 6aeaaa8

[SPARK-23938][SQL] Modification to a solution with LinkedHashMap and …

5d2a78e

…indexes represented as arrays.

[SPARK-23938][SQL] Coercion rule for the key type

3c849cb

mgaido91 reviewed Aug 10, 2018

View reviewed changes

[SPARK-23938][SQL] Bug fix + type coercion tests

595161f

ueshin reviewed Aug 10, 2018

View reviewed changes

Merge remote-tracking branch 'spark/master' into SPARK-23938

6995f1e

[SPARK-23938][SQL] Key type resolved by findCommonTypeDifferentOnlyIn…

2b7e9e5

…NullFlags + coercion rule refactoring

ueshin reviewed Aug 14, 2018

View reviewed changes

[SPARK-23938][SQL] Addressing review comments

bcd4e0f

mgaido91 reviewed Aug 14, 2018

View reviewed changes

asfgit closed this in 42263fd Aug 14, 2018

ueshin mentioned this pull request Aug 16, 2018

[SPARK-23938][SQL][FOLLOW-UP][TEST] Nullabilities of value arguments should be true. #22126

Closed

gatorsmile mentioned this pull request Oct 25, 2018

[SPARK-25832][SQL][BRANCH-2.4] Revert newly added map related functions #22827

Closed


		override def functions: Seq[Expression] = function :: Nil

		override def nullable: Boolean = left.nullable \|\| right.nullable


		@transient lazy val functionForEval: Expression = functionsForEval.head

		@transient lazy val (keyType, leftValueType, _) =

[SPARK-23938][SQL] Add map_zip_with function #22017

[SPARK-23938][SQL] Add map_zip_with function #22017

Conversation

mn-mikke commented Aug 6, 2018

What changes were proposed in this pull request?

How was this patch tested?

mn-mikke commented Aug 6, 2018

SparkQA commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mn-mikke Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

SparkQA commented Aug 8, 2018

SparkQA commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgaido91 Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Aug 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Aug 9, 2018

SparkQA commented Aug 10, 2018

ueshin commented Aug 10, 2018

mgaido91 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ueshin Aug 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Aug 10, 2018

mn-mikke commented Aug 10, 2018

SparkQA commented Aug 10, 2018

HyukjinKwon commented Aug 13, 2018

SparkQA commented Aug 13, 2018

SparkQA commented Aug 14, 2018

ueshin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgaido91 left a comment

Choose a reason for hiding this comment

mn-mikke Aug 7, 2018 •

edited

Loading

mgaido91 Aug 8, 2018 •

edited

Loading

ueshin Aug 11, 2018 •

edited

Loading