You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "coderfender (via GitHub)" <gi...@apache.org> on 2025/12/17 00:21:08 UTC
[PR] feat: Support string decimal cast [datafusion-comet]
coderfender opened a new pull request, #2925:
URL: https://github.com/apache/datafusion-comet/pull/2925
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## How are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing 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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support string decimal cast [datafusion-comet]
Posted by "coderfender (via GitHub)" <gi...@apache.org>.
coderfender commented on PR #2925:
URL: https://github.com/apache/datafusion-comet/pull/2925#issuecomment-3680143962
Addressed double iteration and we now validate and parse in a single iteration
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support string decimal cast [datafusion-comet]
Posted by "coderfender (via GitHub)" <gi...@apache.org>.
coderfender commented on code in PR #2925:
URL: https://github.com/apache/datafusion-comet/pull/2925#discussion_r2641058678
##########
spark/src/test/scala/org/apache/comet/CometCastSuite.scala:
##########
@@ -684,21 +683,88 @@ class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
+// This is to pass the first `all cast combinations are covered`
ignore("cast StringType to DecimalType(10,2)") {
Review Comment:
@andygrove this test cant be enabled given that we are still marking the casts as incompatible . However I added the same tests (with a different name to make sure they pass) . Enabling this test will fail 'all cast combinations are covered'
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support string decimal cast [datafusion-comet]
Posted by "coderfender (via GitHub)" <gi...@apache.org>.
coderfender commented on PR #2925:
URL: https://github.com/apache/datafusion-comet/pull/2925#issuecomment-3663339660
Sure @andygrove . Let me run some benchmarks on the cast op and update this thread
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support string decimal cast [datafusion-comet]
Posted by "coderfender (via GitHub)" <gi...@apache.org>.
coderfender commented on PR #2925:
URL: https://github.com/apache/datafusion-comet/pull/2925#issuecomment-3678046392
Pushed commit to not have expensive 'trim' and split by '.' operations. Working on the rather difficult part of merging decimal validation and parsing in a single pass
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support string decimal cast [datafusion-comet]
Posted by "coderfender (via GitHub)" <gi...@apache.org>.
coderfender commented on PR #2925:
URL: https://github.com/apache/datafusion-comet/pull/2925#issuecomment-3684114952
Thank you @andygrove
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support string decimal cast [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #2925:
URL: https://github.com/apache/datafusion-comet/pull/2925#issuecomment-3712099081
> Late comment, but did rust_decimal not work for us?
I didn't know this at the time, but there are security issues with rust_decimal (this is currently blocking PR builds in the datafusion core project).
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support string decimal cast [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #2925:
URL: https://github.com/apache/datafusion-comet/pull/2925#issuecomment-3663382328
@coderfender, the functionality looks good overall. There are likely some edge cases we do not support that we need to handle or document. I have only found a few so far, and here is a suggested test to add.
```scala
test("cast StringType to DecimalType non-ASCII characters") {
// TODO fix for Spark 4.0.0
assume(!isSpark40Plus)
val values = Seq(
"\uff11\uff12\uff13", // fullwidth digits １２３
"123\u0000", // null byte at end
"12\u00003", // null byte in middle
"\u0000123", // null byte at start
null).toDF("a")
Seq(true, false).foreach(k =>
castTest(values, DataTypes.createDecimalType(10, 2), testAnsi = k))
}
```
```
!== Correct Answer - 5 == == Spark Answer - 5 ==
struct<a:string,a:decimal(10,2)> struct<a:string,a:decimal(10,2)>
[null,null] [null,null]
![ 123,123.00] [ 123,null]
[12 3,null] [12 3,null]
![123 ,123.00] [123 ,null]
![１２３,123.00] [１２３,null]
```
I think it is fine if we do not support everything, but we should mark the cast as `Incompatible` and add documentation explaining the current limitations.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support string decimal cast [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #2925:
URL: https://github.com/apache/datafusion-comet/pull/2925#issuecomment-3684033931
Thanks @coderfender. This LGTM but could you also update the compatibility guide. It currently states `Does not support inputs ending with 'd' or 'f'. Does not support 'inf'. Does not support ANSI mode. Returns 0.0 instead of null if input contains no digits` and this is not entirely correct now.
With the docs update, I'll be happy to approve.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support string decimal cast [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #2925:
URL: https://github.com/apache/datafusion-comet/pull/2925#discussion_r2625345509
##########
native/spark-expr/src/conversion_funcs/cast.rs:
##########
@@ -1976,6 +1975,363 @@ fn do_cast_string_to_int<
Ok(Some(result))
}
+fn cast_string_to_decimal(
+ array: &ArrayRef,
+ to_type: &DataType,
+ precision: &u8,
+ scale: &i8,
+ eval_mode: EvalMode,
+) -> SparkResult<ArrayRef> {
+ match to_type {
+ DataType::Decimal128(_, _) => {
+ cast_string_to_decimal128_impl(array, eval_mode, *precision, *scale)
+ }
+ DataType::Decimal256(_, _) => {
+ cast_string_to_decimal256_impl(array, eval_mode, *precision, *scale)
+ }
+ _ => Err(SparkError::Internal(format!(
+ "Unexpected type in cast_string_to_decimal: {:?}",
+ to_type
+ ))),
+ }
+}
+
+fn cast_string_to_decimal128_impl(
+ array: &ArrayRef,
+ eval_mode: EvalMode,
+ precision: u8,
+ scale: i8,
+) -> SparkResult<ArrayRef> {
+ let string_array = array
+ .as_any()
+ .downcast_ref::<StringArray>()
+ .ok_or_else(|| SparkError::Internal("Expected string array".to_string()))?;
+
+ let mut decimal_builder = Decimal128Builder::with_capacity(string_array.len());
+
+ for i in 0..string_array.len() {
+ if string_array.is_null(i) {
+ decimal_builder.append_null();
+ } else {
+ let str_value = string_array.value(i).trim();
+ match parse_string_to_decimal(str_value, precision, scale) {
+ Ok(Some(decimal_value)) => {
+ decimal_builder.append_value(decimal_value);
+ }
+ Ok(None) => {
+ if eval_mode == EvalMode::Ansi {
+ return Err(invalid_value(
+ string_array.value(i),
+ "STRING",
+ &format!("DECIMAL({},{})", precision, scale),
+ ));
+ }
+ decimal_builder.append_null();
+ }
+ Err(e) => {
+ if eval_mode == EvalMode::Ansi {
+ return Err(e);
+ }
+ decimal_builder.append_null();
+ }
+ }
+ }
+ }
+
+ Ok(Arc::new(
+ decimal_builder
+ .with_precision_and_scale(precision, scale)?
+ .finish(),
+ ))
+}
+
+fn cast_string_to_decimal256_impl(
+ array: &ArrayRef,
+ eval_mode: EvalMode,
+ precision: u8,
+ scale: i8,
+) -> SparkResult<ArrayRef> {
+ let string_array = array
+ .as_any()
+ .downcast_ref::<StringArray>()
+ .ok_or_else(|| SparkError::Internal("Expected string array".to_string()))?;
+
+ let mut decimal_builder = PrimitiveBuilder::<Decimal256Type>::with_capacity(string_array.len());
+
+ for i in 0..string_array.len() {
+ if string_array.is_null(i) {
+ decimal_builder.append_null();
+ } else {
+ let str_value = string_array.value(i).trim();
+ match parse_string_to_decimal(str_value, precision, scale) {
+ Ok(Some(decimal_value)) => {
+ // Convert i128 to i256
+ let i256_value = i256::from_i128(decimal_value);
+ decimal_builder.append_value(i256_value);
+ }
+ Ok(None) => {
+ if eval_mode == EvalMode::Ansi {
+ return Err(invalid_value(
+ str_value,
+ "STRING",
+ &format!("DECIMAL({},{})", precision, scale),
+ ));
+ }
+ decimal_builder.append_null();
+ }
+ Err(e) => {
+ if eval_mode == EvalMode::Ansi {
+ return Err(e);
+ }
+ decimal_builder.append_null();
+ }
+ }
+ }
+ }
+
+ Ok(Arc::new(
+ decimal_builder
+ .with_precision_and_scale(precision, scale)?
+ .finish(),
+ ))
+}
+
+/// Validates if a string is a valid decimal similar to BigDecimal
+fn is_valid_decimal_format(s: &str) -> bool {
+ if s.is_empty() {
+ return false;
+ }
+
+ let bytes = s.as_bytes();
+ let mut idx = 0;
+ let len = bytes.len();
+
+ // Skip leading +/- signs
+ if bytes[idx] == b'+' || bytes[idx] == b'-' {
+ idx += 1;
+ if idx >= len {
+ // Sign only. Fail early
+ return false;
+ }
+ }
+
+ // Check invalid cases like "++", "+-"
+ if bytes[idx] == b'+' || bytes[idx] == b'-' {
+ return false;
+ }
+
+ // Now we need at least one digit either before or after a decimal point
+ let mut has_digit = false;
+ let mut is_decimal_point_seen = false;
+
+ while idx < len {
+ let ch = bytes[idx];
+
+ if ch.is_ascii_digit() {
+ has_digit = true;
+ idx += 1;
+ } else if ch == b'.' {
+ if is_decimal_point_seen {
+ // Multiple decimal points or decimal after exponent
+ return false;
+ }
+ is_decimal_point_seen = true;
+ idx += 1;
+ } else if ch.eq_ignore_ascii_case(&b'e') {
+ if !has_digit {
+ // Exponent without any digits before it
+ return false;
+ }
+ idx += 1;
+ // Exponent part must have optional sign followed by atleast a digit
+ if idx >= len {
+ return false;
+ }
+
+ if bytes[idx] == b'+' || bytes[idx] == b'-' {
+ idx += 1;
+ if idx >= len {
+ return false;
+ }
+ }
+
+ // Must have at least one digit in exponent
+ if !bytes[idx].is_ascii_digit() {
+ return false;
+ }
+
+ // Rest all should only be digits
+ while idx < len {
+ if !bytes[idx].is_ascii_digit() {
+ return false;
+ }
+ idx += 1;
+ }
+ break;
+ } else {
+ // Invalid character found. Fail fast
+ return false;
+ }
+ }
+ has_digit
+}
+
+/// Parse a string to decimal following Spark's behavior
+fn parse_string_to_decimal(s: &str, precision: u8, scale: i8) -> SparkResult<Option<i128>> {
+ if s.is_empty() {
+ return Ok(None);
+ }
+ // Handle special values (inf, nan, etc.)
+ if s.eq_ignore_ascii_case("inf")
+ || s.eq_ignore_ascii_case("+inf")
+ || s.eq_ignore_ascii_case("infinity")
+ || s.eq_ignore_ascii_case("+infinity")
+ || s.eq_ignore_ascii_case("-inf")
+ || s.eq_ignore_ascii_case("-infinity")
+ || s.eq_ignore_ascii_case("nan")
+ {
+ return Ok(None);
+ }
+
+ if !is_valid_decimal_format(s) {
Review Comment:
Iterating over the string twice (once for validation and once for parsing) seems expensive
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support string decimal cast [datafusion-comet]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2925:
URL: https://github.com/apache/datafusion-comet/pull/2925#issuecomment-3682949388
## [Codecov](https://app.codecov.io/gh/apache/datafusion-comet/pull/2925?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 9.99%. Comparing base ([`f09f8af`](https://app.codecov.io/gh/apache/datafusion-comet/commit/f09f8af64c6599255e116a376f4f008f2fd63b43?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`6a6c80d`](https://app.codecov.io/gh/apache/datafusion-comet/commit/6a6c80d0c66e93c8f08ba4fd7d72096049c04ae0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 785 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion-comet/pull/2925?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...scala/org/apache/comet/expressions/CometCast.scala](https://app.codecov.io/gh/apache/datafusion-comet/pull/2925?src=pr&el=tree&filepath=spark%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fcomet%2Fexpressions%2FCometCast.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3Bhcmsvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9jb21ldC9leHByZXNzaW9ucy9Db21ldENhc3Quc2NhbGE=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion-comet/pull/2925?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #2925 +/- ##
============================================
- Coverage 56.12% 9.99% -46.14%
+ Complexity 976 234 -742
============================================
Files 119 167 +48
Lines 11743 15478 +3735
Branches 2251 2542 +291
============================================
- Hits 6591 1547 -5044
- Misses 4012 13754 +9742
+ Partials 1140 177 -963
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/datafusion-comet/pull/2925?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support string decimal cast [datafusion-comet]
Posted by "coderfender (via GitHub)" <gi...@apache.org>.
coderfender commented on PR #2925:
URL: https://github.com/apache/datafusion-comet/pull/2925#issuecomment-3684062501
@andygrove the docs are already updated AFAIK
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support string decimal cast [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #2925:
URL: https://github.com/apache/datafusion-comet/pull/2925#issuecomment-3663326197
Thanks for moving this to a separate PR @coderfender. I left some initial comments. Could you run some benchmarks? I'd like to get a feel for performance.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support string decimal cast [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #2925:
URL: https://github.com/apache/datafusion-comet/pull/2925
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support string decimal cast [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #2925:
URL: https://github.com/apache/datafusion-comet/pull/2925#discussion_r2641053036
##########
spark/src/test/scala/org/apache/comet/CometCastSuite.scala:
##########
@@ -684,21 +683,88 @@ class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
+// This is to pass the first `all cast combinations are covered`
ignore("cast StringType to DecimalType(10,2)") {
Review Comment:
can this test also be enabled 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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support string decimal cast [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #2925:
URL: https://github.com/apache/datafusion-comet/pull/2925#discussion_r2625348185
##########
spark/src/test/scala/org/apache/comet/CometCastSuite.scala:
##########
@@ -684,22 +684,73 @@ class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
- ignore("cast StringType to DecimalType(10,2)") {
- // https://github.com/apache/datafusion-comet/issues/325
- val values = gen.generateStrings(dataSize, numericPattern, 8).toDF("a")
- castTest(values, DataTypes.createDecimalType(10, 2))
+ test("cast StringType to DecimalType(10,2)") {
+ // TODO fix for Spark 4.0.0
+ assume(!isSpark40Plus)
Review Comment:
What is the issue with Spark 4? Do we need to file a new issue and reference it 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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support string decimal cast [datafusion-comet]
Posted by "coderfender (via GitHub)" <gi...@apache.org>.
coderfender commented on PR #2925:
URL: https://github.com/apache/datafusion-comet/pull/2925#issuecomment-3712088019
Yeah . I had a chat with @andygrove about this and preferred to keep the cast logic to ourselves to match with Spark rather than relying on a new library
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support string decimal cast [datafusion-comet]
Posted by "coderfender (via GitHub)" <gi...@apache.org>.
coderfender commented on PR #2925:
URL: https://github.com/apache/datafusion-comet/pull/2925#issuecomment-3682866288
Benchmarks after adding further optimizations
```
================================================================================================
Running benchmark DecimalType(2,2)
================================================================================================
OpenJDK 64-Bit Server VM 17.0.16+8-LTS on Mac OS X 16.0
Apple M2 Max
Cast function to : DecimalType(2,2) , ansi mode enabled : false: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------
SQL Parquet - Spark Cast expr from STRING to : DECIMAL(2,2) , ansi mode enabled : false 106 130 14 9.4 106.3 1.0X
SQL Parquet - Comet Cast expr from STRING to : DECIMAL(2,2) , ansi mode enabled : false 94 123 36 10.7 93.5 1.1X
================================================================================================
Running benchmark DecimalType(2,2)
================================================================================================
OpenJDK 64-Bit Server VM 17.0.16+8-LTS on Mac OS X 16.0
Apple M2 Max
Cast function to : DecimalType(2,2) , ansi mode enabled : true: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
----------------------------------------------------------------------------------------------------------------------------------------------------------------------
SQL Parquet - Spark Cast expr from STRING to : DECIMAL(2,2) , ansi mode enabled : true 88 109 18 11.3 88.3 1.0X
SQL Parquet - Comet Cast expr from STRING to : DECIMAL(2,2) , ansi mode enabled : true 78 85 9 12.8 78.2 1.1X
================================================================================================
Running benchmark DecimalType(10,2)
================================================================================================
OpenJDK 64-Bit Server VM 17.0.16+8-LTS on Mac OS X 16.0
Apple M2 Max
Cast function to : DecimalType(10,2) , ansi mode enabled : false: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------------------------------------------------------
SQL Parquet - Spark Cast expr from STRING to : DECIMAL(10,2) , ansi mode enabled : false 102 132 22 9.8 101.7 1.0X
SQL Parquet - Comet Cast expr from STRING to : DECIMAL(10,2) , ansi mode enabled : false 86 104 18 11.7 85.8 1.2X
================================================================================================
Running benchmark DecimalType(10,2)
================================================================================================
OpenJDK 64-Bit Server VM 17.0.16+8-LTS on Mac OS X 16.0
Apple M2 Max
Cast function to : DecimalType(10,2) , ansi mode enabled : true: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------
SQL Parquet - Spark Cast expr from STRING to : DECIMAL(10,2) , ansi mode enabled : true 118 150 34 8.5 118.1 1.0X
SQL Parquet - Comet Cast expr from STRING to : DECIMAL(10,2) , ansi mode enabled : true 86 138 36 11.6 86.1 1.4X
================================================================================================
Running benchmark DecimalType(20,2)
================================================================================================
OpenJDK 64-Bit Server VM 17.0.16+8-LTS on Mac OS X 16.0
Apple M2 Max
Cast function to : DecimalType(20,2) , ansi mode enabled : false: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------------------------------------------------------
SQL Parquet - Spark Cast expr from STRING to : DECIMAL(20,2) , ansi mode enabled : false 158 198 40 6.3 157.7 1.0X
SQL Parquet - Comet Cast expr from STRING to : DECIMAL(20,2) , ansi mode enabled : false 146 150 4 6.8 146.2 1.1X
================================================================================================
Running benchmark DecimalType(20,2)
================================================================================================
OpenJDK 64-Bit Server VM 17.0.16+8-LTS on Mac OS X 16.0
Apple M2 Max
Cast function to : DecimalType(20,2) , ansi mode enabled : true: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------
SQL Parquet - Spark Cast expr from STRING to : DECIMAL(20,2) , ansi mode enabled : true 160 210 39 6.3 159.6 1.0X
SQL Parquet - Comet Cast expr from STRING to : DECIMAL(20,2) , ansi mode enabled : true 145 150 6 6.9 145.0 1.1X
================================================================================================
Running benchmark DecimalType(34,10)
================================================================================================
OpenJDK 64-Bit Server VM 17.0.16+8-LTS on Mac OS X 16.0
Apple M2 Max
Cast function to : DecimalType(34,10) , ansi mode enabled : false: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------
SQL Parquet - Spark Cast expr from STRING to : DECIMAL(34,10) , ansi mode enabled : false 350 365 27 2.9 350.0 1.0X
SQL Parquet - Comet Cast expr from STRING to : DECIMAL(34,10) , ansi mode enabled : false 191 203 12 5.2 191.0 1.8X
================================================================================================
Running benchmark DecimalType(34,10)
================================================================================================
OpenJDK 64-Bit Server VM 17.0.16+8-LTS on Mac OS X 16.0
Apple M2 Max
Cast function to : DecimalType(34,10) , ansi mode enabled : true: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------------------------------------------------------
SQL Parquet - Spark Cast expr from STRING to : DECIMAL(34,10) , ansi mode enabled : true 347 354 12 2.9 346.5 1.0X
SQL Parquet - Comet Cast expr from STRING to : DECIMAL(34,10) , ansi mode enabled : true 192 224 38 5.2 191.6 1.8X
```
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support string decimal cast [datafusion-comet]
Posted by "coderfender (via GitHub)" <gi...@apache.org>.
coderfender commented on PR #2925:
URL: https://github.com/apache/datafusion-comet/pull/2925#issuecomment-3663403499
Great catch @andygrove . Let me try and see if I can resolve the issue or move forward with these changes and mark it compatible
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support string decimal cast [datafusion-comet]
Posted by "coderfender (via GitHub)" <gi...@apache.org>.
coderfender commented on PR #2925:
URL: https://github.com/apache/datafusion-comet/pull/2925#issuecomment-3683891186
@andygrove please take a look whenever you get a chance. Thank you
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support string decimal cast [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #2925:
URL: https://github.com/apache/datafusion-comet/pull/2925#discussion_r2625347126
##########
native/spark-expr/src/conversion_funcs/cast.rs:
##########
@@ -1976,6 +1975,363 @@ fn do_cast_string_to_int<
Ok(Some(result))
}
+fn cast_string_to_decimal(
+ array: &ArrayRef,
+ to_type: &DataType,
+ precision: &u8,
+ scale: &i8,
+ eval_mode: EvalMode,
+) -> SparkResult<ArrayRef> {
+ match to_type {
+ DataType::Decimal128(_, _) => {
+ cast_string_to_decimal128_impl(array, eval_mode, *precision, *scale)
+ }
+ DataType::Decimal256(_, _) => {
+ cast_string_to_decimal256_impl(array, eval_mode, *precision, *scale)
+ }
+ _ => Err(SparkError::Internal(format!(
+ "Unexpected type in cast_string_to_decimal: {:?}",
+ to_type
+ ))),
+ }
+}
+
+fn cast_string_to_decimal128_impl(
+ array: &ArrayRef,
+ eval_mode: EvalMode,
+ precision: u8,
+ scale: i8,
+) -> SparkResult<ArrayRef> {
+ let string_array = array
+ .as_any()
+ .downcast_ref::<StringArray>()
+ .ok_or_else(|| SparkError::Internal("Expected string array".to_string()))?;
+
+ let mut decimal_builder = Decimal128Builder::with_capacity(string_array.len());
+
+ for i in 0..string_array.len() {
+ if string_array.is_null(i) {
+ decimal_builder.append_null();
+ } else {
+ let str_value = string_array.value(i).trim();
+ match parse_string_to_decimal(str_value, precision, scale) {
+ Ok(Some(decimal_value)) => {
+ decimal_builder.append_value(decimal_value);
+ }
+ Ok(None) => {
+ if eval_mode == EvalMode::Ansi {
+ return Err(invalid_value(
+ string_array.value(i),
+ "STRING",
+ &format!("DECIMAL({},{})", precision, scale),
+ ));
+ }
+ decimal_builder.append_null();
+ }
+ Err(e) => {
+ if eval_mode == EvalMode::Ansi {
+ return Err(e);
+ }
+ decimal_builder.append_null();
+ }
+ }
+ }
+ }
+
+ Ok(Arc::new(
+ decimal_builder
+ .with_precision_and_scale(precision, scale)?
+ .finish(),
+ ))
+}
+
+fn cast_string_to_decimal256_impl(
+ array: &ArrayRef,
+ eval_mode: EvalMode,
+ precision: u8,
+ scale: i8,
+) -> SparkResult<ArrayRef> {
+ let string_array = array
+ .as_any()
+ .downcast_ref::<StringArray>()
+ .ok_or_else(|| SparkError::Internal("Expected string array".to_string()))?;
+
+ let mut decimal_builder = PrimitiveBuilder::<Decimal256Type>::with_capacity(string_array.len());
+
+ for i in 0..string_array.len() {
+ if string_array.is_null(i) {
+ decimal_builder.append_null();
+ } else {
+ let str_value = string_array.value(i).trim();
+ match parse_string_to_decimal(str_value, precision, scale) {
+ Ok(Some(decimal_value)) => {
+ // Convert i128 to i256
+ let i256_value = i256::from_i128(decimal_value);
+ decimal_builder.append_value(i256_value);
+ }
+ Ok(None) => {
+ if eval_mode == EvalMode::Ansi {
+ return Err(invalid_value(
+ str_value,
+ "STRING",
+ &format!("DECIMAL({},{})", precision, scale),
+ ));
+ }
+ decimal_builder.append_null();
+ }
+ Err(e) => {
+ if eval_mode == EvalMode::Ansi {
+ return Err(e);
+ }
+ decimal_builder.append_null();
+ }
+ }
+ }
+ }
+
+ Ok(Arc::new(
+ decimal_builder
+ .with_precision_and_scale(precision, scale)?
+ .finish(),
+ ))
+}
+
+/// Validates if a string is a valid decimal similar to BigDecimal
+fn is_valid_decimal_format(s: &str) -> bool {
+ if s.is_empty() {
+ return false;
+ }
+
+ let bytes = s.as_bytes();
+ let mut idx = 0;
+ let len = bytes.len();
+
+ // Skip leading +/- signs
+ if bytes[idx] == b'+' || bytes[idx] == b'-' {
+ idx += 1;
+ if idx >= len {
+ // Sign only. Fail early
+ return false;
+ }
+ }
+
+ // Check invalid cases like "++", "+-"
+ if bytes[idx] == b'+' || bytes[idx] == b'-' {
+ return false;
+ }
+
+ // Now we need at least one digit either before or after a decimal point
+ let mut has_digit = false;
+ let mut is_decimal_point_seen = false;
+
+ while idx < len {
+ let ch = bytes[idx];
+
+ if ch.is_ascii_digit() {
+ has_digit = true;
+ idx += 1;
+ } else if ch == b'.' {
+ if is_decimal_point_seen {
+ // Multiple decimal points or decimal after exponent
+ return false;
+ }
+ is_decimal_point_seen = true;
+ idx += 1;
+ } else if ch.eq_ignore_ascii_case(&b'e') {
+ if !has_digit {
+ // Exponent without any digits before it
+ return false;
+ }
+ idx += 1;
+ // Exponent part must have optional sign followed by atleast a digit
+ if idx >= len {
+ return false;
+ }
+
+ if bytes[idx] == b'+' || bytes[idx] == b'-' {
+ idx += 1;
+ if idx >= len {
+ return false;
+ }
+ }
+
+ // Must have at least one digit in exponent
+ if !bytes[idx].is_ascii_digit() {
+ return false;
+ }
+
+ // Rest all should only be digits
+ while idx < len {
+ if !bytes[idx].is_ascii_digit() {
+ return false;
+ }
+ idx += 1;
+ }
+ break;
+ } else {
+ // Invalid character found. Fail fast
+ return false;
+ }
+ }
+ has_digit
+}
+
+/// Parse a string to decimal following Spark's behavior
+fn parse_string_to_decimal(s: &str, precision: u8, scale: i8) -> SparkResult<Option<i128>> {
+ if s.is_empty() {
+ return Ok(None);
+ }
+ // Handle special values (inf, nan, etc.)
+ if s.eq_ignore_ascii_case("inf")
+ || s.eq_ignore_ascii_case("+inf")
+ || s.eq_ignore_ascii_case("infinity")
+ || s.eq_ignore_ascii_case("+infinity")
+ || s.eq_ignore_ascii_case("-inf")
+ || s.eq_ignore_ascii_case("-infinity")
+ || s.eq_ignore_ascii_case("nan")
+ {
+ return Ok(None);
+ }
+
+ if !is_valid_decimal_format(s) {
+ return Ok(None);
+ }
+
+ match parse_decimal_str(s) {
+ Ok((mantissa, exponent)) => {
+ // Convert to target scale
+ let target_scale = scale as i32;
+ let scale_adjustment = target_scale - exponent;
+
+ let scaled_value = if scale_adjustment >= 0 {
+ // Need to multiply (increase scale) but return None if scale is too high to fit i128
+ if scale_adjustment > 38 {
+ return Ok(None);
+ }
+ mantissa.checked_mul(10_i128.pow(scale_adjustment as u32))
+ } else {
+ // Need to multiply (increase scale) but return None if scale is too high to fit i128
+ let abs_scale_adjustment = (-scale_adjustment) as u32;
+ if abs_scale_adjustment > 38 {
+ return Ok(Some(0));
+ }
+
+ let divisor = 10_i128.pow(abs_scale_adjustment);
+ let quotient_opt = mantissa.checked_div(divisor);
+ // Check if divisor is 0
+ if quotient_opt.is_none() {
+ return Ok(None);
+ }
+ let quotient = quotient_opt.unwrap();
+ let remainder = mantissa % divisor;
+
+ // Round half up: if abs(remainder) >= divisor/2, round away from zero
+ let half_divisor = divisor / 2;
+ let rounded = if remainder.abs() >= half_divisor {
+ if mantissa >= 0 {
+ quotient + 1
+ } else {
+ quotient - 1
+ }
+ } else {
+ quotient
+ };
+ Some(rounded)
+ };
+
+ match scaled_value {
+ Some(value) => {
+ // Check if it fits target precision
+ if is_validate_decimal_precision(value, precision) {
+ Ok(Some(value))
+ } else {
+ Ok(None)
+ }
+ }
+ None => {
+ // Overflow while scaling
+ Ok(None)
+ }
+ }
+ }
+ Err(_) => Ok(None),
+ }
+}
+
+/// Parse a decimal string into mantissa and scale
+/// e.g., "123.45" -> (12345, 2), "-0.001" -> (-1, 3)
+fn parse_decimal_str(s: &str) -> Result<(i128, i32), String> {
+ let s = s.trim();
+ if s.is_empty() {
+ return Err("Empty string".to_string());
+ }
+
+ let (mantissa_str, exponent) = if let Some(e_pos) = s.find(|c| ['e', 'E'].contains(&c)) {
+ let mantissa_part = &s[..e_pos];
+ let exponent_part = &s[e_pos + 1..];
+ // Parse exponent
+ let exp: i32 = exponent_part
+ .parse()
+ .map_err(|e| format!("Invalid exponent: {}", e))?;
+
+ (mantissa_part, exp)
+ } else {
+ (s, 0)
+ };
+
+ let negative = mantissa_str.starts_with('-');
+ let mantissa_str = if negative || mantissa_str.starts_with('+') {
+ &mantissa_str[1..]
+ } else {
+ mantissa_str
+ };
+
+ let split_by_dot: Vec<&str> = mantissa_str.split('.').collect();
Review Comment:
Calling `split` seems expensive 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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support string decimal cast [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #2925:
URL: https://github.com/apache/datafusion-comet/pull/2925#discussion_r2625343838
##########
native/spark-expr/src/conversion_funcs/cast.rs:
##########
@@ -1976,6 +1975,363 @@ fn do_cast_string_to_int<
Ok(Some(result))
}
+fn cast_string_to_decimal(
+ array: &ArrayRef,
+ to_type: &DataType,
+ precision: &u8,
+ scale: &i8,
+ eval_mode: EvalMode,
+) -> SparkResult<ArrayRef> {
+ match to_type {
+ DataType::Decimal128(_, _) => {
+ cast_string_to_decimal128_impl(array, eval_mode, *precision, *scale)
+ }
+ DataType::Decimal256(_, _) => {
+ cast_string_to_decimal256_impl(array, eval_mode, *precision, *scale)
+ }
+ _ => Err(SparkError::Internal(format!(
+ "Unexpected type in cast_string_to_decimal: {:?}",
+ to_type
+ ))),
+ }
+}
+
+fn cast_string_to_decimal128_impl(
+ array: &ArrayRef,
+ eval_mode: EvalMode,
+ precision: u8,
+ scale: i8,
+) -> SparkResult<ArrayRef> {
+ let string_array = array
+ .as_any()
+ .downcast_ref::<StringArray>()
+ .ok_or_else(|| SparkError::Internal("Expected string array".to_string()))?;
+
+ let mut decimal_builder = Decimal128Builder::with_capacity(string_array.len());
+
+ for i in 0..string_array.len() {
+ if string_array.is_null(i) {
+ decimal_builder.append_null();
+ } else {
+ let str_value = string_array.value(i).trim();
Review Comment:
It would be better to avoid `trim` since this could be expensive (creating a new string). It would be more efficient for the parser to ignore whitespace at the end of this 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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support string decimal cast [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on PR #2925:
URL: https://github.com/apache/datafusion-comet/pull/2925#issuecomment-3712073922
Late comment, but did rust_decimal not work for us?
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org