Skip to content

Conversation

@viirya
Copy link
Member

@viirya viirya commented Jun 22, 2015

@SparkQA
Copy link

SparkQA commented Jun 22, 2015

Test build #35447 has finished for PR 6934 at commit 59e41aa.

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

@rxin
Copy link
Contributor

rxin commented Jun 23, 2015

cc @cloud-fan can you help review this? Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove self: Product =>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bit -> numBits

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check numBits is valid or not to fail fast?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently if given bit length is not one of the permitted values, the result value is NULL. So I think it should not be to fail fast?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The handling of invalid bit length is useful for SQL (we can not easily valid the parameter in SQL parser), but for DataFrame API, it's good to fail fast, give better feedback to users.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to fail fast

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I will add it later.

@SparkQA
Copy link

SparkQA commented Jun 24, 2015

Test build #35694 has finished for PR 6934 at commit 8573aff.

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

@davies
Copy link
Contributor

davies commented Jun 24, 2015

LGTM, left two minor comments (good to have).

@SparkQA
Copy link

SparkQA commented Jun 25, 2015

Test build #35802 has finished for PR 6934 at commit 35e0bb3.

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

@asfgit asfgit closed this in 47c874b Jun 26, 2015
asfgit pushed a commit that referenced this pull request Jun 29, 2015
Jira: https://issues.apache.org/jira/browse/SPARK-8235

I added the support for sha1. If I understood rxin correctly, sha and sha1 should execute the same algorithm, shouldn't they?

Please take a close look on the Python part. This is adopted from #6934

Author: Tarek Auel <[email protected]>
Author: Tarek Auel <[email protected]>

Closes #6963 from tarekauel/SPARK-8235 and squashes the following commits:

f064563 [Tarek Auel] change to shaHex
7ce3cdc [Tarek Auel] rely on automatic cast
a1251d6 [Tarek Auel] Merge remote-tracking branch 'upstream/master' into SPARK-8235
68eb043 [Tarek Auel] added docstring
be5aff1 [Tarek Auel] improved error message
7336c96 [Tarek Auel] added type check
cf23a80 [Tarek Auel] simplified example
ebf75ef [Tarek Auel] [SPARK-8301] updated the python documentation. Removed sha in python and scala
6d6ff0d [Tarek Auel] [SPARK-8233] added docstring
ea191a9 [Tarek Auel] [SPARK-8233] fixed signatureof python function. Added expected type to misc
e3fd7c3 [Tarek Auel] SPARK[8235] added sha to the list of __all__
e5dad4e [Tarek Auel] SPARK[8235] sha / sha1
@viirya viirya deleted the expr_sha2 branch December 27, 2023 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants