[SPARK-42191][SQL] Support udf 'luhn_check' #39747
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vinodkc
changed the title
|
Hi @dtenedor, @srielau, @HyukjinKwon @cloud-fan , @gengliangwang |
dtenedor
reviewed
Jan 25, 2023
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala
Outdated
Show resolved
Hide resolved
sql/core/src/test/resources/sql-tests/inputs/string-functions.sql
Outdated
Show resolved
Hide resolved
|
The general algorithm and test coverage look correct |
vinodkc
force-pushed
the
br_udf_luhn_check
branch
2 times, most recently
from
80e6dbc
to
bdba53c
Compare
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala
Outdated
Show resolved
Hide resolved
vinodkc
force-pushed
the
br_udf_luhn_check
branch
from
bdba53c
to
57c9c74
Compare
dtenedor
reviewed
Jan 27, 2023
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala
Outdated
Show resolved
Hide resolved
sql/core/src/test/resources/sql-tests/inputs/string-functions.sql
Outdated
Show resolved
Hide resolved
vinodkc
force-pushed
the
br_udf_luhn_check
branch
3 times, most recently
from
906622a
to
ccf8243
Compare
cloud-fan
reviewed
Jan 29, 2023
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Jan 29, 2023
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala
Outdated
Show resolved
Hide resolved
vinodkc
force-pushed
the
br_udf_luhn_check
branch
4 times, most recently
from
ff77d09
to
23095da
Compare
cloud-fan
reviewed
Jan 30, 2023
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtils.java
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Jan 30, 2023
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala
Show resolved
Hide resolved
cloud-fan
reviewed
Jan 30, 2023
|
I'm not super against this, but this seems too niche to put into the Spark API |
|
Hi @srowen I get where you're coming from. For background, we have a Jira [1] to add a suite of data masking functions into Spark. This is one of a family of such proposed functions. The idea is to support ETL with redaction of original datasets to make it safer for sharing. |
|
Right, I don't think most of those belong in the Spark API. This is what UDF and utility libs are for |
|
@srowen yeah, we should apply discretion for those functions on that list. There's something to be said for having a built-in library of functions that are useful enough for general-purpose consumption, but not everything belongs in there. Maybe for this particular case we can let it through as it is in fact present in Postgres and Trino (as linked in the PR description) but we could do a review of the others and make go/no-go decisions on each one together. WDYT? |
vinodkc
force-pushed
the
br_udf_luhn_check
branch
from
23095da
to
ca8f807
Compare
dtenedor
approved these changes
Jan 31, 2023
There was a problem hiding this comment.
LGTM, please address comments and add requested tests from @cloud-fan before merging ✅
cloud-fan
reviewed
Jan 31, 2023
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtils.java
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Jan 31, 2023
sql/core/src/test/resources/sql-tests/inputs/string-functions.sql
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Jan 31, 2023
sql/core/src/test/resources/sql-tests/inputs/string-functions.sql
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Jan 31, 2023
sql/core/src/test/resources/sql-tests/inputs/string-functions.sql
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Jan 31, 2023
sql/core/src/test/scala/org/apache/spark/sql/ColumnExpressionSuite.scala
Outdated
Show resolved
Hide resolved
vinodkc
force-pushed
the
br_udf_luhn_check
branch
from
ca8f807
to
207f391
Compare
cloud-fan
reviewed
Jan 31, 2023
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtils.java
Outdated
Show resolved
Hide resolved
vinodkc
force-pushed
the
br_udf_luhn_check
branch
2 times, most recently
from
3df9507
to
899555d
Compare
cloud-fan
reviewed
Jan 31, 2023
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtils.java
Outdated
Show resolved
Hide resolved
vinodkc
force-pushed
the
br_udf_luhn_check
branch
2 times, most recently
from
e0c0d6c
to
b7af875
Compare
cloud-fan
reviewed
Jan 31, 2023
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtils.java
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Jan 31, 2023
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtils.java
Outdated
Show resolved
Hide resolved
vinodkc
force-pushed
the
br_udf_luhn_check
branch
from
b7af875
to
6044c00
Compare
cloud-fan
approved these changes
Feb 1, 2023
|
thanks, merging to master! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
This PR adds a built-in function to check if a given number string is a valid Luhn number. It shall return true if the number string is a valid Luhn number, and false otherwise.
Why are the changes needed?
This checksum function is widely applied to credit card numbers and government identification numbers to distinguish valid numbers from mistyped, incorrect numbers
Ref : Trino
Postgresql
Does this PR introduce any user-facing change?
Yes, new udf
luhn_checkHow was this patch tested?
Added test cases