[SPARK-8784] [SQL] Add Python API for hex and unhex #7181

davies · 2015-07-02T07:04:18Z

Also improve the performance of hex/unhex

davies · 2015-07-02T07:04:31Z

rxin · 2015-07-02T07:07:50Z

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

+ * Resulting characters are returned as a byte array.
+ */
+case class Unhex(child: Expression)
+  extends UnaryExpression with AutoCastInputTypes with Serializable {


can you use ExpectsInputTypes here? since I will remove AutoCastInputTypes.

AmplabJenkins · 2015-07-02T07:08:11Z

Merged build triggered.

AmplabJenkins · 2015-07-02T07:08:18Z

Merged build started.

rxin · 2015-07-02T07:08:22Z

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

-    checkEvaluation(UnHex(Literal("737472696E67")), "string".getBytes)
-    checkEvaluation(UnHex(Literal("")), new Array[Byte](0))
-    checkEvaluation(UnHex(Literal("0")), Array[Byte](0))
+    checkEvaluation(Unhex(Literal("737472696E67")), "string".getBytes)


add a test for null literal of string type

SparkQA · 2015-07-02T07:08:57Z

Test build #36358 has started for PR 7181 at commit 1a24082.

rxin · 2015-07-02T07:09:27Z

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

+  }
+
+  // lookup table to translate '0' -> 0 ... 'F'/'f' -> 15
+  private[this] val unhexDigits = {


can we move the two tables into some static field in an object?

We can, but putting them here is more clear. Only one object per Expression, I think it's fine.

why would it be more clear? are you thinking about the distance between its definition and where it is used?

this is one case where java beats scala with static fields. Ideally all the string functions should just be member functions in UTF8String.

Less code is better than more code. From performance's point of view, there is never an end to stop optimize it. I think we could go with something that's good enough (won't be the bottle neck).

how is this less code? It is just a bad idea to create unnecessary state. Just move both tables into two fields in Hex object.

BTW I absolutely disagree that "less code is better than more code" as an ethos. While it can be true in many cases, there are plenty of counter examples:

putting everything into a single file without any namespacing reduces the import statements, but that creates bad logical structures

not writing any test cases reduces the amount of code also, but that's obviously bad

using arcane scala features can substantially reduce code size in certain cases, at the expense of readability

In this case, I don't see how this creates less code (it takes 2 lines of code to define a scala object -- you can even put it in an existing java class like in UTF8String as a static field).

AmplabJenkins · 2015-07-02T07:23:10Z

Merged build triggered.

AmplabJenkins · 2015-07-02T07:23:20Z

Merged build started.

SparkQA · 2015-07-02T07:27:11Z

Test build #36362 has started for PR 7181 at commit c3af78c.

SparkQA · 2015-07-02T07:27:35Z

Test build #36358 has finished for PR 7181 at commit 1a24082.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- trait ExpectsInputTypes
- trait AutoCastInputTypes
- abstract class BinaryExpression extends Expression with trees.BinaryNode[Expression]
- abstract class BinaryOperator extends BinaryExpression
- abstract class BinaryArithmetic extends BinaryOperator
- case class Unhex(child: Expression)
- abstract class BinaryComparison extends BinaryOperator with Predicate

AmplabJenkins · 2015-07-02T07:27:43Z

Merged build finished. Test FAILed.

davies · 2015-07-02T07:34:12Z

The test failed because of ExpectsInputTypes, blocked by #7175

zhichao-li · 2015-07-02T07:56:37Z

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

+    }
+    // two characters form the hex value.
+    while (i < bytes.length) {
+      if (bytes(i) < 0 || bytes(i + 1) < 0) {


There would be exception on my previous logic when facing non-ascii character. Thanks for fixing this.

SparkQA · 2015-07-02T09:10:33Z

Test build #36362 has finished for PR 7181 at commit c3af78c.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Unhex(child: Expression)

AmplabJenkins · 2015-07-02T09:10:42Z

Merged build finished. Test FAILed.

AmplabJenkins · 2015-07-02T17:33:11Z

Build triggered.

AmplabJenkins · 2015-07-02T17:33:18Z

Build started.

SparkQA · 2015-07-02T17:34:30Z

Test build #36416 has started for PR 7181 at commit 25156b7.

SparkQA · 2015-07-02T17:39:33Z

Test build #36416 has finished for PR 7181 at commit 25156b7.

This patch fails to build.
This patch does not merge cleanly.
This patch adds the following public classes (experimental):
- case class Unhex(child: Expression)

AmplabJenkins · 2015-07-02T17:39:34Z

Build finished. Test FAILed.

AmplabJenkins · 2015-07-02T18:33:11Z

Build triggered.

AmplabJenkins · 2015-07-02T18:33:19Z

Build started.

SparkQA · 2015-07-02T18:36:29Z

Test build #36423 has started for PR 7181 at commit b31fc9a.

Conflicts: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/math.scala sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/MathFunctionsSuite.scala

AmplabJenkins · 2015-07-02T20:38:10Z

Merged build triggered.

AmplabJenkins · 2015-07-02T20:38:20Z

Merged build started.

SparkQA · 2015-07-02T20:41:18Z

Test build #36429 has started for PR 7181 at commit f032fbb.

SparkQA · 2015-07-02T20:49:28Z

Test build #36423 has finished for PR 7181 at commit b31fc9a.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds the following public classes (experimental):
- case class Unhex(child: Expression)

AmplabJenkins · 2015-07-02T20:50:03Z

Build finished. Test PASSed.

SparkQA · 2015-07-02T22:19:54Z

Test build #36429 has finished for PR 7181 at commit f032fbb.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class CreateNamedStruct(children: Seq[Expression]) extends Expression
- case class Unhex(child: Expression)
- case class ShiftLeft(left: Expression, right: Expression) extends BinaryExpression
- case class ShiftRight(left: Expression, right: Expression) extends BinaryExpression

AmplabJenkins · 2015-07-02T22:20:35Z

Merged build finished. Test PASSed.

rxin · 2015-07-02T22:43:01Z

Thanks - merging this in master.

rxin · 2015-07-02T23:25:41Z

I reverted the patch since it broke the build. Can you submit a new PR?

JoshRosen · 2015-07-02T23:34:29Z

Was the build break caused by racing merges?

rxin · 2015-07-02T23:40:12Z

Not 100% sure.

Add Python API for hex and unhex

1a24082

rxin reviewed Jul 2, 2015
View reviewed changes

address commments

c3af78c

zhichao-li reviewed Jul 2, 2015
View reviewed changes

address comments and fix test

25156b7

Update math.scala

b31fc9a

Davies Liu added 2 commits July 2, 2015 13:12

Merge branch 'master' of github.com:apache/spark into hex

49e325f

Conflicts: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/math.scala sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/MathFunctionsSuite.scala

Merge branch 'hex' of github.com:davies/spark into hex

f032fbb

asfgit closed this in fc7aebd Jul 2, 2015

[SPARK-8784] [SQL] Add Python API for hex and unhex #7181

[SPARK-8784] [SQL] Add Python API for hex and unhex #7181

Conversation

davies commented Jul 2, 2015

davies commented Jul 2, 2015

Choose a reason for hiding this comment

AmplabJenkins commented Jul 2, 2015

AmplabJenkins commented Jul 2, 2015

Choose a reason for hiding this comment

SparkQA commented Jul 2, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented Jul 2, 2015

AmplabJenkins commented Jul 2, 2015

SparkQA commented Jul 2, 2015

SparkQA commented Jul 2, 2015

AmplabJenkins commented Jul 2, 2015

davies commented Jul 2, 2015

Choose a reason for hiding this comment

SparkQA commented Jul 2, 2015

AmplabJenkins commented Jul 2, 2015

AmplabJenkins commented Jul 2, 2015

AmplabJenkins commented Jul 2, 2015

SparkQA commented Jul 2, 2015

SparkQA commented Jul 2, 2015

AmplabJenkins commented Jul 2, 2015

AmplabJenkins commented Jul 2, 2015

AmplabJenkins commented Jul 2, 2015

SparkQA commented Jul 2, 2015

AmplabJenkins commented Jul 2, 2015

AmplabJenkins commented Jul 2, 2015

SparkQA commented Jul 2, 2015

SparkQA commented Jul 2, 2015

AmplabJenkins commented Jul 2, 2015

SparkQA commented Jul 2, 2015

AmplabJenkins commented Jul 2, 2015

rxin commented Jul 2, 2015

rxin commented Jul 2, 2015

JoshRosen commented Jul 2, 2015

rxin commented Jul 2, 2015