[SPARK-12567][SQL] Add aes_{encrypt,decrypt} UDFs #10527

vectorijk · 2015-12-30T14:31:50Z

No description provided.

SparkQA · 2015-12-30T18:52:21Z

Test build #48484 has finished for PR 10527 at commit 9edeba7.

This patch fails from timeout after a configured wait of 250m.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class AesDecrypt(left: Expression, right: Expression)

vectorijk · 2015-12-30T22:16:12Z

Jenkins, retest this please.

SparkQA · 2015-12-31T00:08:35Z

Test build #48511 has finished for PR 10527 at commit 9edeba7.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class AesDecrypt(left: Expression, right: Expression)

SparkQA · 2016-01-05T10:12:06Z

Test build #48748 has finished for PR 10527 at commit 0558bf8.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class AesEncrypt(left: Expression, right: Expression)
- case class AesDecrypt(left: Expression, right: Expression)

SparkQA · 2016-01-05T11:30:00Z

Test build #48744 has finished for PR 10527 at commit 27693b4.

This patch fails from timeout after a configured wait of 250m.
This patch does not merge cleanly.
This patch adds the following public classes (experimental):
- case class AesDecrypt(left: Expression, right: Expression)

vectorijk · 2016-01-05T23:56:44Z

cc @cloud-fan @marmbrus @davies

cloud-fan · 2016-01-06T01:27:01Z

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

no need to with Serializable

SparkQA · 2016-01-07T04:02:59Z

Test build #48890 has finished for PR 10527 at commit ed38390.

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

cloud-fan · 2016-01-07T05:33:46Z

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

So when exception happens, codegen version will throw it, and the interpreted version will return null?

SparkQA · 2016-01-07T16:00:05Z

Test build #48943 has finished for PR 10527 at commit 9476822.

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

cloud-fan · 2016-01-11T22:56:23Z

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

why does only this test need extra installation?

Because Oracle JRE/JDK supports AES-128 out of the box. AES-192 and AES-256 are supported if Java Cryptography Extension (JCE) Unlimited Strength Jurisdiction Policy Files are installed. In this test case, key size is 256 bits. We should install JCE not only with this test also any test with 192/256 bits key. I commented out this test by default in case this test fails.

ping @cloud-fan?

I'm not sure if we can introduce a feature that may require extra installation... cc @rxin @marmbrus

I think it might be okay, as long as the failure is clear and it doesn't break other things when its not installed. What happens when its not installed?

@marmbrus When Java Cryptography Extension (JCE) not installed under oracle jdk, it will return null and will not break other things. Plus, we don't need to worry about installing this extension under openjdk, it works fine with openjdk.

@cloud-fan @marmbrus ping?

cloud-fan · 2016-01-27T19:24:36Z

retest this please.

SparkQA · 2016-01-27T19:33:46Z

Test build #50212 has finished for PR 10527 at commit 9476822.

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

vectorijk · 2016-01-28T02:29:09Z

retest this please

SparkQA · 2016-01-28T05:25:22Z

Test build #50243 has finished for PR 10527 at commit 9476822.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds no public classes.

cloud-fan · 2016-01-28T19:40:44Z

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

we should follow the style of other classes in this file, i.e. use double quotation marks and \n

vectorijk · 2016-01-31T04:11:29Z

@cloud-fan Okay, addressed comments.

cloud-fan · 2016-01-31T04:22:55Z

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

I'm not quite sure how we should deal with errors, return null or throw exception? cc @marmbrus

I checked some similar expressions, Encode/Decode will throw exception if its charset string parameter is invalid, Sha2 will return null if its bit length parameter is invalid.

For AesEncrypt, what we should do if its secret key parameter is invalid? @vectorijk are there any other errors will be thrown during execution?

cc @rxin , what's the general rule for this kind of stuff?

https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala#L336

we wrap everything in an analysis exception during expression construction.

it's not during construction, but execution. The parameters may be non-foldable and we can only know it's valid or not during execution. BTW the encrypting process may fail and throw exception, should we silently ignore it and return null, or rethrow the exception and fail the execution?

I'd say in this case we should rethrow the exception to fail everything.

@cloud-fan What I have implemented was trying to follow the way Sha2 did.
I think if secret key is invalid. It will return null just like the way Sha2 did. There is no other errors should be thrown during execution.

yea, you followed Sha2, but Sha2 may also be wrong. In this case we think rethrow exception makes more sense, and we can also fix Sha2 later.

@cloud-fan okay, i will try to rethrow exception as Encode/Decode does.

SparkQA · 2016-01-31T05:44:19Z

Test build #50456 has finished for PR 10527 at commit 3a2510b.

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

SparkQA · 2016-01-31T06:53:34Z

Test build #50457 has finished for PR 10527 at commit 04a14cf.

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

SparkQA · 2016-02-13T09:40:40Z

Test build #51235 has finished for PR 10527 at commit 6bc1b63.

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

cloud-fan · 2016-02-14T06:21:28Z

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

+
+    // key length (80 bits) is not one of the permitted values (128, 192 or 256 bits)
+    intercept[java.security.InvalidKeyException] {
+      evaluate(AesDecrypt(UnBase64(Literal("y6Ss+zCYObpCbgfWfyNWTw==")),


This only tests interpreted version, we also need to test codegen version, i.e. create an unsafe projection using AesDecrypt and execute it.

@cloud-fan I added codegen version tests with creating unsafe projection. But I'm not very sure whether I am doing right or not. Could you take a look again?
Meanwhile, I took look at code ExpressionEvalHelper.scala checkEvalutionWithUnsafeProjection(). Is that doing the same thing as you said to test codegen version?

SparkQA · 2016-02-18T10:30:19Z

Test build #51476 has finished for PR 10527 at commit dc76057.

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

to avoid python test errors

SparkQA · 2016-02-19T03:17:17Z

Test build #51508 has finished for PR 10527 at commit 8e2960c.

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

cloud-fan · 2016-02-19T08:51:25Z

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

+    assert(instance1.apply(null).getString(0) === "ABC")
+
+    val instance2 = UnsafeProjection.create(expr2 :: Nil)
+    assert(instance2.apply(null).getString(0) === "")


The above 2 are not needed. Normal cases are already covered by checkEvaluation, we only need to test error case

Okay, Done that.

cloud-fan · 2016-02-19T10:04:24Z

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

+      evaluate(expr3)
+    }
+    intercept[java.security.InvalidKeyException] {
+      UnsafeProjection.create(expr3::Nil).apply(null)


nit: expr3 :: Nil, we should add space

cloud-fan · 2016-02-19T10:04:48Z

LGTM overall

SparkQA · 2016-02-19T11:31:52Z

Test build #51549 has finished for PR 10527 at commit 60b9da7.

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

SparkQA · 2016-02-19T11:49:21Z

Test build #51551 has finished for PR 10527 at commit 0856fb0.

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

davies · 2016-02-20T06:26:13Z

LGTM, merging this into master, thanks!

rxin · 2016-02-20T06:29:04Z

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

+   * @group misc_funcs
+   * @since 2.0.0
+   */
+  def aes_encrypt(input: Column, key: Column): Column = withExpr {


key should just be an int

Key is not integer, it's binary, see examples.

I also think so. The examples is just one of those cases. Key also could be abcdef1234567890.

then we should probably just take a string.

the thing is we want to facilitate the most common cases, e.g. if 99% of the time people are passing in keys directly rather than relying on some other columns' values, we should just let them pass the literals.

Some of the functions take literals, some not., In 2.0, should we clean up all the mess?

What are the other examples?

Actually we do have a mess of it. Generally we have 3 kinds of parameters: Column, column name string, literal. For example, pow takes 2 parameters, and we have 8 overloaded versions of it, which are all combinations of these 3 kinds of parameters except both literals. However, some functions like pmod, only have both Column version. Some functions like sha2 only have Column, literal combination.

Personly I think for something like sha2 and this one, where one parameter will be literal 99% of the time, we should just give a Column, literal combination. i.e. def aes_encrypt(input: Column, key: String). For something like pow, we only need to provide the both Column version, as column name string and literal are easy to parse to Column, by col() and lit().

rxin · 2016-02-20T06:40:40Z

@davies does this implementation even make sense? decrypt always return a string, while encrypt always take in a binary? The two are not symmetric.

rxin · 2016-02-20T06:43:29Z

Sorry I'm not convinced this is correct (decrypt returning string type), and there are other issues with it. I'm just going to revert the patch, since it is unlikely other things will conflict with this.

rxin · 2016-02-20T06:44:51Z

@vectorijk I've reverted the patch. Can you reopen the pull request and fix the return types?

vectorijk · 2016-02-20T06:46:32Z

Ok, Sure. I will do that.

davies · 2016-02-20T06:46:59Z

I did not look into this closely, since @cloud-fan already reviewed this many rounds. sorry for the rush.

rxin · 2016-02-20T06:50:03Z

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

+@ExpressionDescription(
+  usage = "_FUNC_(input, key) - Decrypts input using AES.",
+  extended = "> SELECT _FUNC_(UnBase64('y6Ss+zCYObpCbgfWfyNWTw=='),'1234567890123456');\n 'ABC'")
+case class AesDecrypt(left: Expression, right: Expression)


i find it confusing to have left/right here (e.g. input, key)

Let's give them a proper name, and then just override def left: Expression = input.

also if the key is literal, i'd just do some input data type checking in analysis (override checkInputTypes) to make sure the key is in acceptable range.

rxin · 2016-02-20T06:54:22Z

I would also just remove support for 192/256, so we don't have to explain the JCE stuff.

cloud-fan · 2016-02-21T04:26:15Z

Sorry missed the symmetry part, but it is symmetric underneath, except AesDecrypt wrap the bytes into a string at last. We should return binary directly, sorry for the rush.

vectorijk · 2016-02-21T04:35:53Z

Thanks so much for suggestion! I will open a new PR for update.

maver1ck · 2018-02-12T09:39:45Z

@vectorijk Is this PR dead ?

vectorijk force-pushed the spark-12567 branch 2 times, most recently from 27693b4 to 0558bf8 Compare January 5, 2016 08:07

cloud-fan reviewed Jan 6, 2016
View reviewed changes

cloud-fan reviewed Jan 7, 2016
View reviewed changes

cloud-fan reviewed Jan 11, 2016
View reviewed changes

vectorijk force-pushed the spark-12567 branch from 9476822 to bc7aa2c Compare January 28, 2016 02:46

cloud-fan reviewed Jan 28, 2016
View reviewed changes

cloud-fan reviewed Jan 31, 2016
View reviewed changes

vectorijk added 2 commits February 9, 2016 01:46

[SPARK-12567] add aes {encrypt, decrypt}

2fb9b30

codegen version has been consistent with non-codegen version

c3e8331

cloud-fan reviewed Feb 14, 2016
View reviewed changes

Python API for aes_encrypt and aes_decrypt

dc76057

vectorijk added 3 commits February 18, 2016 03:38

add function decorator ignore_unicode_prefix

170b863

to avoid python test errors

add test cases under DataFrame API

5571087

test codegen version

8e2960c

cloud-fan reviewed Feb 19, 2016
View reviewed changes

fix

60b9da7

cloud-fan reviewed Feb 19, 2016
View reviewed changes

nit

0856fb0

rxin reviewed Feb 20, 2016
View reviewed changes

asfgit closed this in 4f9a664 Feb 20, 2016

rxin reviewed Feb 20, 2016
View reviewed changes

sarutak mentioned this pull request Oct 7, 2021

[SPARK-12567][SQL] Add aes_encrypt and aes_decrypt builtin functions #32801

Closed

[SPARK-12567][SQL] Add aes_{encrypt,decrypt} UDFs #10527

[SPARK-12567][SQL] Add aes_{encrypt,decrypt} UDFs #10527

Uh oh!

Conversation

vectorijk commented Dec 30, 2015

Uh oh!

SparkQA commented Dec 30, 2015

Uh oh!

vectorijk commented Dec 30, 2015

Uh oh!

SparkQA commented Dec 31, 2015

Uh oh!

SparkQA commented Jan 5, 2016

Uh oh!

SparkQA commented Jan 5, 2016

Uh oh!

vectorijk commented Jan 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 7, 2016

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!

cloud-fan commented Jan 27, 2016

Uh oh!

SparkQA commented Jan 27, 2016

Uh oh!

vectorijk commented Jan 28, 2016

Uh oh!

SparkQA commented Jan 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vectorijk commented Jan 31, 2016

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!

SparkQA commented Jan 31, 2016

Uh oh!

SparkQA commented Jan 31, 2016

Uh oh!

SparkQA commented Feb 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!