[SPARK-8234][SQL] misc function: md5 #6779

qiansl127 · 2015-06-12T06:33:05Z

No description provided.

chenghao-intel · 2015-06-12T06:36:04Z

@rxin @liancheng can you trigger the unit test?

rxin · 2015-06-12T06:38:05Z

Jenkins, ok to test.

rxin · 2015-06-12T06:40:30Z

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

+  override def dataType: DataType = StringType
+
+  override def expectedChildTypes: Seq[DataType] =
+    if (child.dataType == BinaryType) Seq(BinaryType) else Seq(StringType)


this doesn't make sense -- since it is using child's data type to check data type... you should remove ExpectsInputTypes and explicitly define the type check

SparkQA · 2015-06-12T06:47:50Z

Test build #34756 has finished for PR 6779 at commit b7efa2a.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Md5(child: Expression)
- case class SetInFilter[T <: Comparable[T]](

marmbrus · 2015-06-12T08:25:17Z

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

+    val s1 = 'a.string.at(0)
+    val s2 = 'a.binary.at(0)
+    checkEvaluation(Md5(s1), "902fbdd2b1df0c4f70b4a5d23525e932", create_row("ABC"))
+    checkEvaluation(Md5(s2), "6ac1e56bc78f031059be7be854522c4c",


I think this test would be simpler/clearer if you just pass literals to the expression. There are other tests that make sure that we can extract values from rows.

Additionally It would be good to test some edge cases: an empty string, null, etc

+1 for edge cases.

@qiansl127 you can write code like

checkEvaluation(Md5(Literal("ABC"), "902fbdd2b1df0c4f70b4a5d23525e932") checkEvaluation(Md5(Literal.create(null, BinaryType), "...") checkEvaluation(Md5(Literal.create(null, StringType), "...")

SparkQA · 2015-06-12T09:24:58Z

Test build #34765 has finished for PR 6779 at commit 4d66fd1.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Md5(child: Expression) extends UnaryExpression
- case class SetInFilter[T <: Comparable[T]](

chenghao-intel · 2015-06-12T13:41:26Z

sql/core/src/main/scala/org/apache/spark/sql/functions.scala

+  //////////////////////////////////////////////////////////////////////////////////////////////
+
+  /**
+   * Calculates the MD5 digest and returns the value as a 32 character hex string.


Sorry, I set a bad example in strlen, we'd better insert an empty line after the doc and right before the @ group

SparkQA · 2015-06-15T04:11:57Z

Test build #34907 has finished for PR 6779 at commit d77a806.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2015-06-15T06:45:56Z

Test build #34908 has finished for PR 6779 at commit efa8b93.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Md5(child: Expression) extends UnaryExpression

qiansl127 · 2015-06-15T06:47:58Z

@rxin @chenghao-intel

SparkQA · 2015-06-15T09:26:16Z

Test build #34922 has finished for PR 6779 at commit d9a28e4.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Md5(child: Expression)

SparkQA · 2015-06-16T03:39:34Z

Test build #34964 has finished for PR 6779 at commit c75bb09.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Md5(child: Expression)

qiansl127 · 2015-06-16T07:07:14Z

@rxin @chenghao-intel Any thing need to be improved?

adrian-wang · 2015-06-16T07:16:55Z

add a test using sql(), and maybe change the filename of misc.scala to miscFunctions.scala

LGTM otherwise.

chenghao-intel · 2015-06-16T08:32:51Z

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

+
+  override def expectedChildTypes: Seq[DataType] = Seq(BinaryType)
+
+  override def checkInputDataTypes(): TypeCheckResult =


This override is not necessary now.

I mean we can remove this function overriding, as we implemented the expectedChildTypes already.

yes, please see doc for ExpectsInputTypes. We don't need to do type check for it.

SparkQA · 2015-06-17T04:03:53Z

Test build #35013 has finished for PR 6779 at commit da60eb3.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Md5(child: Expression)

chenghao-intel · 2015-06-17T13:27:07Z

LGTM, cc @rxin

rxin · 2015-06-18T06:50:42Z

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

+    }
+  }
+
+  override def toString: String = s"MD5($child)"


i don't think you need the toString here ... since case class already handles it.

rxin · 2015-06-18T06:52:02Z

Looks good - but please add a codegen version of this.

cloud-fan · 2015-06-19T03:43:31Z

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

+  override def genCode(ctx: CodeGenContext, ev: GeneratedExpressionCode): String = {
+    defineCodeGen(ctx, ev, c =>
+      s"""org.apache.spark.unsafe.types.UTF8String.fromString
+         |(org.apache.commons.codec.digest.DigestUtils.md5Hex($c))""".stripMargin)


Do we need a new line here? maybe just

"abcd" + "efgh"

It's probably not a big problem, as we don't want to make the generated code more than 100 characters per line, right? Or @qiansl127 can update that in the follow up PRs.

we have called stripMargin, so it will still result in one line.
Anyway I think the current code is acceptable.

the generated code can be as long as we want.

in this case indenting this way is really weird. we should indent it this way

"org.apache.spark.unsafe.types.UTF8String.fromString" + s"(org.apache.commons.codec.digest.DigestUtils.md5Hex($c))"

SparkQA · 2015-06-19T03:46:55Z

Test build #35210 has finished for PR 6779 at commit 04bd27b.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Md5(child: Expression)

chenghao-intel · 2015-06-19T05:13:26Z

@rxin I think this is ready to be merged.

rxin · 2015-06-19T05:26:07Z

Can you fix the indent? LGTM other than that.

SparkQA · 2015-06-19T07:38:09Z

Test build #35228 has finished for PR 6779 at commit 11fcdb2.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Md5(child: Expression)

qiansl127 · 2015-06-19T07:51:42Z

@rxin Done!

rxin · 2015-06-19T17:49:17Z

Thanks. Merging.

rxin reviewed Jun 12, 2015
View reviewed changes

marmbrus reviewed Jun 12, 2015
View reviewed changes

chenghao-intel reviewed Jun 12, 2015
View reviewed changes

qiansl127 force-pushed the MD5 branch from 4d66fd1 to d77a806 Compare June 15, 2015 04:02

qiansl127 force-pushed the MD5 branch from efa8b93 to d9a28e4 Compare June 15, 2015 08:06

chenghao-intel reviewed Jun 16, 2015
View reviewed changes

qiansl127 added 7 commits June 17, 2015 10:14

Add md5 function

c166167

Rewrite the type check for Md5

b8c73b4

Add more test case

60ccde1

format to scala type

1df0b5b

Accept only BinaryType for Md5

12c61f4

Format code

9509ad0

Remove checkInputDataTypes function

da60eb3

qiansl127 force-pushed the MD5 branch from c75bb09 to da60eb3 Compare June 17, 2015 02:17

rxin reviewed Jun 18, 2015
View reviewed changes

Add codegen

04bd27b

cloud-fan reviewed Jun 19, 2015
View reviewed changes

Fix the indent

11fcdb2

asfgit closed this in 0c32fc1 Jun 19, 2015

qiansl127 deleted the MD5 branch June 30, 2015 02:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-8234][SQL] misc function: md5 #6779

[SPARK-8234][SQL] misc function: md5 #6779

qiansl127 commented Jun 12, 2015

chenghao-intel commented Jun 12, 2015

rxin commented Jun 12, 2015

rxin Jun 12, 2015

SparkQA commented Jun 12, 2015

marmbrus Jun 12, 2015

chenghao-intel Jun 12, 2015

SparkQA commented Jun 12, 2015

chenghao-intel Jun 12, 2015

SparkQA commented Jun 15, 2015

SparkQA commented Jun 15, 2015

qiansl127 commented Jun 15, 2015

SparkQA commented Jun 15, 2015

SparkQA commented Jun 16, 2015

qiansl127 commented Jun 16, 2015

adrian-wang commented Jun 16, 2015

chenghao-intel Jun 16, 2015

chenghao-intel Jun 16, 2015

cloud-fan Jun 16, 2015

SparkQA commented Jun 17, 2015

chenghao-intel commented Jun 17, 2015

rxin Jun 18, 2015

rxin commented Jun 18, 2015

cloud-fan Jun 19, 2015

chenghao-intel Jun 19, 2015

adrian-wang Jun 19, 2015

rxin Jun 19, 2015

SparkQA commented Jun 19, 2015

chenghao-intel commented Jun 19, 2015

rxin commented Jun 19, 2015

SparkQA commented Jun 19, 2015

qiansl127 commented Jun 19, 2015

rxin commented Jun 19, 2015


		override def expectedChildTypes: Seq[DataType] = Seq(BinaryType)

		override def checkInputDataTypes(): TypeCheckResult =

[SPARK-8234][SQL] misc function: md5 #6779

[SPARK-8234][SQL] misc function: md5 #6779

Conversation

qiansl127 commented Jun 12, 2015

chenghao-intel commented Jun 12, 2015

rxin commented Jun 12, 2015

Choose a reason for hiding this comment

SparkQA commented Jun 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jun 12, 2015

Choose a reason for hiding this comment

SparkQA commented Jun 15, 2015

SparkQA commented Jun 15, 2015

qiansl127 commented Jun 15, 2015

SparkQA commented Jun 15, 2015

SparkQA commented Jun 16, 2015

qiansl127 commented Jun 16, 2015

adrian-wang commented Jun 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jun 17, 2015

chenghao-intel commented Jun 17, 2015

Choose a reason for hiding this comment

rxin commented Jun 18, 2015

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 Jun 19, 2015

chenghao-intel commented Jun 19, 2015

rxin commented Jun 19, 2015

SparkQA commented Jun 19, 2015

qiansl127 commented Jun 19, 2015

rxin commented Jun 19, 2015