Re: [PR] feat: Implement Spark-compatible CAST between integer types [datafusion-comet]
andygrove merged PR #340: URL: https://github.com/apache/datafusion-comet/pull/340 -- 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: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org
Re: [PR] feat: Implement Spark-compatible CAST between integer types [datafusion-comet]
andygrove commented on PR #340:
URL: https://github.com/apache/datafusion-comet/pull/340#issuecomment-209373
@ganeshkumar269 here is my suggestion:
```scala
if (CometSparkSessionExtensions.isSpark34Plus) {
// for Spark 3.4 we expect to reproduce the error message
exactly
assert(cometMessage == sparkMessage)
} else if (CometSparkSessionExtensions.isSpark33Plus) {
// for Spark 3.3 we just need to strip the prefix from the
Comet message
// before comparing
val cometMessageModified = cometMessage
.replace("[CAST_INVALID_INPUT] ", "")
.replace("[CAST_OVERFLOW] ", "")
assert(cometMessageModified == sparkMessage)
} else if (CometSparkSessionExtensions.isSpark32) {
// for Spark 3.2 we just make sure we are seeing a similar
type of error
if (sparkMessage.contains("causes overflow")) {
assert(cometMessage.contains("due to an overflow"))
} else {
// assume that this is an invalid input message in the form:
// `invalid input syntax for type numeric:
-9223372036854775809`
// we just check that the Comet message contains the same
literal value
val i = sparkMessage.indexOf(':') + 2
assert(cometMessage.contains(sparkMessage.substring(i)))
}
}
```
I tested this from the command line for all Spark versions using:
```
mvn test -Pspark-3.2 -DwildcardSuites=org.apache.comet.CometCastSuite
-Dspotless.check.skip=true
```
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
Re: [PR] feat: Implement Spark-compatible CAST between integer types [datafusion-comet]
ganeshkumar269 commented on PR #340: URL: https://github.com/apache/datafusion-comet/pull/340#issuecomment-2093731716 > @ganeshkumar269 My original code for comparing errors in 3.2/3.3 was not very robust. I am also looking at this now to see if I can help improve these checks. is there a way I could help here? -- 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: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org
Re: [PR] feat: Implement Spark-compatible CAST between integer types [datafusion-comet]
andygrove commented on PR #340: URL: https://github.com/apache/datafusion-comet/pull/340#issuecomment-2093685799 @ganeshkumar269 My original code for comparing errors in 3.2/3.3 was not very robust. I am also looking at this now to see if I can help improve these checks. -- 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: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org
Re: [PR] feat: Implement Spark-compatible CAST between integer types [datafusion-comet]
andygrove commented on PR #340: URL: https://github.com/apache/datafusion-comet/pull/340#issuecomment-2093626420 @ganeshkumar269 It looks like the error message check needs a little more work. Some tests are failing on Spark 3.3. This error message does not contain `:` or `overflow`. In this specific case, looking for `CAST_INVALID_INPUT` would be a more robust check. ``` - cast StringType to LongType *** FAILED *** (416 milliseconds) "[CAST_INVALID_INPUT] The value '-9223372036854775809' of the type "STRING" cannot be cast to "BIGINT" because it is malformed. Correct the value as per the syntax, or change its target type. Use `try_cast` to tolerate malformed input and return NULL instead. If necessary set "spark.sql.ansi.enabled" to "false" to bypass this error." did not contain "overflow" (CometCastSuite.scala:881) ``` -- 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: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org
Re: [PR] feat: Implement Spark-compatible CAST between integer types [datafusion-comet]
ganeshkumar269 commented on PR #340: URL: https://github.com/apache/datafusion-comet/pull/340#issuecomment-2093556497 thanks @andygrove 🙏🏾 , glad to be a contributor to comet. -- 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: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org
Re: [PR] feat: Implement Spark-compatible CAST between integer types [datafusion-comet]
andygrove commented on code in PR #340:
URL: https://github.com/apache/datafusion-comet/pull/340#discussion_r1589576324
##
spark/src/test/scala/org/apache/comet/CometCastSuite.scala:
##
@@ -665,6 +665,30 @@ class CometCastSuite extends CometTestBase with
AdaptiveSparkPlanHelper {
castTest(generateTimestamps(), DataTypes.DateType)
}
+ test("cast short to byte") {
Review Comment:
These methods already exist in main but have different naming, so I think
you need to upmerge/rebase against main.
Example:
```
ignore("cast ShortType to ByteType")
```
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
Re: [PR] feat: Implement Spark-compatible CAST between integer types [datafusion-comet]
ganeshkumar269 commented on code in PR #340:
URL: https://github.com/apache/datafusion-comet/pull/340#discussion_r1589427151
##
spark/src/test/scala/org/apache/comet/CometCastSuite.scala:
##
@@ -807,11 +833,22 @@ class CometCastSuite extends CometTestBase with
AdaptiveSparkPlanHelper {
} else {
// Spark 3.2 and 3.3 have a different error message format so we
can't do a direct
// comparison between Spark and Comet.
+ // In the case of CAST_INVALID_INPUT
// Spark message is in format `invalid input syntax for type
TYPE: VALUE`
// Comet message is in format `The value 'VALUE' of the type
FROM_TYPE cannot be cast to TO_TYPE`
// We just check that the comet message contains the same
invalid value as the Spark message
- val sparkInvalidValue =
sparkMessage.substring(sparkMessage.indexOf(':') + 2)
- assert(cometMessage.contains(sparkInvalidValue))
+ // In the case of CAST_OVERFLOW
+ // Spark message is in format `Casting VALUE to TO_TYPE causes
overflow`
+ // Comet message is in format `The value 'VALUE' of the type
FROM_TYPE cannot be cast to TO_TYPE
+ // due to an overflow`
+ // We check if the comet message contains 'overflow'.
+ val sparkInvalidValue = if (sparkMessage.indexOf(':') == -1) {
+EMPTY_STRING
+ } else {
+sparkMessage.substring(sparkMessage.indexOf(':') + 2)
+ }
+ assert(
+cometMessage.contains(sparkInvalidValue) ||
cometMessage.contains("overflow"))
Review Comment:
doesnt look like overflow error message has ':' in it, i ran
spark.sql("select cast(9223372036854775807 as int)").show() in my local on
various spark versions.
**3.4** - [CAST_OVERFLOW] The value 9223372036854775807L of the type
"BIGINT" cannot be cast to "INT" due to an overflow. Use `try_cast` to tolerate
overflow and return NULL instead. If necessary set "spark.sql.ansi.enabled" to
"false" to bypass this error
**3.3** - The value 9223372036854775807L of the type "BIGINT" cannot be cast
to "INT" due to an overflow. Use `try_cast` to tolerate overflow and return
NULL instead. If necessary set "spark.sql.ansi.enabled" to "false" to bypass
this error
**3.2** - Casting 9223372036854775807 to int causes overflow
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
Re: [PR] feat: Implement Spark-compatible CAST between integer types [datafusion-comet]
ganeshkumar269 commented on code in PR #340:
URL: https://github.com/apache/datafusion-comet/pull/340#discussion_r1589427151
##
spark/src/test/scala/org/apache/comet/CometCastSuite.scala:
##
@@ -807,11 +833,22 @@ class CometCastSuite extends CometTestBase with
AdaptiveSparkPlanHelper {
} else {
// Spark 3.2 and 3.3 have a different error message format so we
can't do a direct
// comparison between Spark and Comet.
+ // In the case of CAST_INVALID_INPUT
// Spark message is in format `invalid input syntax for type
TYPE: VALUE`
// Comet message is in format `The value 'VALUE' of the type
FROM_TYPE cannot be cast to TO_TYPE`
// We just check that the comet message contains the same
invalid value as the Spark message
- val sparkInvalidValue =
sparkMessage.substring(sparkMessage.indexOf(':') + 2)
- assert(cometMessage.contains(sparkInvalidValue))
+ // In the case of CAST_OVERFLOW
+ // Spark message is in format `Casting VALUE to TO_TYPE causes
overflow`
+ // Comet message is in format `The value 'VALUE' of the type
FROM_TYPE cannot be cast to TO_TYPE
+ // due to an overflow`
+ // We check if the comet message contains 'overflow'.
+ val sparkInvalidValue = if (sparkMessage.indexOf(':') == -1) {
+EMPTY_STRING
+ } else {
+sparkMessage.substring(sparkMessage.indexOf(':') + 2)
+ }
+ assert(
+cometMessage.contains(sparkInvalidValue) ||
cometMessage.contains("overflow"))
Review Comment:
doesnt look like overflow error message has ':' in it.
**3.4** - [CAST_OVERFLOW] The value 9223372036854775807L of the type
"BIGINT" cannot be cast to "INT" due to an overflow. Use `try_cast` to tolerate
overflow and return NULL instead. If necessary set "spark.sql.ansi.enabled" to
"false" to bypass this error
**3.3** - The value 9223372036854775807L of the type "BIGINT" cannot be cast
to "INT" due to an overflow. Use `try_cast` to tolerate overflow and return
NULL instead. If necessary set "spark.sql.ansi.enabled" to "false" to bypass
this error
**3.2** - Casting 9223372036854775807 to int causes overflow
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
Re: [PR] feat: Implement Spark-compatible CAST between integer types [datafusion-comet]
andygrove commented on code in PR #340:
URL: https://github.com/apache/datafusion-comet/pull/340#discussion_r1588373466
##
spark/src/test/scala/org/apache/comet/CometCastSuite.scala:
##
@@ -807,11 +833,22 @@ class CometCastSuite extends CometTestBase with
AdaptiveSparkPlanHelper {
} else {
// Spark 3.2 and 3.3 have a different error message format so we
can't do a direct
// comparison between Spark and Comet.
+ // In the case of CAST_INVALID_INPUT
// Spark message is in format `invalid input syntax for type
TYPE: VALUE`
// Comet message is in format `The value 'VALUE' of the type
FROM_TYPE cannot be cast to TO_TYPE`
// We just check that the comet message contains the same
invalid value as the Spark message
- val sparkInvalidValue =
sparkMessage.substring(sparkMessage.indexOf(':') + 2)
- assert(cometMessage.contains(sparkInvalidValue))
+ // In the case of CAST_OVERFLOW
+ // Spark message is in format `Casting VALUE to TO_TYPE causes
overflow`
+ // Comet message is in format `The value 'VALUE' of the type
FROM_TYPE cannot be cast to TO_TYPE
+ // due to an overflow`
+ // We check if the comet message contains 'overflow'.
+ val sparkInvalidValue = if (sparkMessage.indexOf(':') == -1) {
+EMPTY_STRING
+ } else {
+sparkMessage.substring(sparkMessage.indexOf(':') + 2)
+ }
+ assert(
+cometMessage.contains(sparkInvalidValue) ||
cometMessage.contains("overflow"))
Review Comment:
Yes, something like that. I haven't reviewed the overflow messages to see if
they contain `:` though (in any of the spark versions 3.2, 3.3, and 3.4)
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
Re: [PR] feat: Implement Spark-compatible CAST between integer types [datafusion-comet]
ganeshkumar269 commented on code in PR #340:
URL: https://github.com/apache/datafusion-comet/pull/340#discussion_r1588000457
##
spark/src/test/scala/org/apache/comet/CometCastSuite.scala:
##
@@ -807,11 +833,22 @@ class CometCastSuite extends CometTestBase with
AdaptiveSparkPlanHelper {
} else {
// Spark 3.2 and 3.3 have a different error message format so we
can't do a direct
// comparison between Spark and Comet.
+ // In the case of CAST_INVALID_INPUT
// Spark message is in format `invalid input syntax for type
TYPE: VALUE`
// Comet message is in format `The value 'VALUE' of the type
FROM_TYPE cannot be cast to TO_TYPE`
// We just check that the comet message contains the same
invalid value as the Spark message
- val sparkInvalidValue =
sparkMessage.substring(sparkMessage.indexOf(':') + 2)
- assert(cometMessage.contains(sparkInvalidValue))
+ // In the case of CAST_OVERFLOW
+ // Spark message is in format `Casting VALUE to TO_TYPE causes
overflow`
+ // Comet message is in format `The value 'VALUE' of the type
FROM_TYPE cannot be cast to TO_TYPE
+ // due to an overflow`
+ // We check if the comet message contains 'overflow'.
+ val sparkInvalidValue = if (sparkMessage.indexOf(':') == -1) {
+EMPTY_STRING
+ } else {
+sparkMessage.substring(sparkMessage.indexOf(':') + 2)
+ }
+ assert(
+cometMessage.contains(sparkInvalidValue) ||
cometMessage.contains("overflow"))
Review Comment:
you are right, my bad 😅 . so incase sparkMessage doesnt have ':' should I
assert on just commetMessage.contains("overflow")
something like this,
```
if sparkMessage.indexOf(':') == -1 then
assert(commetMessage.contains("overflow"))
else assert(commetMessage.contains(sparkInvalidValue))
```
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
Re: [PR] feat: Implement Spark-compatible CAST between integer types [datafusion-comet]
ganeshkumar269 commented on code in PR #340:
URL: https://github.com/apache/datafusion-comet/pull/340#discussion_r1588000457
##
spark/src/test/scala/org/apache/comet/CometCastSuite.scala:
##
@@ -807,11 +833,22 @@ class CometCastSuite extends CometTestBase with
AdaptiveSparkPlanHelper {
} else {
// Spark 3.2 and 3.3 have a different error message format so we
can't do a direct
// comparison between Spark and Comet.
+ // In the case of CAST_INVALID_INPUT
// Spark message is in format `invalid input syntax for type
TYPE: VALUE`
// Comet message is in format `The value 'VALUE' of the type
FROM_TYPE cannot be cast to TO_TYPE`
// We just check that the comet message contains the same
invalid value as the Spark message
- val sparkInvalidValue =
sparkMessage.substring(sparkMessage.indexOf(':') + 2)
- assert(cometMessage.contains(sparkInvalidValue))
+ // In the case of CAST_OVERFLOW
+ // Spark message is in format `Casting VALUE to TO_TYPE causes
overflow`
+ // Comet message is in format `The value 'VALUE' of the type
FROM_TYPE cannot be cast to TO_TYPE
+ // due to an overflow`
+ // We check if the comet message contains 'overflow'.
+ val sparkInvalidValue = if (sparkMessage.indexOf(':') == -1) {
+EMPTY_STRING
+ } else {
+sparkMessage.substring(sparkMessage.indexOf(':') + 2)
+ }
+ assert(
+cometMessage.contains(sparkInvalidValue) ||
cometMessage.contains("overflow"))
Review Comment:
you are right, my bad 😅 . so incase sparkMessage doesnt have ':' should I
assert on just commetMessage.contains("overflow")
something like this,
```
if sparkMessage.indexOf(':') == -1 then
assert(commetMessage.contains("overflow"))
else assert(commetMessage.contains(sparkInvalidValue) ||
commetMessage.contains("overflow"))
```
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
Re: [PR] feat: Implement Spark-compatible CAST between integer types [datafusion-comet]
andygrove commented on code in PR #340:
URL: https://github.com/apache/datafusion-comet/pull/340#discussion_r1586886509
##
spark/src/test/scala/org/apache/comet/CometCastSuite.scala:
##
@@ -807,11 +833,22 @@ class CometCastSuite extends CometTestBase with
AdaptiveSparkPlanHelper {
} else {
// Spark 3.2 and 3.3 have a different error message format so we
can't do a direct
// comparison between Spark and Comet.
+ // In the case of CAST_INVALID_INPUT
// Spark message is in format `invalid input syntax for type
TYPE: VALUE`
// Comet message is in format `The value 'VALUE' of the type
FROM_TYPE cannot be cast to TO_TYPE`
// We just check that the comet message contains the same
invalid value as the Spark message
- val sparkInvalidValue =
sparkMessage.substring(sparkMessage.indexOf(':') + 2)
- assert(cometMessage.contains(sparkInvalidValue))
+ // In the case of CAST_OVERFLOW
+ // Spark message is in format `Casting VALUE to TO_TYPE causes
overflow`
+ // Comet message is in format `The value 'VALUE' of the type
FROM_TYPE cannot be cast to TO_TYPE
+ // due to an overflow`
+ // We check if the comet message contains 'overflow'.
+ val sparkInvalidValue = if (sparkMessage.indexOf(':') == -1) {
+EMPTY_STRING
+ } else {
+sparkMessage.substring(sparkMessage.indexOf(':') + 2)
+ }
+ assert(
+cometMessage.contains(sparkInvalidValue) ||
cometMessage.contains("overflow"))
Review Comment:
If `sparkInvalidValue` is `EMPTY_STRING`, won't
`cometMessage.contains(sparkInvalidValue)` always be true?
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
Re: [PR] feat: Implement Spark-compatible CAST between integer types [datafusion-comet]
ganeshkumar269 commented on PR #340: URL: https://github.com/apache/datafusion-comet/pull/340#issuecomment-2088848565 Hi @andygrove , i have added a check before we fetch sparkInvalidValue, defaulting it to EMPTY_STRING if ':' is not present. Also added additional comments on why we are checking for the presence of 'overflow' string. -- 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: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org
Re: [PR] feat: Implement Spark-compatible CAST between integer types [datafusion-comet]
andygrove commented on PR #340: URL: https://github.com/apache/datafusion-comet/pull/340#issuecomment-2087678461 > thanks for the inputs @viirya @andygrove , I have added another check in the assert statement where we compare exception messages. Do you think this approach looks good? or do I need to separate the "invalid cast" and "overflow" assert statements. > > If this change is fine, I will modify the code comments to accommodate "overflow" exception aswell. I think this general approach is OK to handle different types of expected errors. I left a specific comment on the code as well. -- 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: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org
Re: [PR] feat: Implement Spark-compatible CAST between integer types [datafusion-comet]
andygrove commented on code in PR #340:
URL: https://github.com/apache/datafusion-comet/pull/340#discussion_r1585636219
##
spark/src/test/scala/org/apache/comet/CometCastSuite.scala:
##
@@ -722,7 +746,8 @@ class CometCastSuite extends CometTestBase with
AdaptiveSparkPlanHelper {
// Comet message is in format `The value 'VALUE' of the type
FROM_TYPE cannot be cast to TO_TYPE`
// We just check that the comet message contains the same
invalid value as the Spark message
val sparkInvalidValue =
sparkMessage.substring(sparkMessage.indexOf(':') + 2)
Review Comment:
Now that we are handling multiple types of error here, we should probably
check if `sparkMessage.indexOf(':')` returns a non-zero value before trying to
use it.
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
Re: [PR] feat: Implement Spark-compatible CAST between integer types [datafusion-comet]
ganeshkumar269 commented on PR #340: URL: https://github.com/apache/datafusion-comet/pull/340#issuecomment-2087182424 thanks for the inputs @viirya @andygrove , I have added another check in the assert statement where we compare exception messages. Do you think this approach looks good? or do I need to separate the "invalid cast" and "overflow" assert statements. If this change is fine, I will modify the code comments to accommodate "overflow" exception aswell. -- 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: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org
Re: [PR] feat: Implement Spark-compatible CAST between integer types [datafusion-comet]
andygrove commented on PR #340: URL: https://github.com/apache/datafusion-comet/pull/340#issuecomment-2085669471 As @viirya said, we can handle the difference in error message format in the scala test (we already have examples of this in the `castTest` method). For versions prior to 3.4 perhaps you could just check for the word "overflow" in the error message. ``` - cast short to byte *** FAILED *** (439 milliseconds) "Execution error: [CAST_OVERFLOW] The value 18716S of the type "SMALLINT" cannot be cast to "TINYINT" due to an overflow. Use `try_cast` to tolerate overflow and return NULL instead. If necessary set "spark.sql.ansi.enabled" to "false" to bypass this error." did not contain "asting 18716 to tinyint causes overflow" (CometCastSuite.scala:200) ``` -- 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: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org
Re: [PR] feat: Implement Spark-compatible CAST between integer types [datafusion-comet]
viirya commented on PR #340: URL: https://github.com/apache/datafusion-comet/pull/340#issuecomment-2083495794 > Also reg the errors in the CI pipeline, looks like error message for 3.2 is a bit different compared to 3.3 and 3.4 for overflow cases. so in the rust code I will have to add a check for spark version and return the error message accordingly. > How can I get spark version from the rust side? For this kind of case, we tend to return same error message at native side, but handle this difference in Scala tests. -- 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: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org
Re: [PR] feat: Implement Spark-compatible CAST between integer types [datafusion-comet]
ganeshkumar269 commented on PR #340: URL: https://github.com/apache/datafusion-comet/pull/340#issuecomment-2083227842 Hi @viirya @andygrove , firstly please let me know if this PR aligns with the expectations on how to fix the issue, if not kindly provide pointers on how I can move in the right direction. Also reg the errors in the CI pipeline, looks like error message for 3.2 is a bit different compared to 3.3 and 3.4 for overflow cases. so in the rust code I will have to add a check for spark version and return the error message accordingly. How can I get spark version from the rust side? -- 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: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org
