Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: enable full decimal to decimal support #1385

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix: enable full decimal to decimal support #1385

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a2689a3
enable decimal to decimal and enable castoptions to be passed
himadripal Feb 6, 2025
99727b8
make decimal precision error check hard coded common in test for all …
himadripal Feb 11, 2025
42648f4
review comments and fix failing test.
himadripal Feb 14, 2025
4df3e68
review comments
himadripal Feb 16, 2025
5cce0c4
add special handling for decimal in doc generation, fix the assert as…
himadripal Feb 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion native/spark-expr/src/conversion_funcs/cast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -872,6 +872,13 @@ fn cast_array(
let array = array_with_timezone(array, cast_options.timezone.clone(), Some(to_type))?;
let from_type = array.data_type().clone();

let native_cast_options: CastOptions = CastOptions {
safe: !matches!(cast_options.eval_mode, EvalMode::Ansi), // take safe mode from cast_options passed
format_options: FormatOptions::new()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think one can use a default value defined for FormatOptions here

Copy link
Contributor Author

@himadripal himadripal Feb 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the default CAST_OPTIONS which is replaced by this native_cast_options had two these set to

static TIMESTAMP_FORMAT: Option<&str> = Some("%Y-%m-%d %H:%M:%S%.f");
           
 timestamp_format: TIMESTAMP_FORMAT,
 timestamp_tz_format: TIMESTAMP_FORMAT,

If we change it to default, I checked FormatOptions::default() implementation set these

            timestamp_format: None,
            timestamp_tz_format: None,

Hence kept it as it is defined inside default CAST_OPTIONS for comet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. (The format options are used only to make the cast of timestamp to string compatible with Spark, and are not needed anywhere else) but I guess it is a good idea to be consistent everywhere.

.with_timestamp_tz_format(TIMESTAMP_FORMAT)
.with_timestamp_format(TIMESTAMP_FORMAT),
};

let array = match &from_type {
Dictionary(key_type, value_type)
if key_type.as_ref() == &Int32
Expand Down Expand Up @@ -963,7 +970,7 @@ fn cast_array(
|| is_datafusion_spark_compatible(from_type, to_type, cast_options.allow_incompat) =>
{
// use DataFusion cast only when we know that it is compatible with Spark
Ok(cast_with_options(&array, to_type, &CAST_OPTIONS)?)
Ok(cast_with_options(&array, to_type, &native_cast_options)?)
}
_ => {
// we should never reach this code because the Scala code should be checking
Expand Down
9 changes: 2 additions & 7 deletions spark/src/main/scala/org/apache/comet/expressions/CometCast.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,8 @@ object CometCast {
case _ =>
Unsupported
}
case (from: DecimalType, to: DecimalType) =>
if (to.precision < from.precision) {
// https://github.com/apache/datafusion/issues/13492
Incompatible(Some("Casting to smaller precision is not supported"))
} else {
Compatible()
}
case (_: DecimalType, _: DecimalType) =>
Compatible()
case (DataTypes.StringType, _) =>
canCastFromString(toType, timeZoneId, evalMode)
case (_, DataTypes.StringType) =>
Expand Down
17 changes: 13 additions & 4 deletions spark/src/test/scala/org/apache/comet/CometCastSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -913,8 +913,8 @@ class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper {
val values = Seq(BigDecimal("12345.6789"), BigDecimal("9876.5432"), BigDecimal("123.4567"))
val df = withNulls(values)
.toDF("b")
.withColumn("a", col("b").cast(DecimalType(6, 2)))
checkSparkAnswer(df)
.withColumn("a", col("b").cast(DecimalType(38, 28)))
castTest(df, DecimalType(6, 2))
}

test("cast between decimals with higher precision than source") {
Expand Down Expand Up @@ -1133,8 +1133,17 @@ class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper {
.replace(".WITH_SUGGESTION] ", "]")
.startsWith(cometMessage))
} else if (CometSparkSessionExtensions.isSpark34Plus) {
// for Spark 3.4 we expect to reproduce the error message exactly
assert(cometMessage == sparkMessage)
// for comet decimal conversion throws ArrowError(string) from arrow
if (cometMessage.contains("Invalid argument error")) {
val regex =
"\\[\\[?(NUMERIC_VALUE_OUT_OF_RANGE|Invalid argument error)\\]?:? .*? (\\d+(\\.\\d+)?) .*? Decimal\\(?(\\d+),?\\s?(\\d+)\\)?.*?\\]?"
assert(cometMessage.matches(regex) == sparkMessage.matches(regex))

} else {
// for Spark 3.4 we expect to reproduce the error message exactly
assert(cometMessage == sparkMessage)

}
} else {
// for Spark 3.3 we just need to strip the prefix from the Comet message
// before comparing
Expand Down
Loading