You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "mihailom-db (via GitHub)" <gi...@apache.org> on 2024/10/16 15:10:01 UTC
[PR] [WIP][SPARK-49993][SQL] Improve error messages for Conv, Sum and Average [spark]
mihailom-db opened a new pull request, #48499:
URL: https://github.com/apache/spark/pull/48499
### What changes were proposed in this pull request?
This PR improves messages for ANSI related issues for Conv, Sum and Average. It also adds try_conv expressions that sets failOnError to false by default.
### Why are the changes needed?
The [PR](https://github.com/apache/spark/pull/48206) for removing ANSI suggestion in ARITHMETIC_OVERFLOW was getting too big, so this PR aims to split the work into multiple tasks.
### Does this PR introduce _any_ user-facing change?
Yes, new suggestions are added to do try_conv, try_sum and try_average
### How was this patch tested?
Tests added.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
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
Re: [PR] [WIP][SPARK-49993][SQL] Improve error messages for Conv, Sum and Average [spark]
Posted by "mihailom-db (via GitHub)" <gi...@apache.org>.
mihailom-db commented on code in PR #48499:
URL: https://github.com/apache/spark/pull/48499#discussion_r1803352688
##########
sql/core/src/test/resources/sql-tests/results/ansi/math.sql.out:
##########
@@ -761,7 +761,7 @@ org.apache.spark.SparkArithmeticException
"errorClass" : "ARITHMETIC_OVERFLOW",
"sqlState" : "22003",
"messageParameters" : {
- "alternative" : "",
+ "alternative" : " Use 'try_conv' to tolerate overflow and return NULL instead.",
Review Comment:
Agreed. This is what @MaxGekk pointed out as well in the https://github.com/apache/spark/pull/48206. We will deal with this change there, as this PR is only here to improve messages and fix errors that do not have suggestions apart from ANSI turn 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
Re: [PR] [WIP][SPARK-49993][SQL] Improve error messages for Conv, Sum and Average [spark]
Posted by "mihailom-db (via GitHub)" <gi...@apache.org>.
mihailom-db commented on PR #48499:
URL: https://github.com/apache/spark/pull/48499#issuecomment-2418949365
@MaxGekk @HyukjinKwon I added tests and fixed one test in DataFrameSuite, as it was not completely using aggFn parameter
--
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
Re: [PR] [SPARK-49993][SQL] Improve error messages for Sum and Average [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #48499: [SPARK-49993][SQL] Improve error messages for Sum and Average
URL: https://github.com/apache/spark/pull/48499
--
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
Re: [PR] [WIP][SPARK-49993][SQL] Improve error messages for Conv, Sum and Average [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #48499:
URL: https://github.com/apache/spark/pull/48499#discussion_r1803825405
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala:
##########
@@ -418,6 +418,47 @@ case class Acosh(child: Expression)
override protected def withNewChildInternal(newChild: Expression): Acosh = copy(child = newChild)
}
+/**
+ * Try to convert a num from one base to another
+ *
+ * @param numExpr the number to be converted
+ * @param fromBaseExpr from which base
+ * @param toBaseExpr to which base
+ */
+@ExpressionDescription(
+ usage = "_FUNC_(num, from_base, to_base) - Convert `num` from `from_base` to `to_base`.",
+ examples = """
+ Examples:
+ > SELECT _FUNC_('100', 2, 10);
+ 4
+ > SELECT _FUNC_(-10, 16, -10);
+ -16
Review Comment:
I believe we should show at least one example of the `NULL` result.
--
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
Re: [PR] [SPARK-49993][SQL] Improve error messages for Sum and Average [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #48499:
URL: https://github.com/apache/spark/pull/48499#issuecomment-2441867297
+1, LGTM. Merging to master.
Thank you, @mihailom-db and @srielau @HyukjinKwon for review.
--
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
Re: [PR] [WIP][SPARK-49993][SQL] Improve error messages for Conv, Sum and Average [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #48499:
URL: https://github.com/apache/spark/pull/48499#discussion_r1803829603
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/decimalExpressions.scala:
##########
@@ -270,7 +272,8 @@ case class DecimalDivideWithOverflowCheck(
if (nullOnOverflow) {
null
} else {
- throw QueryExecutionErrors.overflowInSumOfDecimalError(getContextOrNull())
+ throw QueryExecutionErrors.overflowInSumOfDecimalError(getContextOrNull(),
+ suggestedFunc = "try_average")
Review Comment:
Is there this functions? I have found only `try_avg`:
https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala#L462
--
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
Re: [PR] [WIP][SPARK-49993][SQL] Improve error messages for Conv, Sum and Average [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #48499:
URL: https://github.com/apache/spark/pull/48499#discussion_r1803818653
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala:
##########
@@ -418,6 +418,47 @@ case class Acosh(child: Expression)
override protected def withNewChildInternal(newChild: Expression): Acosh = copy(child = newChild)
}
+/**
+ * Try to convert a num from one base to another
+ *
+ * @param numExpr the number to be converted
+ * @param fromBaseExpr from which base
+ * @param toBaseExpr to which base
+ */
+@ExpressionDescription(
+ usage = "_FUNC_(num, from_base, to_base) - Convert `num` from `from_base` to `to_base`.",
+ examples = """
+ Examples:
+ > SELECT _FUNC_('100', 2, 10);
+ 4
+ > SELECT _FUNC_(-10, 16, -10);
+ -16
+ """,
+ since = "4.0.0",
+ group = "math_funcs")
+case class TryConv(
+ numExpr: Expression,
+ fromBaseExpr: Expression,
+ toBaseExpr: Expression,
+ replacement: Expression)
+ extends RuntimeReplaceable with InheritAnalysisRules {
+
+ def this(
+ numExpr: Expression,
+ fromBaseExpr: Expression,
+ toBaseExpr: Expression) =
Review Comment:
Please, fix indentations.
--
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
Re: [PR] [WIP][SPARK-49993][SQL] Improve error messages for Conv, Sum and Average [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #48499:
URL: https://github.com/apache/spark/pull/48499#discussion_r1803341654
##########
sql/core/src/test/resources/sql-tests/results/ansi/math.sql.out:
##########
@@ -761,7 +761,7 @@ org.apache.spark.SparkArithmeticException
"errorClass" : "ARITHMETIC_OVERFLOW",
"sqlState" : "22003",
"messageParameters" : {
- "alternative" : "",
+ "alternative" : " Use 'try_conv' to tolerate overflow and return NULL instead.",
Review Comment:
Fundamental: Why do we need to put a sentence into alternative? Is there a way to put the nglish portion into the message text?
--
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
Re: [PR] [WIP][SPARK-49993][SQL] Improve error messages for Conv, Sum and Average [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #48499:
URL: https://github.com/apache/spark/pull/48499#discussion_r1803826795
##########
python/pyspark/sql/functions/builtin.py:
##########
@@ -7248,6 +7248,41 @@ def log2(col: "ColumnOrName") -> Column:
return _invoke_function_over_columns("log2", col)
+@_try_remote_functions
+def try_conv(col: "ColumnOrName", fromBase: int, toBase: int) -> Column:
+ """
+ This is a special version of `conv` that performs the same operation, but returns a
+ NULL value instead of raising an error if the decoding cannot be performed.
+
+ .. versionadded:: 4.0.0
+
+ Parameters
+ ----------
+ col : :class:`~pyspark.sql.Column` or str
+ a column to convert base for.
+ fromBase: int
+ from base number.
+ toBase: int
+ to base number.
+
+ Returns
+ -------
+ :class:`~pyspark.sql.Column`
+ logariphm of given value.
+
+ Examples
+ --------
+ >>> df = spark.createDataFrame([("010101",)], ['n'])
+ >>> df.select(try_conv(df.n, 2, 16).alias('hex')).collect()
+ [Row(hex='15')]
Review Comment:
can you show an example of NULL result?
--
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