You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "vinodkc (via GitHub)" <gi...@apache.org> on 2023/01/25 19:38:04 UTC
[GitHub] [spark] vinodkc opened a new pull request, #39747: [SPARK-40686][SQL] Support udf 'luhn_check'
vinodkc opened a new pull request, #39747:
URL: https://github.com/apache/spark/pull/39747
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
Support UDF 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.
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### 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](https://trino.io/docs/current/functions/string.html)
[Postgresql](https://wiki.postgresql.org/wiki/Luhn_algorithm)
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
Yes, new udf `luhn_check`
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
Added test cases
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] vinodkc commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "vinodkc (via GitHub)" <gi...@apache.org>.
vinodkc commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1090049431
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -3039,3 +3039,100 @@ case class SplitPart (
partNum = newChildren.apply(2))
}
}
+
+/**
+ * Function to check if a given number string is a valid Luhn number. Returns true, if the number
+ * string is a valid Luhn number, false otherwise.
+ */
+@ExpressionDescription(
+ usage = """
+ _FUNC_(str ) - Checks that a string of digits is valid according to the Luhn algorithm.
+ This checksum function is widely applied on credit card numbers and government identification
+ numbers to distinguish valid numbers from mistyped, incorrect numbers.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_('8112189876');
+ true
+ > SELECT _FUNC_('79927398713');
+ true
+ > SELECT _FUNC_('79927398714');
+ false
+ """,
+ since = "3.5.0",
+ group = "string_funcs")
+case class Luhncheck(child: Expression) extends UnaryExpression with ExpectsInputTypes {
+
+ override def nullable: Boolean = false
+
+ override protected def withNewChildInternal(newChild: Expression): Luhncheck =
+ copy(child = newChild)
+
+ override def inputTypes: Seq[AbstractDataType] = Seq(StringType)
+
+ override def dataType: DataType = BooleanType
+
+ /**
+ * Default behavior of evaluation according to the default nullability of UnaryExpression. If
+ * subclass of UnaryExpression override nullable, probably should also override this.
+ */
+ override def eval(input: InternalRow): Any = Luhncheck.isLuhnNumber(child.eval(input))
Review Comment:
Thanks for the review comment.
I updated the code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dtenedor commented on pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on PR #39747:
URL: https://github.com/apache/spark/pull/39747#issuecomment-1409030293
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] vinodkc commented on pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "vinodkc (via GitHub)" <gi...@apache.org>.
vinodkc commented on PR #39747:
URL: https://github.com/apache/spark/pull/39747#issuecomment-1404150949
Hi @dtenedor, @dtenedor @srielau, @HyukjinKwon @cloud-fan , @gengliangwang
Could please review this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1089961244
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -3039,3 +3039,100 @@ case class SplitPart (
partNum = newChildren.apply(2))
}
}
+
+/**
+ * Function to check if a given number string is a valid Luhn number. Returns true, if the number
+ * string is a valid Luhn number, false otherwise.
+ */
+@ExpressionDescription(
+ usage = """
+ _FUNC_(str ) - Checks that a string of digits is valid according to the Luhn algorithm.
+ This checksum function is widely applied on credit card numbers and government identification
+ numbers to distinguish valid numbers from mistyped, incorrect numbers.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_('8112189876');
+ true
+ > SELECT _FUNC_('79927398713');
+ true
+ > SELECT _FUNC_('79927398714');
+ false
+ """,
+ since = "3.5.0",
+ group = "string_funcs")
+case class Luhncheck(child: Expression) extends UnaryExpression with ExpectsInputTypes {
+
+ override def nullable: Boolean = false
+
+ override protected def withNewChildInternal(newChild: Expression): Luhncheck =
+ copy(child = newChild)
+
+ override def inputTypes: Seq[AbstractDataType] = Seq(StringType)
+
+ override def dataType: DataType = BooleanType
+
+ /**
+ * Default behavior of evaluation according to the default nullability of UnaryExpression. If
+ * subclass of UnaryExpression override nullable, probably should also override this.
+ */
+ override def eval(input: InternalRow): Any = Luhncheck.isLuhnNumber(child.eval(input))
+
+ /**
+ * Returns Java source code that can be compiled to evaluate this expression. The default
+ * behavior is to call the eval method of the expression. Concrete expression implementations
+ * should override this to do actual code generation.
+ *
+ * @param ctx
+ * a [[CodegenContext]]
+ * @param ev
+ * an [[ExprCode]] with unique terms.
+ * @return
+ * an [[ExprCode]] containing the Java source code to generate the given expression
+ */
+ override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
+ defineCodeGen(
+ ctx,
+ ev,
+ c => {
+ s"""org.apache.spark.sql.catalyst.expressions.Luhncheck.isLuhnNumber($c)"""
+ })
+ }
+}
+
+object Luhncheck {
Review Comment:
`AesDecrypt` is a good example. We can also move the code to `ExpressionImplUtils`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] vinodkc commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "vinodkc (via GitHub)" <gi...@apache.org>.
vinodkc commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1091322757
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -3039,3 +3039,43 @@ case class SplitPart (
partNum = newChildren.apply(2))
}
}
+
+/**
+ * Function to check if a given number string is a valid Luhn number. Returns true, if the number
+ * string is a valid Luhn number, false otherwise.
+ */
+@ExpressionDescription(
+ usage = """
+ _FUNC_(str ) - Checks that a string of digits is valid according to the Luhn algorithm.
+ This checksum function is widely applied on credit card numbers and government identification
+ numbers to distinguish valid numbers from mistyped, incorrect numbers.
Review Comment:
To check newlines, I tried following tests
`spark-sql> desc function luhn_check;`
```
Function: luhn_check
Class: org.apache.spark.sql.catalyst.expressions.Luhncheck
Usage:
luhn_check(str ) - Checks that a string of digits is valid according to the Luhn algorithm.
This checksum function is widely applied on credit card numbers and government identification
numbers to distinguish valid numbers from mistyped, incorrect numbers.
```
I did the same test on other builtin functions like `substring_index`, `rpad`
```
spark-sql> desc function substring_index;
Function: substring_index
Class: org.apache.spark.sql.catalyst.expressions.SubstringIndex
Usage:
substring_index(str, delim, count) - Returns the substring from `str` before `count` occurrences of the delimiter `delim`.
If `count` is positive, everything to the left of the final delimiter (counting from the
left) is returned. If `count` is negative, everything to the right of the final delimiter
(counting from the right) is returned. The function substring_index performs a case-sensitive match
when searching for `delim`.
```
Please suggest, if it is not suggested to add new lines, I can change the function doc
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] vinodkc commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "vinodkc (via GitHub)" <gi...@apache.org>.
vinodkc commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1092388057
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtils.java:
##########
@@ -35,6 +35,35 @@ public class ExpressionImplUtils {
private static final int GCM_IV_LEN = 12;
private static final int GCM_TAG_LEN = 128;
+ /**
+ * Function to check if a given number string is a valid Luhn number
+ * @param numberString
+ * the number string to check
+ * @return
+ * true if the number string is a valid Luhn number, false otherwise.
+ */
+ public static boolean isLuhnNumber(UTF8String numberString) {
+ String digits = numberString.toString();
+ // Check if all characters in the input string are digits.
+ if (!digits.isEmpty() && digits.chars().allMatch(Character::isDigit)) {
Review Comment:
Done
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtils.java:
##########
@@ -35,6 +35,35 @@ public class ExpressionImplUtils {
private static final int GCM_IV_LEN = 12;
private static final int GCM_TAG_LEN = 128;
+ /**
+ * Function to check if a given number string is a valid Luhn number
+ * @param numberString
+ * the number string to check
+ * @return
+ * true if the number string is a valid Luhn number, false otherwise.
+ */
+ public static boolean isLuhnNumber(UTF8String numberString) {
+ String digits = numberString.toString();
+ // Check if all characters in the input string are digits.
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] vinodkc commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "vinodkc (via GitHub)" <gi...@apache.org>.
vinodkc commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1089504826
##########
sql/core/src/test/resources/sql-tests/inputs/string-functions.sql:
##########
@@ -231,3 +231,39 @@ CREATE TEMPORARY VIEW fmtTable(fmtField) AS SELECT * FROM VALUES ('invalidFormat
SELECT to_binary('abc', fmtField) FROM fmtTable;
-- Clean up
DROP VIEW IF EXISTS fmtTable;
+-- luhn_check
+select luhn_check('4111111111111111');
+select luhn_check('5500000000000004');
+select luhn_check('340000000000009');
+select luhn_check('6011000000000004');
+select luhn_check('378282246310005');
+select luhn_check('6011000990139424');
+select luhn_check('1234567890');
+select luhn_check('4111111111111');
+select luhn_check('4111111111111112');
+select luhn_check('371449635398431');
+select luhn_check('371449635398432');
+select luhn_check('30569309025904');
+select luhn_check('30569309025905');
+select luhn_check('6011111111111117');
+select luhn_check('6011111111111118');
+select luhn_check('3530111333300000');
+select luhn_check('3530111333300001');
+select luhn_check('5105105105105100');
+select luhn_check('5105105105105106');
+select luhn_check('510B105105105106');
+select luhn_check('E105105105105106');
+select luhn_check('-5105105105105106');
+select luhn_check(5105105105105106);
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] vinodkc commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "vinodkc (via GitHub)" <gi...@apache.org>.
vinodkc commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1092388362
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtils.java:
##########
@@ -35,6 +35,35 @@ public class ExpressionImplUtils {
private static final int GCM_IV_LEN = 12;
private static final int GCM_TAG_LEN = 128;
+ /**
+ * Function to check if a given number string is a valid Luhn number
+ * @param numberString
+ * the number string to check
+ * @return
+ * true if the number string is a valid Luhn number, false otherwise.
+ */
+ public static boolean isLuhnNumber(UTF8String numberString) {
+ String digits = numberString.toString();
+ // Check if all characters in the input string are digits.
+ if (digits.isEmpty()) return false;
+ int checkSum = 0;
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dtenedor commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1087116548
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -3039,3 +3039,113 @@ case class SplitPart (
partNum = newChildren.apply(2))
}
}
+
+/**
+ * Function to check if a given number string is a valid Luhn number.
+ * Returns true, if the number string is a valid Luhn number, false otherwise
+ */
+@ExpressionDescription(
+ usage = """
+ _FUNC_(str ) - Checks that a string of digits is valid according to the Luhn algorithm
Review Comment:
```suggestion
_FUNC_(str ) - Checks that a string of digits is valid according to the Luhn algorithm.
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -3039,3 +3039,113 @@ case class SplitPart (
partNum = newChildren.apply(2))
}
}
+
+/**
+ * Function to check if a given number string is a valid Luhn number.
+ * Returns true, if the number string is a valid Luhn number, false otherwise
+ */
+@ExpressionDescription(
+ usage = """
+ _FUNC_(str ) - Checks that a string of digits is valid according to the Luhn algorithm
+ This checksum function is widely applied on credit card numbers and government identification
+ numbers to distinguish valid numbers from mistyped, incorrect numbers.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_('8112189876');
+ true
+ > SELECT _FUNC_('79927398713');
+ true
+ > SELECT _FUNC_('79927398714');
+ false
+ """,
+ since = "3.4.0",
+ group = "string_funcs")
+case class Luhncheck(child: Expression) extends UnaryExpression with ExpectsInputTypes {
+
+ override def nullable: Boolean = false
+
+ override protected def withNewChildInternal(newChild: Expression): Luhncheck =
+ copy(child = newChild)
+
+ /**
+ * Expected input types from child expressions. The i-th position in the returned seq indicates
+ * the type requirement for the i-th child.
+ *
+ * The possible values at each position are:
+ * 1. a specific data type, for example, LongType, StringType.
+ * 2. a non-leaf abstract data type, for example, NumericType, IntegralType, FractionalType.
+ */
+ override def inputTypes: Seq[AbstractDataType] = Seq(StringType)
+
+ /**
+ * Returns the [[DataType]] of the result of evaluating this expression. It is invalid to query
+ * the dataType of an unresolved expression (i.e., when `resolved` == false).
+ */
+ override def dataType: DataType = BooleanType
+
+ /**
+ * Default behavior of evaluation according to the default nullability of UnaryExpression. If
+ * subclass of UnaryExpression override nullable, probably should also override this.
+ */
+ override def eval(input: InternalRow): Any = Luhncheck.isLuhnNumber(child.eval(input))
+
+ /**
+ * Returns Java source code that can be compiled to evaluate this expression. The default
+ * behavior is to call the eval method of the expression. Concrete expression implementations
+ * should override this to do actual code generation.
+ *
+ * @param ctx
+ * a [[CodegenContext]]
+ * @param ev
+ * an [[ExprCode]] with unique terms.
+ * @return
+ * an [[ExprCode]] containing the Java source code to generate the given expression
+ */
+ override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
+ defineCodeGen(
+ ctx,
+ ev,
+ c => {
+ s"""org.apache.spark.sql.catalyst.expressions.Luhncheck.isLuhnNumber($c)"""
+ })
+ }
+}
+
+object Luhncheck {
+
+ /**
+ * Function to check if a given number string is a valid Luhn number
+ *
+ * @param numberString
+ * the number string to check
+ * @return
+ * true if the number string is a valid Luhn number, false otherwise
+ */
+ def isLuhnNumber(numberString: Any): Boolean = {
+ if (numberString != null) {
+ val digits = numberString.asInstanceOf[UTF8String].toString
+ // check if all characters in the input string are digits
+ if (!digits.forall(_.isDigit)) {
+ return false // if not, return false
+ }
+ // reverse the string so that we can use a foldLeft function
Review Comment:
please update the comment to start with a capital letter and end with punctuation (here and everywhere else in the PR)
##########
sql/core/src/test/resources/sql-tests/inputs/string-functions.sql:
##########
@@ -231,3 +231,29 @@ CREATE TEMPORARY VIEW fmtTable(fmtField) AS SELECT * FROM VALUES ('invalidFormat
SELECT to_binary('abc', fmtField) FROM fmtTable;
-- Clean up
DROP VIEW IF EXISTS fmtTable;
+-- luhn_check
+select luhn_check('4111111111111111');
+select luhn_check('5500000000000004');
+select luhn_check('340000000000009');
+select luhn_check('6011000000000004');
+select luhn_check('378282246310005');
+select luhn_check('6011000990139424');
+select luhn_check('1234567890');
+select luhn_check('4111111111111');
+select luhn_check('4111111111111112');
+select luhn_check('371449635398431');
+select luhn_check('371449635398432');
+select luhn_check('30569309025904');
+select luhn_check('30569309025905');
+select luhn_check('6011111111111117');
+select luhn_check('6011111111111118');
+select luhn_check('3530111333300000');
+select luhn_check('3530111333300001');
+select luhn_check('5105105105105100');
+select luhn_check('5105105105105106');
+select luhn_check('510B105105105106');
+select luhn_check('E105105105105106');
Review Comment:
also add some negative test cases where the input is the wrong type? and some positive tests where the input is an attribute reference instead of a constant string?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -3039,3 +3039,113 @@ case class SplitPart (
partNum = newChildren.apply(2))
}
}
+
+/**
+ * Function to check if a given number string is a valid Luhn number.
+ * Returns true, if the number string is a valid Luhn number, false otherwise
+ */
+@ExpressionDescription(
+ usage = """
+ _FUNC_(str ) - Checks that a string of digits is valid according to the Luhn algorithm
+ This checksum function is widely applied on credit card numbers and government identification
+ numbers to distinguish valid numbers from mistyped, incorrect numbers.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_('8112189876');
+ true
+ > SELECT _FUNC_('79927398713');
+ true
+ > SELECT _FUNC_('79927398714');
+ false
+ """,
+ since = "3.4.0",
+ group = "string_funcs")
+case class Luhncheck(child: Expression) extends UnaryExpression with ExpectsInputTypes {
+
+ override def nullable: Boolean = false
+
+ override protected def withNewChildInternal(newChild: Expression): Luhncheck =
+ copy(child = newChild)
+
+ /**
+ * Expected input types from child expressions. The i-th position in the returned seq indicates
+ * the type requirement for the i-th child.
+ *
+ * The possible values at each position are:
+ * 1. a specific data type, for example, LongType, StringType.
+ * 2. a non-leaf abstract data type, for example, NumericType, IntegralType, FractionalType.
+ */
+ override def inputTypes: Seq[AbstractDataType] = Seq(StringType)
+
+ /**
+ * Returns the [[DataType]] of the result of evaluating this expression. It is invalid to query
+ * the dataType of an unresolved expression (i.e., when `resolved` == false).
+ */
+ override def dataType: DataType = BooleanType
+
+ /**
+ * Default behavior of evaluation according to the default nullability of UnaryExpression. If
+ * subclass of UnaryExpression override nullable, probably should also override this.
+ */
+ override def eval(input: InternalRow): Any = Luhncheck.isLuhnNumber(child.eval(input))
+
+ /**
+ * Returns Java source code that can be compiled to evaluate this expression. The default
+ * behavior is to call the eval method of the expression. Concrete expression implementations
+ * should override this to do actual code generation.
+ *
+ * @param ctx
+ * a [[CodegenContext]]
+ * @param ev
+ * an [[ExprCode]] with unique terms.
+ * @return
+ * an [[ExprCode]] containing the Java source code to generate the given expression
+ */
+ override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
+ defineCodeGen(
+ ctx,
+ ev,
+ c => {
+ s"""org.apache.spark.sql.catalyst.expressions.Luhncheck.isLuhnNumber($c)"""
+ })
+ }
+}
+
+object Luhncheck {
+
+ /**
+ * Function to check if a given number string is a valid Luhn number
+ *
+ * @param numberString
+ * the number string to check
+ * @return
+ * true if the number string is a valid Luhn number, false otherwise
+ */
+ def isLuhnNumber(numberString: Any): Boolean = {
+ if (numberString != null) {
+ val digits = numberString.asInstanceOf[UTF8String].toString
+ // check if all characters in the input string are digits
+ if (!digits.forall(_.isDigit)) {
+ return false // if not, return false
Review Comment:
`return` is generally unsafe in Scala/Spark, please use an `if` block instead
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1091348733
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtils.java:
##########
@@ -35,6 +35,35 @@ public class ExpressionImplUtils {
private static final int GCM_IV_LEN = 12;
private static final int GCM_TAG_LEN = 128;
+ /**
+ * Function to check if a given number string is a valid Luhn number
+ * @param numberString
+ * the number string to check
+ * @return
+ * true if the number string is a valid Luhn number, false otherwise.
+ */
+ public static boolean isLuhnNumber(UTF8String numberString) {
+ String digits = numberString.toString();
+ // Check if all characters in the input string are digits.
+ if (!digits.trim().isEmpty() && digits.chars().allMatch(Character::isDigit)) {
Review Comment:
do we need the `trim()` call here? luhn doesn't support spaces.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dtenedor commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1089460097
##########
sql/core/src/test/resources/sql-tests/inputs/string-functions.sql:
##########
@@ -231,3 +231,39 @@ CREATE TEMPORARY VIEW fmtTable(fmtField) AS SELECT * FROM VALUES ('invalidFormat
SELECT to_binary('abc', fmtField) FROM fmtTable;
-- Clean up
DROP VIEW IF EXISTS fmtTable;
+-- luhn_check
+select luhn_check('4111111111111111');
+select luhn_check('5500000000000004');
+select luhn_check('340000000000009');
+select luhn_check('6011000000000004');
+select luhn_check('378282246310005');
+select luhn_check('6011000990139424');
+select luhn_check('1234567890');
+select luhn_check('4111111111111');
+select luhn_check('4111111111111112');
+select luhn_check('371449635398431');
+select luhn_check('371449635398432');
+select luhn_check('30569309025904');
+select luhn_check('30569309025905');
+select luhn_check('6011111111111117');
+select luhn_check('6011111111111118');
+select luhn_check('3530111333300000');
+select luhn_check('3530111333300001');
+select luhn_check('5105105105105100');
+select luhn_check('5105105105105106');
+select luhn_check('510B105105105106');
+select luhn_check('E105105105105106');
+select luhn_check('-5105105105105106');
+select luhn_check(5105105105105106);
Review Comment:
maybe add some cases with whitespace to show the resulting behavior?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -3039,3 +3039,113 @@ case class SplitPart (
partNum = newChildren.apply(2))
}
}
+
+/**
+ * Function to check if a given number string is a valid Luhn number.
+ * Returns true, if the number string is a valid Luhn number, false otherwise
+ */
+@ExpressionDescription(
+ usage = """
+ _FUNC_(str ) - Checks that a string of digits is valid according to the Luhn algorithm.
+ This checksum function is widely applied on credit card numbers and government identification
+ numbers to distinguish valid numbers from mistyped, incorrect numbers.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_('8112189876');
+ true
+ > SELECT _FUNC_('79927398713');
+ true
+ > SELECT _FUNC_('79927398714');
+ false
+ """,
+ since = "3.5.0",
+ group = "string_funcs")
+case class Luhncheck(child: Expression) extends UnaryExpression with ExpectsInputTypes {
+
+ override def nullable: Boolean = false
+
+ override protected def withNewChildInternal(newChild: Expression): Luhncheck =
+ copy(child = newChild)
+
+ /**
+ * Expected input types from child expressions. The i-th position in the returned seq indicates
Review Comment:
no need to copy the method comment from the base `Expression` class unchanged, you can just leave the method un-commented if it is simple and self-explanatory.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -3039,3 +3039,113 @@ case class SplitPart (
partNum = newChildren.apply(2))
}
}
+
+/**
+ * Function to check if a given number string is a valid Luhn number.
+ * Returns true, if the number string is a valid Luhn number, false otherwise
+ */
+@ExpressionDescription(
+ usage = """
+ _FUNC_(str ) - Checks that a string of digits is valid according to the Luhn algorithm.
+ This checksum function is widely applied on credit card numbers and government identification
+ numbers to distinguish valid numbers from mistyped, incorrect numbers.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_('8112189876');
+ true
+ > SELECT _FUNC_('79927398713');
+ true
+ > SELECT _FUNC_('79927398714');
+ false
+ """,
+ since = "3.5.0",
+ group = "string_funcs")
+case class Luhncheck(child: Expression) extends UnaryExpression with ExpectsInputTypes {
+
+ override def nullable: Boolean = false
+
+ override protected def withNewChildInternal(newChild: Expression): Luhncheck =
+ copy(child = newChild)
+
+ /**
+ * Expected input types from child expressions. The i-th position in the returned seq indicates
+ * the type requirement for the i-th child.
+ *
+ * The possible values at each position are:
+ * 1. a specific data type, for example, LongType, StringType.
+ * 2. a non-leaf abstract data type, for example, NumericType, IntegralType, FractionalType.
+ */
+ override def inputTypes: Seq[AbstractDataType] = Seq(StringType)
+
+ /**
+ * Returns the [[DataType]] of the result of evaluating this expression. It is invalid to query
+ * the dataType of an unresolved expression (i.e., when `resolved` == false).
+ */
+ override def dataType: DataType = BooleanType
+
+ /**
+ * Default behavior of evaluation according to the default nullability of UnaryExpression. If
+ * subclass of UnaryExpression override nullable, probably should also override this.
+ */
+ override def eval(input: InternalRow): Any = Luhncheck.isLuhnNumber(child.eval(input))
+
+ /**
+ * Returns Java source code that can be compiled to evaluate this expression. The default
+ * behavior is to call the eval method of the expression. Concrete expression implementations
+ * should override this to do actual code generation.
+ *
+ * @param ctx
+ * a [[CodegenContext]]
+ * @param ev
+ * an [[ExprCode]] with unique terms.
+ * @return
+ * an [[ExprCode]] containing the Java source code to generate the given expression
+ */
+ override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
+ defineCodeGen(
+ ctx,
+ ev,
+ c => {
+ s"""org.apache.spark.sql.catalyst.expressions.Luhncheck.isLuhnNumber($c)"""
+ })
+ }
+}
+
+object Luhncheck {
+
+ /**
+ * Function to check if a given number string is a valid Luhn number
+ *
+ * @param numberString
+ * the number string to check
+ * @return
+ * true if the number string is a valid Luhn number, false otherwise
+ */
+ def isLuhnNumber(numberString: Any): Boolean =
+ numberString match {
+ case number: UTF8String =>
+ val digits = number.toString
+ // Check if all characters in the input string are digits.
+ if (digits.forall(_.isDigit)) {
+ // Reverse the string so that we can use a foldLeft function
+ // and iterate through the digits in reverse order.
+ val (checkSum, _) = digits.reverse.foldLeft((0, false)) { case ((nSum, isSecond), d) =>
+ // Convert the digit character to an int.
+ val digit = d.asDigit
+ // Double the digit if it's the second digit in the sequence.
+ val doubled = if (isSecond) digit * 2 else digit
+ // Add the two digits of the doubled number to the sum.
+ val sum = nSum + (doubled % 10) + (doubled / 10)
+ // Toggle the isSecond flag for the next iteration.
+ (sum, !isSecond)
+ }
+ // Check if the final sum is divisible by 10
+ checkSum % 10 == 0
+ } else {
+ false
+ }
Review Comment:
```suggestion
}
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -3039,3 +3039,113 @@ case class SplitPart (
partNum = newChildren.apply(2))
}
}
+
+/**
+ * Function to check if a given number string is a valid Luhn number.
+ * Returns true, if the number string is a valid Luhn number, false otherwise
+ */
+@ExpressionDescription(
+ usage = """
+ _FUNC_(str ) - Checks that a string of digits is valid according to the Luhn algorithm.
+ This checksum function is widely applied on credit card numbers and government identification
+ numbers to distinguish valid numbers from mistyped, incorrect numbers.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_('8112189876');
+ true
+ > SELECT _FUNC_('79927398713');
+ true
+ > SELECT _FUNC_('79927398714');
+ false
+ """,
+ since = "3.5.0",
+ group = "string_funcs")
+case class Luhncheck(child: Expression) extends UnaryExpression with ExpectsInputTypes {
+
+ override def nullable: Boolean = false
+
+ override protected def withNewChildInternal(newChild: Expression): Luhncheck =
+ copy(child = newChild)
+
+ /**
+ * Expected input types from child expressions. The i-th position in the returned seq indicates
+ * the type requirement for the i-th child.
+ *
+ * The possible values at each position are:
+ * 1. a specific data type, for example, LongType, StringType.
+ * 2. a non-leaf abstract data type, for example, NumericType, IntegralType, FractionalType.
+ */
+ override def inputTypes: Seq[AbstractDataType] = Seq(StringType)
+
+ /**
+ * Returns the [[DataType]] of the result of evaluating this expression. It is invalid to query
+ * the dataType of an unresolved expression (i.e., when `resolved` == false).
+ */
+ override def dataType: DataType = BooleanType
+
+ /**
+ * Default behavior of evaluation according to the default nullability of UnaryExpression. If
+ * subclass of UnaryExpression override nullable, probably should also override this.
+ */
+ override def eval(input: InternalRow): Any = Luhncheck.isLuhnNumber(child.eval(input))
+
+ /**
+ * Returns Java source code that can be compiled to evaluate this expression. The default
+ * behavior is to call the eval method of the expression. Concrete expression implementations
+ * should override this to do actual code generation.
+ *
+ * @param ctx
+ * a [[CodegenContext]]
+ * @param ev
+ * an [[ExprCode]] with unique terms.
+ * @return
+ * an [[ExprCode]] containing the Java source code to generate the given expression
+ */
+ override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
+ defineCodeGen(
+ ctx,
+ ev,
+ c => {
+ s"""org.apache.spark.sql.catalyst.expressions.Luhncheck.isLuhnNumber($c)"""
+ })
+ }
+}
+
+object Luhncheck {
+
+ /**
+ * Function to check if a given number string is a valid Luhn number
+ *
+ * @param numberString
+ * the number string to check
+ * @return
+ * true if the number string is a valid Luhn number, false otherwise
+ */
+ def isLuhnNumber(numberString: Any): Boolean =
+ numberString match {
+ case number: UTF8String =>
+ val digits = number.toString
+ // Check if all characters in the input string are digits.
+ if (digits.forall(_.isDigit)) {
+ // Reverse the string so that we can use a foldLeft function
+ // and iterate through the digits in reverse order.
+ val (checkSum, _) = digits.reverse.foldLeft((0, false)) { case ((nSum, isSecond), d) =>
+ // Convert the digit character to an int.
+ val digit = d.asDigit
+ // Double the digit if it's the second digit in the sequence.
+ val doubled = if (isSecond) digit * 2 else digit
+ // Add the two digits of the doubled number to the sum.
+ val sum = nSum + (doubled % 10) + (doubled / 10)
+ // Toggle the isSecond flag for the next iteration.
+ (sum, !isSecond)
+ }
+ // Check if the final sum is divisible by 10
+ checkSum % 10 == 0
+ } else {
+ false
Review Comment:
```suggestion
false
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "srowen (via GitHub)" <gi...@apache.org>.
srowen commented on PR #39747:
URL: https://github.com/apache/spark/pull/39747#issuecomment-1408711809
I'm not super against this, but this seems too niche to put into the Spark API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1091349674
##########
sql/core/src/test/resources/sql-tests/inputs/string-functions.sql:
##########
@@ -231,3 +231,39 @@ CREATE TEMPORARY VIEW fmtTable(fmtField) AS SELECT * FROM VALUES ('invalidFormat
SELECT to_binary('abc', fmtField) FROM fmtTable;
-- Clean up
DROP VIEW IF EXISTS fmtTable;
+-- luhn_check
+-- basic cases
+select luhn_check('4111111111111111');
+select luhn_check('5500000000000004');
+select luhn_check('340000000000009');
+select luhn_check('6011000000000004');
+select luhn_check('378282246310005');
+select luhn_check('6011000990139424');
+
+-- spaces in the beginning/middle/end
+select luhn_check('4111111111111111 ');
+select luhn_check('4111111 111111111');
+select luhn_check(' 4111111111111111');
+-- space
+select luhn_check('');
+select luhn_check(' ');
+-- non-digits
+select luhn_check('510B105105105106');
+select luhn_check('ABCDED');
+-- null
+select luhn_check(null);
+-- non string (test implicit cast)
+select luhn_check(6011111111111117);
+select luhn_check(6011111111111118);
+select luhn_check(123.456);
+
+select luhn_check('0');
+
+SELECT luhn_check(c1) from values ("4111111111111111"), ("5105105105105106") as tab(c1);
Review Comment:
why do we need extra tests for `from values`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1091349245
##########
sql/core/src/test/resources/sql-tests/inputs/string-functions.sql:
##########
@@ -231,3 +231,39 @@ CREATE TEMPORARY VIEW fmtTable(fmtField) AS SELECT * FROM VALUES ('invalidFormat
SELECT to_binary('abc', fmtField) FROM fmtTable;
-- Clean up
DROP VIEW IF EXISTS fmtTable;
+-- luhn_check
+-- basic cases
+select luhn_check('4111111111111111');
+select luhn_check('5500000000000004');
+select luhn_check('340000000000009');
+select luhn_check('6011000000000004');
+select luhn_check('378282246310005');
+select luhn_check('6011000990139424');
+
+-- spaces in the beginning/middle/end
+select luhn_check('4111111111111111 ');
+select luhn_check('4111111 111111111');
+select luhn_check(' 4111111111111111');
+-- space
+select luhn_check('');
+select luhn_check(' ');
+-- non-digits
+select luhn_check('510B105105105106');
+select luhn_check('ABCDED');
+-- null
+select luhn_check(null);
+-- non string (test implicit cast)
+select luhn_check(6011111111111117);
+select luhn_check(6011111111111118);
+select luhn_check(123.456);
+
+select luhn_check('0');
Review Comment:
shall we move it to the `-- basic cases` section? It's also the number string case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #39747:
URL: https://github.com/apache/spark/pull/39747#issuecomment-1411305718
thanks, merging to master!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1090378992
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -3039,3 +3039,43 @@ case class SplitPart (
partNum = newChildren.apply(2))
}
}
+
+/**
+ * Function to check if a given number string is a valid Luhn number. Returns true, if the number
+ * string is a valid Luhn number, false otherwise.
+ */
+@ExpressionDescription(
+ usage = """
+ _FUNC_(str ) - Checks that a string of digits is valid according to the Luhn algorithm.
+ This checksum function is widely applied on credit card numbers and government identification
+ numbers to distinguish valid numbers from mistyped, incorrect numbers.
Review Comment:
can you try running `desc function luhn_check` locally and check the result? IIRC we should not add new lines in the function doc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1089960943
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -3039,3 +3039,100 @@ case class SplitPart (
partNum = newChildren.apply(2))
}
}
+
+/**
+ * Function to check if a given number string is a valid Luhn number. Returns true, if the number
+ * string is a valid Luhn number, false otherwise.
+ */
+@ExpressionDescription(
+ usage = """
+ _FUNC_(str ) - Checks that a string of digits is valid according to the Luhn algorithm.
+ This checksum function is widely applied on credit card numbers and government identification
+ numbers to distinguish valid numbers from mistyped, incorrect numbers.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_('8112189876');
+ true
+ > SELECT _FUNC_('79927398713');
+ true
+ > SELECT _FUNC_('79927398714');
+ false
+ """,
+ since = "3.5.0",
+ group = "string_funcs")
+case class Luhncheck(child: Expression) extends UnaryExpression with ExpectsInputTypes {
+
+ override def nullable: Boolean = false
+
+ override protected def withNewChildInternal(newChild: Expression): Luhncheck =
+ copy(child = newChild)
+
+ override def inputTypes: Seq[AbstractDataType] = Seq(StringType)
+
+ override def dataType: DataType = BooleanType
+
+ /**
+ * Default behavior of evaluation according to the default nullability of UnaryExpression. If
+ * subclass of UnaryExpression override nullable, probably should also override this.
+ */
+ override def eval(input: InternalRow): Any = Luhncheck.isLuhnNumber(child.eval(input))
Review Comment:
For simple implementation like this, I think we can just make `Luhncheck` exnted `RuntimeReplaceable`, and use `StaticInvoke` as the replacement expression.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -3039,3 +3039,100 @@ case class SplitPart (
partNum = newChildren.apply(2))
}
}
+
+/**
+ * Function to check if a given number string is a valid Luhn number. Returns true, if the number
+ * string is a valid Luhn number, false otherwise.
+ */
+@ExpressionDescription(
+ usage = """
+ _FUNC_(str ) - Checks that a string of digits is valid according to the Luhn algorithm.
+ This checksum function is widely applied on credit card numbers and government identification
+ numbers to distinguish valid numbers from mistyped, incorrect numbers.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_('8112189876');
+ true
+ > SELECT _FUNC_('79927398713');
+ true
+ > SELECT _FUNC_('79927398714');
+ false
+ """,
+ since = "3.5.0",
+ group = "string_funcs")
+case class Luhncheck(child: Expression) extends UnaryExpression with ExpectsInputTypes {
+
+ override def nullable: Boolean = false
+
+ override protected def withNewChildInternal(newChild: Expression): Luhncheck =
+ copy(child = newChild)
+
+ override def inputTypes: Seq[AbstractDataType] = Seq(StringType)
+
+ override def dataType: DataType = BooleanType
+
+ /**
+ * Default behavior of evaluation according to the default nullability of UnaryExpression. If
+ * subclass of UnaryExpression override nullable, probably should also override this.
+ */
+ override def eval(input: InternalRow): Any = Luhncheck.isLuhnNumber(child.eval(input))
Review Comment:
For simple implementation like this, I think we can just make `Luhncheck` extend `RuntimeReplaceable`, and use `StaticInvoke` as the replacement expression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] vinodkc commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "vinodkc (via GitHub)" <gi...@apache.org>.
vinodkc commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1091446740
##########
sql/core/src/test/resources/sql-tests/inputs/string-functions.sql:
##########
@@ -231,3 +231,39 @@ CREATE TEMPORARY VIEW fmtTable(fmtField) AS SELECT * FROM VALUES ('invalidFormat
SELECT to_binary('abc', fmtField) FROM fmtTable;
-- Clean up
DROP VIEW IF EXISTS fmtTable;
+-- luhn_check
+-- basic cases
+select luhn_check('4111111111111111');
+select luhn_check('5500000000000004');
+select luhn_check('340000000000009');
+select luhn_check('6011000000000004');
+select luhn_check('378282246310005');
+select luhn_check('6011000990139424');
Review Comment:
Updated input values to cover all code branches.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1091465324
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtils.java:
##########
@@ -35,6 +35,35 @@ public class ExpressionImplUtils {
private static final int GCM_IV_LEN = 12;
private static final int GCM_TAG_LEN = 128;
+ /**
+ * Function to check if a given number string is a valid Luhn number
+ * @param numberString
+ * the number string to check
+ * @return
+ * true if the number string is a valid Luhn number, false otherwise.
+ */
+ public static boolean isLuhnNumber(UTF8String numberString) {
+ String digits = numberString.toString();
+ // Check if all characters in the input string are digits.
+ if (digits.isEmpty()) return false;
+ int checkSum = 0;
Review Comment:
nit: the indentation is off.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] vinodkc commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "vinodkc (via GitHub)" <gi...@apache.org>.
vinodkc commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1087365898
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -3039,3 +3039,113 @@ case class SplitPart (
partNum = newChildren.apply(2))
}
}
+
+/**
+ * Function to check if a given number string is a valid Luhn number.
+ * Returns true, if the number string is a valid Luhn number, false otherwise
+ */
+@ExpressionDescription(
+ usage = """
+ _FUNC_(str ) - Checks that a string of digits is valid according to the Luhn algorithm
Review Comment:
Done
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -3039,3 +3039,113 @@ case class SplitPart (
partNum = newChildren.apply(2))
}
}
+
+/**
+ * Function to check if a given number string is a valid Luhn number.
+ * Returns true, if the number string is a valid Luhn number, false otherwise
+ */
+@ExpressionDescription(
+ usage = """
+ _FUNC_(str ) - Checks that a string of digits is valid according to the Luhn algorithm
+ This checksum function is widely applied on credit card numbers and government identification
+ numbers to distinguish valid numbers from mistyped, incorrect numbers.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_('8112189876');
+ true
+ > SELECT _FUNC_('79927398713');
+ true
+ > SELECT _FUNC_('79927398714');
+ false
+ """,
+ since = "3.4.0",
+ group = "string_funcs")
+case class Luhncheck(child: Expression) extends UnaryExpression with ExpectsInputTypes {
+
+ override def nullable: Boolean = false
+
+ override protected def withNewChildInternal(newChild: Expression): Luhncheck =
+ copy(child = newChild)
+
+ /**
+ * Expected input types from child expressions. The i-th position in the returned seq indicates
+ * the type requirement for the i-th child.
+ *
+ * The possible values at each position are:
+ * 1. a specific data type, for example, LongType, StringType.
+ * 2. a non-leaf abstract data type, for example, NumericType, IntegralType, FractionalType.
+ */
+ override def inputTypes: Seq[AbstractDataType] = Seq(StringType)
+
+ /**
+ * Returns the [[DataType]] of the result of evaluating this expression. It is invalid to query
+ * the dataType of an unresolved expression (i.e., when `resolved` == false).
+ */
+ override def dataType: DataType = BooleanType
+
+ /**
+ * Default behavior of evaluation according to the default nullability of UnaryExpression. If
+ * subclass of UnaryExpression override nullable, probably should also override this.
+ */
+ override def eval(input: InternalRow): Any = Luhncheck.isLuhnNumber(child.eval(input))
+
+ /**
+ * Returns Java source code that can be compiled to evaluate this expression. The default
+ * behavior is to call the eval method of the expression. Concrete expression implementations
+ * should override this to do actual code generation.
+ *
+ * @param ctx
+ * a [[CodegenContext]]
+ * @param ev
+ * an [[ExprCode]] with unique terms.
+ * @return
+ * an [[ExprCode]] containing the Java source code to generate the given expression
+ */
+ override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
+ defineCodeGen(
+ ctx,
+ ev,
+ c => {
+ s"""org.apache.spark.sql.catalyst.expressions.Luhncheck.isLuhnNumber($c)"""
+ })
+ }
+}
+
+object Luhncheck {
+
+ /**
+ * Function to check if a given number string is a valid Luhn number
+ *
+ * @param numberString
+ * the number string to check
+ * @return
+ * true if the number string is a valid Luhn number, false otherwise
+ */
+ def isLuhnNumber(numberString: Any): Boolean = {
+ if (numberString != null) {
+ val digits = numberString.asInstanceOf[UTF8String].toString
+ // check if all characters in the input string are digits
+ if (!digits.forall(_.isDigit)) {
+ return false // if not, return false
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1090383764
##########
sql/core/src/test/resources/sql-tests/inputs/string-functions.sql:
##########
@@ -231,3 +231,42 @@ CREATE TEMPORARY VIEW fmtTable(fmtField) AS SELECT * FROM VALUES ('invalidFormat
SELECT to_binary('abc', fmtField) FROM fmtTable;
-- Clean up
DROP VIEW IF EXISTS fmtTable;
+-- luhn_check
+select luhn_check('4111111111111111');
+select luhn_check('4111111111111111 ');
+select luhn_check('5500000000000004');
+select luhn_check('340000000000009');
+select luhn_check('6011000000000004');
Review Comment:
Ideally the more tests the better, but we should also think about coverage. The test cases I can think of
1. a few basic cases
2. spaces in the beginning/middle/end
3. non-digits
4. empty string
5. null
6. non string (test implicit cast)
Let's add comments before each test(s) to mention which case it targets. And we don't need so many basic test cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] vinodkc commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "vinodkc (via GitHub)" <gi...@apache.org>.
vinodkc commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1088398561
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -3039,3 +3039,113 @@ case class SplitPart (
partNum = newChildren.apply(2))
}
}
+
+/**
+ * Function to check if a given number string is a valid Luhn number.
+ * Returns true, if the number string is a valid Luhn number, false otherwise
+ */
+@ExpressionDescription(
+ usage = """
+ _FUNC_(str ) - Checks that a string of digits is valid according to the Luhn algorithm.
+ This checksum function is widely applied on credit card numbers and government identification
+ numbers to distinguish valid numbers from mistyped, incorrect numbers.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_('8112189876');
+ true
+ > SELECT _FUNC_('79927398713');
+ true
+ > SELECT _FUNC_('79927398714');
+ false
+ """,
+ since = "3.4.0",
Review Comment:
Updated, thanks for the review comment
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] vinodkc commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "vinodkc (via GitHub)" <gi...@apache.org>.
vinodkc commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1089506295
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -3039,3 +3039,113 @@ case class SplitPart (
partNum = newChildren.apply(2))
}
}
+
+/**
+ * Function to check if a given number string is a valid Luhn number.
+ * Returns true, if the number string is a valid Luhn number, false otherwise
+ */
+@ExpressionDescription(
+ usage = """
+ _FUNC_(str ) - Checks that a string of digits is valid according to the Luhn algorithm.
+ This checksum function is widely applied on credit card numbers and government identification
+ numbers to distinguish valid numbers from mistyped, incorrect numbers.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_('8112189876');
+ true
+ > SELECT _FUNC_('79927398713');
+ true
+ > SELECT _FUNC_('79927398714');
+ false
+ """,
+ since = "3.5.0",
+ group = "string_funcs")
+case class Luhncheck(child: Expression) extends UnaryExpression with ExpectsInputTypes {
+
+ override def nullable: Boolean = false
+
+ override protected def withNewChildInternal(newChild: Expression): Luhncheck =
+ copy(child = newChild)
+
+ /**
+ * Expected input types from child expressions. The i-th position in the returned seq indicates
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1091349494
##########
sql/core/src/test/resources/sql-tests/inputs/string-functions.sql:
##########
@@ -231,3 +231,39 @@ CREATE TEMPORARY VIEW fmtTable(fmtField) AS SELECT * FROM VALUES ('invalidFormat
SELECT to_binary('abc', fmtField) FROM fmtTable;
-- Clean up
DROP VIEW IF EXISTS fmtTable;
+-- luhn_check
+-- basic cases
+select luhn_check('4111111111111111');
+select luhn_check('5500000000000004');
+select luhn_check('340000000000009');
+select luhn_check('6011000000000004');
+select luhn_check('378282246310005');
+select luhn_check('6011000990139424');
Review Comment:
do these 6 tests cover different code branches?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1091350090
##########
sql/core/src/test/scala/org/apache/spark/sql/ColumnExpressionSuite.scala:
##########
@@ -3081,4 +3081,19 @@ class ColumnExpressionSuite extends QueryTest with SharedSparkSession {
checkAnswer(df.select($"dd" / ($"num" + 3)),
Seq((Duration.ofDays(2))).toDF)
}
+
+ test("luhn check") {
Review Comment:
I think the golden file tests fully cover it. If we add a scala API for luhn check, we need this test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] vinodkc commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "vinodkc (via GitHub)" <gi...@apache.org>.
vinodkc commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1091446189
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtils.java:
##########
@@ -35,6 +35,35 @@ public class ExpressionImplUtils {
private static final int GCM_IV_LEN = 12;
private static final int GCM_TAG_LEN = 128;
+ /**
+ * Function to check if a given number string is a valid Luhn number
+ * @param numberString
+ * the number string to check
+ * @return
+ * true if the number string is a valid Luhn number, false otherwise.
+ */
+ public static boolean isLuhnNumber(UTF8String numberString) {
+ String digits = numberString.toString();
+ // Check if all characters in the input string are digits.
+ if (!digits.trim().isEmpty() && digits.chars().allMatch(Character::isDigit)) {
Review Comment:
trim is not required, removed it now
##########
sql/core/src/test/resources/sql-tests/inputs/string-functions.sql:
##########
@@ -231,3 +231,39 @@ CREATE TEMPORARY VIEW fmtTable(fmtField) AS SELECT * FROM VALUES ('invalidFormat
SELECT to_binary('abc', fmtField) FROM fmtTable;
-- Clean up
DROP VIEW IF EXISTS fmtTable;
+-- luhn_check
+-- basic cases
+select luhn_check('4111111111111111');
+select luhn_check('5500000000000004');
+select luhn_check('340000000000009');
+select luhn_check('6011000000000004');
+select luhn_check('378282246310005');
+select luhn_check('6011000990139424');
+
+-- spaces in the beginning/middle/end
+select luhn_check('4111111111111111 ');
+select luhn_check('4111111 111111111');
+select luhn_check(' 4111111111111111');
+-- space
+select luhn_check('');
+select luhn_check(' ');
+-- non-digits
+select luhn_check('510B105105105106');
+select luhn_check('ABCDED');
+-- null
+select luhn_check(null);
+-- non string (test implicit cast)
+select luhn_check(6011111111111117);
+select luhn_check(6011111111111118);
+select luhn_check(123.456);
+
+select luhn_check('0');
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] vinodkc commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "vinodkc (via GitHub)" <gi...@apache.org>.
vinodkc commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1091445199
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtils.java:
##########
@@ -35,6 +35,39 @@ public class ExpressionImplUtils {
private static final int GCM_IV_LEN = 12;
private static final int GCM_TAG_LEN = 128;
+ /**
+ * Function to check if a given number string is a valid Luhn number
+ * @param numberString
+ * the number string to check
+ * @return
+ * true if the number string is a valid Luhn number, false otherwise.
+ */
+ public static boolean isLuhnNumber(UTF8String numberString) {
+ String digits = numberString.toString();
+ // Check if all characters in the input string are digits.
+ java.util.function.Predicate<Character> isDigit = c -> Character.isDigit(c);
+ java.util.function.Predicate<Character> isNotWhitespace = c -> !Character.isWhitespace(c);
+ if (!digits.isEmpty() &&
+ digits.chars().allMatch(
+ c -> Character.isDigit(c) && !Character.isWhitespace((char) c))) {
Review Comment:
By mistake that change got added, fixed the condition now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dtenedor commented on pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on PR #39747:
URL: https://github.com/apache/spark/pull/39747#issuecomment-1409008814
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.
[1] https://issues.apache.org/jira/browse/SPARK-40686
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] vinodkc commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "vinodkc (via GitHub)" <gi...@apache.org>.
vinodkc commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1090049452
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -3039,3 +3039,100 @@ case class SplitPart (
partNum = newChildren.apply(2))
}
}
+
+/**
+ * Function to check if a given number string is a valid Luhn number. Returns true, if the number
+ * string is a valid Luhn number, false otherwise.
+ */
+@ExpressionDescription(
+ usage = """
+ _FUNC_(str ) - Checks that a string of digits is valid according to the Luhn algorithm.
+ This checksum function is widely applied on credit card numbers and government identification
+ numbers to distinguish valid numbers from mistyped, incorrect numbers.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_('8112189876');
+ true
+ > SELECT _FUNC_('79927398713');
+ true
+ > SELECT _FUNC_('79927398714');
+ false
+ """,
+ since = "3.5.0",
+ group = "string_funcs")
+case class Luhncheck(child: Expression) extends UnaryExpression with ExpectsInputTypes {
+
+ override def nullable: Boolean = false
+
+ override protected def withNewChildInternal(newChild: Expression): Luhncheck =
+ copy(child = newChild)
+
+ override def inputTypes: Seq[AbstractDataType] = Seq(StringType)
+
+ override def dataType: DataType = BooleanType
+
+ /**
+ * Default behavior of evaluation according to the default nullability of UnaryExpression. If
+ * subclass of UnaryExpression override nullable, probably should also override this.
+ */
+ override def eval(input: InternalRow): Any = Luhncheck.isLuhnNumber(child.eval(input))
+
+ /**
+ * Returns Java source code that can be compiled to evaluate this expression. The default
+ * behavior is to call the eval method of the expression. Concrete expression implementations
+ * should override this to do actual code generation.
+ *
+ * @param ctx
+ * a [[CodegenContext]]
+ * @param ev
+ * an [[ExprCode]] with unique terms.
+ * @return
+ * an [[ExprCode]] containing the Java source code to generate the given expression
+ */
+ override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
+ defineCodeGen(
+ ctx,
+ ev,
+ c => {
+ s"""org.apache.spark.sql.catalyst.expressions.Luhncheck.isLuhnNumber($c)"""
+ })
+ }
+}
+
+object Luhncheck {
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "srowen (via GitHub)" <gi...@apache.org>.
srowen commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1091441853
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtils.java:
##########
@@ -35,6 +35,39 @@ public class ExpressionImplUtils {
private static final int GCM_IV_LEN = 12;
private static final int GCM_TAG_LEN = 128;
+ /**
+ * Function to check if a given number string is a valid Luhn number
+ * @param numberString
+ * the number string to check
+ * @return
+ * true if the number string is a valid Luhn number, false otherwise.
+ */
+ public static boolean isLuhnNumber(UTF8String numberString) {
+ String digits = numberString.toString();
+ // Check if all characters in the input string are digits.
+ java.util.function.Predicate<Character> isDigit = c -> Character.isDigit(c);
+ java.util.function.Predicate<Character> isNotWhitespace = c -> !Character.isWhitespace(c);
+ if (!digits.isEmpty() &&
+ digits.chars().allMatch(
+ c -> Character.isDigit(c) && !Character.isWhitespace((char) c))) {
Review Comment:
(It does not)
BTW, I think using allMatch is going to be non-trivial overhead. You're already iterating over chars, just check in the loop below and short-circuit if something isn't a digit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] vinodkc commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "vinodkc (via GitHub)" <gi...@apache.org>.
vinodkc commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1087366037
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -3039,3 +3039,113 @@ case class SplitPart (
partNum = newChildren.apply(2))
}
}
+
+/**
+ * Function to check if a given number string is a valid Luhn number.
+ * Returns true, if the number string is a valid Luhn number, false otherwise
+ */
+@ExpressionDescription(
+ usage = """
+ _FUNC_(str ) - Checks that a string of digits is valid according to the Luhn algorithm
+ This checksum function is widely applied on credit card numbers and government identification
+ numbers to distinguish valid numbers from mistyped, incorrect numbers.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_('8112189876');
+ true
+ > SELECT _FUNC_('79927398713');
+ true
+ > SELECT _FUNC_('79927398714');
+ false
+ """,
+ since = "3.4.0",
+ group = "string_funcs")
+case class Luhncheck(child: Expression) extends UnaryExpression with ExpectsInputTypes {
+
+ override def nullable: Boolean = false
+
+ override protected def withNewChildInternal(newChild: Expression): Luhncheck =
+ copy(child = newChild)
+
+ /**
+ * Expected input types from child expressions. The i-th position in the returned seq indicates
+ * the type requirement for the i-th child.
+ *
+ * The possible values at each position are:
+ * 1. a specific data type, for example, LongType, StringType.
+ * 2. a non-leaf abstract data type, for example, NumericType, IntegralType, FractionalType.
+ */
+ override def inputTypes: Seq[AbstractDataType] = Seq(StringType)
+
+ /**
+ * Returns the [[DataType]] of the result of evaluating this expression. It is invalid to query
+ * the dataType of an unresolved expression (i.e., when `resolved` == false).
+ */
+ override def dataType: DataType = BooleanType
+
+ /**
+ * Default behavior of evaluation according to the default nullability of UnaryExpression. If
+ * subclass of UnaryExpression override nullable, probably should also override this.
+ */
+ override def eval(input: InternalRow): Any = Luhncheck.isLuhnNumber(child.eval(input))
+
+ /**
+ * Returns Java source code that can be compiled to evaluate this expression. The default
+ * behavior is to call the eval method of the expression. Concrete expression implementations
+ * should override this to do actual code generation.
+ *
+ * @param ctx
+ * a [[CodegenContext]]
+ * @param ev
+ * an [[ExprCode]] with unique terms.
+ * @return
+ * an [[ExprCode]] containing the Java source code to generate the given expression
+ */
+ override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
+ defineCodeGen(
+ ctx,
+ ev,
+ c => {
+ s"""org.apache.spark.sql.catalyst.expressions.Luhncheck.isLuhnNumber($c)"""
+ })
+ }
+}
+
+object Luhncheck {
+
+ /**
+ * Function to check if a given number string is a valid Luhn number
+ *
+ * @param numberString
+ * the number string to check
+ * @return
+ * true if the number string is a valid Luhn number, false otherwise
+ */
+ def isLuhnNumber(numberString: Any): Boolean = {
+ if (numberString != null) {
+ val digits = numberString.asInstanceOf[UTF8String].toString
+ // check if all characters in the input string are digits
+ if (!digits.forall(_.isDigit)) {
+ return false // if not, return false
+ }
+ // reverse the string so that we can use a foldLeft function
Review Comment:
Done
##########
sql/core/src/test/resources/sql-tests/inputs/string-functions.sql:
##########
@@ -231,3 +231,29 @@ CREATE TEMPORARY VIEW fmtTable(fmtField) AS SELECT * FROM VALUES ('invalidFormat
SELECT to_binary('abc', fmtField) FROM fmtTable;
-- Clean up
DROP VIEW IF EXISTS fmtTable;
+-- luhn_check
+select luhn_check('4111111111111111');
+select luhn_check('5500000000000004');
+select luhn_check('340000000000009');
+select luhn_check('6011000000000004');
+select luhn_check('378282246310005');
+select luhn_check('6011000990139424');
+select luhn_check('1234567890');
+select luhn_check('4111111111111');
+select luhn_check('4111111111111112');
+select luhn_check('371449635398431');
+select luhn_check('371449635398432');
+select luhn_check('30569309025904');
+select luhn_check('30569309025905');
+select luhn_check('6011111111111117');
+select luhn_check('6011111111111118');
+select luhn_check('3530111333300000');
+select luhn_check('3530111333300001');
+select luhn_check('5105105105105100');
+select luhn_check('5105105105105106');
+select luhn_check('510B105105105106');
+select luhn_check('E105105105105106');
Review Comment:
Added additional test cases
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1091465224
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtils.java:
##########
@@ -35,6 +35,35 @@ public class ExpressionImplUtils {
private static final int GCM_IV_LEN = 12;
private static final int GCM_TAG_LEN = 128;
+ /**
+ * Function to check if a given number string is a valid Luhn number
+ * @param numberString
+ * the number string to check
+ * @return
+ * true if the number string is a valid Luhn number, false otherwise.
+ */
+ public static boolean isLuhnNumber(UTF8String numberString) {
+ String digits = numberString.toString();
+ // Check if all characters in the input string are digits.
Review Comment:
```suggestion
// Empty string is not a valid Luhn number
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] vinodkc commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "vinodkc (via GitHub)" <gi...@apache.org>.
vinodkc commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1091338386
##########
sql/core/src/test/resources/sql-tests/inputs/string-functions.sql:
##########
@@ -231,3 +231,42 @@ CREATE TEMPORARY VIEW fmtTable(fmtField) AS SELECT * FROM VALUES ('invalidFormat
SELECT to_binary('abc', fmtField) FROM fmtTable;
-- Clean up
DROP VIEW IF EXISTS fmtTable;
+-- luhn_check
+select luhn_check('4111111111111111');
+select luhn_check('4111111111111111 ');
+select luhn_check('5500000000000004');
+select luhn_check('340000000000009');
+select luhn_check('6011000000000004');
Review Comment:
I added suggested test cases
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1091450516
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtils.java:
##########
@@ -35,6 +35,35 @@ public class ExpressionImplUtils {
private static final int GCM_IV_LEN = 12;
private static final int GCM_TAG_LEN = 128;
+ /**
+ * Function to check if a given number string is a valid Luhn number
+ * @param numberString
+ * the number string to check
+ * @return
+ * true if the number string is a valid Luhn number, false otherwise.
+ */
+ public static boolean isLuhnNumber(UTF8String numberString) {
+ String digits = numberString.toString();
+ // Check if all characters in the input string are digits.
+ if (!digits.isEmpty() && digits.chars().allMatch(Character::isDigit)) {
Review Comment:
As @srowen mentioned, how about returning false in the for loop if space is found? the code can be
```
if (digits.isEmpty()) return false;
for ...
if (!Character.isDigit(digits.charAt(i))) return false;
...
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] vinodkc commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "vinodkc (via GitHub)" <gi...@apache.org>.
vinodkc commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1091310570
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtils.java:
##########
@@ -35,6 +35,35 @@ public class ExpressionImplUtils {
private static final int GCM_IV_LEN = 12;
private static final int GCM_TAG_LEN = 128;
+ /**
+ * Function to check if a given number string is a valid Luhn number
+ * @param numberString
+ * the number string to check
+ * @return
+ * true if the number string is a valid Luhn number, false otherwise.
+ */
+ public static boolean isLuhnNumber(UTF8String numberString) {
+ String digits = numberString.toString();
+ // Check if all characters in the input string are digits.
+ if (!digits.trim().isEmpty() && digits.chars().allMatch(Character::isDigit)) {
+ int checkSum = 0;
+ boolean isSecond = false;
+ for (int i = digits.length() - 1; i >= 0; i--) {
+ int digit = Character.getNumericValue(digits.charAt(i));
Review Comment:
spaces are not supported in a Luhn number check, so it will return false
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] vinodkc commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "vinodkc (via GitHub)" <gi...@apache.org>.
vinodkc commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1089505331
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -3039,3 +3039,113 @@ case class SplitPart (
partNum = newChildren.apply(2))
}
}
+
+/**
+ * Function to check if a given number string is a valid Luhn number.
+ * Returns true, if the number string is a valid Luhn number, false otherwise
+ */
+@ExpressionDescription(
+ usage = """
+ _FUNC_(str ) - Checks that a string of digits is valid according to the Luhn algorithm.
+ This checksum function is widely applied on credit card numbers and government identification
+ numbers to distinguish valid numbers from mistyped, incorrect numbers.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_('8112189876');
+ true
+ > SELECT _FUNC_('79927398713');
+ true
+ > SELECT _FUNC_('79927398714');
+ false
+ """,
+ since = "3.5.0",
+ group = "string_funcs")
+case class Luhncheck(child: Expression) extends UnaryExpression with ExpectsInputTypes {
+
+ override def nullable: Boolean = false
+
+ override protected def withNewChildInternal(newChild: Expression): Luhncheck =
+ copy(child = newChild)
+
+ /**
+ * Expected input types from child expressions. The i-th position in the returned seq indicates
+ * the type requirement for the i-th child.
+ *
+ * The possible values at each position are:
+ * 1. a specific data type, for example, LongType, StringType.
+ * 2. a non-leaf abstract data type, for example, NumericType, IntegralType, FractionalType.
+ */
+ override def inputTypes: Seq[AbstractDataType] = Seq(StringType)
+
+ /**
+ * Returns the [[DataType]] of the result of evaluating this expression. It is invalid to query
+ * the dataType of an unresolved expression (i.e., when `resolved` == false).
+ */
+ override def dataType: DataType = BooleanType
+
+ /**
+ * Default behavior of evaluation according to the default nullability of UnaryExpression. If
+ * subclass of UnaryExpression override nullable, probably should also override this.
+ */
+ override def eval(input: InternalRow): Any = Luhncheck.isLuhnNumber(child.eval(input))
+
+ /**
+ * Returns Java source code that can be compiled to evaluate this expression. The default
+ * behavior is to call the eval method of the expression. Concrete expression implementations
+ * should override this to do actual code generation.
+ *
+ * @param ctx
+ * a [[CodegenContext]]
+ * @param ev
+ * an [[ExprCode]] with unique terms.
+ * @return
+ * an [[ExprCode]] containing the Java source code to generate the given expression
+ */
+ override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
+ defineCodeGen(
+ ctx,
+ ev,
+ c => {
+ s"""org.apache.spark.sql.catalyst.expressions.Luhncheck.isLuhnNumber($c)"""
+ })
+ }
+}
+
+object Luhncheck {
+
+ /**
+ * Function to check if a given number string is a valid Luhn number
+ *
+ * @param numberString
+ * the number string to check
+ * @return
+ * true if the number string is a valid Luhn number, false otherwise
+ */
+ def isLuhnNumber(numberString: Any): Boolean =
+ numberString match {
+ case number: UTF8String =>
+ val digits = number.toString
+ // Check if all characters in the input string are digits.
+ if (digits.forall(_.isDigit)) {
+ // Reverse the string so that we can use a foldLeft function
+ // and iterate through the digits in reverse order.
+ val (checkSum, _) = digits.reverse.foldLeft((0, false)) { case ((nSum, isSecond), d) =>
+ // Convert the digit character to an int.
+ val digit = d.asDigit
+ // Double the digit if it's the second digit in the sequence.
+ val doubled = if (isSecond) digit * 2 else digit
+ // Add the two digits of the doubled number to the sum.
+ val sum = nSum + (doubled % 10) + (doubled / 10)
+ // Toggle the isSecond flag for the next iteration.
+ (sum, !isSecond)
+ }
+ // Check if the final sum is divisible by 10
+ checkSum % 10 == 0
+ } else {
+ false
+ }
Review Comment:
Done
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -3039,3 +3039,113 @@ case class SplitPart (
partNum = newChildren.apply(2))
}
}
+
+/**
+ * Function to check if a given number string is a valid Luhn number.
+ * Returns true, if the number string is a valid Luhn number, false otherwise
+ */
+@ExpressionDescription(
+ usage = """
+ _FUNC_(str ) - Checks that a string of digits is valid according to the Luhn algorithm.
+ This checksum function is widely applied on credit card numbers and government identification
+ numbers to distinguish valid numbers from mistyped, incorrect numbers.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_('8112189876');
+ true
+ > SELECT _FUNC_('79927398713');
+ true
+ > SELECT _FUNC_('79927398714');
+ false
+ """,
+ since = "3.5.0",
+ group = "string_funcs")
+case class Luhncheck(child: Expression) extends UnaryExpression with ExpectsInputTypes {
+
+ override def nullable: Boolean = false
+
+ override protected def withNewChildInternal(newChild: Expression): Luhncheck =
+ copy(child = newChild)
+
+ /**
+ * Expected input types from child expressions. The i-th position in the returned seq indicates
+ * the type requirement for the i-th child.
+ *
+ * The possible values at each position are:
+ * 1. a specific data type, for example, LongType, StringType.
+ * 2. a non-leaf abstract data type, for example, NumericType, IntegralType, FractionalType.
+ */
+ override def inputTypes: Seq[AbstractDataType] = Seq(StringType)
+
+ /**
+ * Returns the [[DataType]] of the result of evaluating this expression. It is invalid to query
+ * the dataType of an unresolved expression (i.e., when `resolved` == false).
+ */
+ override def dataType: DataType = BooleanType
+
+ /**
+ * Default behavior of evaluation according to the default nullability of UnaryExpression. If
+ * subclass of UnaryExpression override nullable, probably should also override this.
+ */
+ override def eval(input: InternalRow): Any = Luhncheck.isLuhnNumber(child.eval(input))
+
+ /**
+ * Returns Java source code that can be compiled to evaluate this expression. The default
+ * behavior is to call the eval method of the expression. Concrete expression implementations
+ * should override this to do actual code generation.
+ *
+ * @param ctx
+ * a [[CodegenContext]]
+ * @param ev
+ * an [[ExprCode]] with unique terms.
+ * @return
+ * an [[ExprCode]] containing the Java source code to generate the given expression
+ */
+ override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
+ defineCodeGen(
+ ctx,
+ ev,
+ c => {
+ s"""org.apache.spark.sql.catalyst.expressions.Luhncheck.isLuhnNumber($c)"""
+ })
+ }
+}
+
+object Luhncheck {
+
+ /**
+ * Function to check if a given number string is a valid Luhn number
+ *
+ * @param numberString
+ * the number string to check
+ * @return
+ * true if the number string is a valid Luhn number, false otherwise
+ */
+ def isLuhnNumber(numberString: Any): Boolean =
+ numberString match {
+ case number: UTF8String =>
+ val digits = number.toString
+ // Check if all characters in the input string are digits.
+ if (digits.forall(_.isDigit)) {
+ // Reverse the string so that we can use a foldLeft function
+ // and iterate through the digits in reverse order.
+ val (checkSum, _) = digits.reverse.foldLeft((0, false)) { case ((nSum, isSecond), d) =>
+ // Convert the digit character to an int.
+ val digit = d.asDigit
+ // Double the digit if it's the second digit in the sequence.
+ val doubled = if (isSecond) digit * 2 else digit
+ // Add the two digits of the doubled number to the sum.
+ val sum = nSum + (doubled % 10) + (doubled / 10)
+ // Toggle the isSecond flag for the next iteration.
+ (sum, !isSecond)
+ }
+ // Check if the final sum is divisible by 10
+ checkSum % 10 == 0
+ } else {
+ false
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1087436365
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -3039,3 +3039,113 @@ case class SplitPart (
partNum = newChildren.apply(2))
}
}
+
+/**
+ * Function to check if a given number string is a valid Luhn number.
+ * Returns true, if the number string is a valid Luhn number, false otherwise
+ */
+@ExpressionDescription(
+ usage = """
+ _FUNC_(str ) - Checks that a string of digits is valid according to the Luhn algorithm.
+ This checksum function is widely applied on credit card numbers and government identification
+ numbers to distinguish valid numbers from mistyped, incorrect numbers.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_('8112189876');
+ true
+ > SELECT _FUNC_('79927398713');
+ true
+ > SELECT _FUNC_('79927398714');
+ false
+ """,
+ since = "3.4.0",
Review Comment:
Apache Spark entered a feature freeze by creating `branch-3.4`. This should be `3.5.0`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] vinodkc commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "vinodkc (via GitHub)" <gi...@apache.org>.
vinodkc commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1088398561
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -3039,3 +3039,113 @@ case class SplitPart (
partNum = newChildren.apply(2))
}
}
+
+/**
+ * Function to check if a given number string is a valid Luhn number.
+ * Returns true, if the number string is a valid Luhn number, false otherwise
+ */
+@ExpressionDescription(
+ usage = """
+ _FUNC_(str ) - Checks that a string of digits is valid according to the Luhn algorithm.
+ This checksum function is widely applied on credit card numbers and government identification
+ numbers to distinguish valid numbers from mistyped, incorrect numbers.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_('8112189876');
+ true
+ > SELECT _FUNC_('79927398713');
+ true
+ > SELECT _FUNC_('79927398714');
+ false
+ """,
+ since = "3.4.0",
Review Comment:
Updated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dtenedor commented on pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on PR #39747:
URL: https://github.com/apache/spark/pull/39747#issuecomment-1404167805
The general algorithm and test coverage look correct
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "srowen (via GitHub)" <gi...@apache.org>.
srowen commented on PR #39747:
URL: https://github.com/apache/spark/pull/39747#issuecomment-1409016849
Right, I don't think most of those belong in the Spark API. This is what UDF and utility libs are for
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1091427571
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtils.java:
##########
@@ -35,6 +35,39 @@ public class ExpressionImplUtils {
private static final int GCM_IV_LEN = 12;
private static final int GCM_TAG_LEN = 128;
+ /**
+ * Function to check if a given number string is a valid Luhn number
+ * @param numberString
+ * the number string to check
+ * @return
+ * true if the number string is a valid Luhn number, false otherwise.
+ */
+ public static boolean isLuhnNumber(UTF8String numberString) {
+ String digits = numberString.toString();
+ // Check if all characters in the input string are digits.
+ java.util.function.Predicate<Character> isDigit = c -> Character.isDigit(c);
+ java.util.function.Predicate<Character> isNotWhitespace = c -> !Character.isWhitespace(c);
+ if (!digits.isEmpty() &&
+ digits.chars().allMatch(
+ c -> Character.isDigit(c) && !Character.isWhitespace((char) c))) {
Review Comment:
I'm confused. `Character.isDigit(c)` returns true for spaces?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] vinodkc commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "vinodkc (via GitHub)" <gi...@apache.org>.
vinodkc commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1091447034
##########
sql/core/src/test/resources/sql-tests/inputs/string-functions.sql:
##########
@@ -231,3 +231,39 @@ CREATE TEMPORARY VIEW fmtTable(fmtField) AS SELECT * FROM VALUES ('invalidFormat
SELECT to_binary('abc', fmtField) FROM fmtTable;
-- Clean up
DROP VIEW IF EXISTS fmtTable;
+-- luhn_check
+-- basic cases
+select luhn_check('4111111111111111');
+select luhn_check('5500000000000004');
+select luhn_check('340000000000009');
+select luhn_check('6011000000000004');
+select luhn_check('378282246310005');
+select luhn_check('6011000990139424');
+
+-- spaces in the beginning/middle/end
+select luhn_check('4111111111111111 ');
+select luhn_check('4111111 111111111');
+select luhn_check(' 4111111111111111');
+-- space
+select luhn_check('');
+select luhn_check(' ');
+-- non-digits
+select luhn_check('510B105105105106');
+select luhn_check('ABCDED');
+-- null
+select luhn_check(null);
+-- non string (test implicit cast)
+select luhn_check(6011111111111117);
+select luhn_check(6011111111111118);
+select luhn_check(123.456);
+
+select luhn_check('0');
+
+SELECT luhn_check(c1) from values ("4111111111111111"), ("5105105105105106") as tab(c1);
Review Comment:
Removed `from values` tests
##########
sql/core/src/test/scala/org/apache/spark/sql/ColumnExpressionSuite.scala:
##########
@@ -3081,4 +3081,19 @@ class ColumnExpressionSuite extends QueryTest with SharedSparkSession {
checkAnswer(df.select($"dd" / ($"num" + 3)),
Seq((Duration.ofDays(2))).toDF)
}
+
+ test("luhn check") {
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1090377071
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtils.java:
##########
@@ -35,6 +35,35 @@ public class ExpressionImplUtils {
private static final int GCM_IV_LEN = 12;
private static final int GCM_TAG_LEN = 128;
+ /**
+ * Function to check if a given number string is a valid Luhn number
+ * @param numberString
+ * the number string to check
+ * @return
+ * true if the number string is a valid Luhn number, false otherwise.
+ */
+ public static boolean isLuhnNumber(UTF8String numberString) {
+ String digits = numberString.toString();
+ // Check if all characters in the input string are digits.
+ if (!digits.trim().isEmpty() && digits.chars().allMatch(Character::isDigit)) {
+ int checkSum = 0;
+ boolean isSecond = false;
+ for (int i = digits.length() - 1; i >= 0; i--) {
+ int digit = Character.getNumericValue(digits.charAt(i));
Review Comment:
shall we handle space here? Or we should trim at the beginning: `String digits = numberString.toString().trim();`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #39747:
URL: https://github.com/apache/spark/pull/39747#discussion_r1090383764
##########
sql/core/src/test/resources/sql-tests/inputs/string-functions.sql:
##########
@@ -231,3 +231,42 @@ CREATE TEMPORARY VIEW fmtTable(fmtField) AS SELECT * FROM VALUES ('invalidFormat
SELECT to_binary('abc', fmtField) FROM fmtTable;
-- Clean up
DROP VIEW IF EXISTS fmtTable;
+-- luhn_check
+select luhn_check('4111111111111111');
+select luhn_check('4111111111111111 ');
+select luhn_check('5500000000000004');
+select luhn_check('340000000000009');
+select luhn_check('6011000000000004');
Review Comment:
Ideally the more tests the better, but we should also think about coverage. The test cases I can think of
1. a few basic cases
2. spaces in the beginning/middle/end
3. non-digits
4. empty string
5. null
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan closed pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #39747: [SPARK-42191][SQL] Support udf 'luhn_check'
URL: https://github.com/apache/spark/pull/39747
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org