Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 13 additions & 1 deletion native/core/src/execution/columnar_to_row.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1068,7 +1068,19 @@ impl ColumnarToRowContext {
})?;
Ok(Arc::new(decimal_array))
}
_ => Ok(Arc::clone(array)),
_ => {
// For any other type mismatch, attempt an Arrow cast.
// This handles cases like Int32 → Date32 (which can happen when Spark
// generates default column values using the physical storage type rather
// than the logical type).
let options = CastOptions::default();
Copy link
Contributor

Choose a reason for hiding this comment

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

this might be expensive to create each time, especially the formatter factory?
can it be const?

Copy link
Member Author

Choose a reason for hiding this comment

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

Claude says that this is cheap and that there are no heap allocations.

CastOptions::default() is just setting a bool and a struct of Option::None fields on the stack. There are no heap allocations. The actual cost is entirely in cast_with_options itself (which processes the array data). Creating the options struct is negligible.

Copy link
Contributor

Choose a reason for hiding this comment

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

cast_with_options(array, schema_type, &options).map_err(|e| {
CometError::Internal(format!(
"Failed to cast array from {:?} to {:?}: {}",
actual_type, schema_type, e
))
})
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -232,11 +232,6 @@ case class CometNativeColumnarToRowExec(child: SparkPlan)

object CometNativeColumnarToRowExec {

/**
* Checks if native columnar to row conversion is enabled.
*/
def isEnabled: Boolean = CometConf.COMET_NATIVE_COLUMNAR_TO_ROW_ENABLED.get()
Copy link
Member Author

Choose a reason for hiding this comment

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

This was unused


/**
* Checks if the given schema is supported by native columnar to row conversion.
*
Expand Down
16 changes: 9 additions & 7 deletions spark/src/test/scala/org/apache/comet/CometFuzzTestBase.scala
Original file line number Diff line number Diff line change
Expand Up @@ -111,14 +111,16 @@ class CometFuzzTestBase extends CometTestBase with AdaptiveSparkPlanHelper {

override protected def test(testName: String, testTags: Tag*)(testFun: => Any)(implicit
pos: Position): Unit = {
Seq("native", "jvm").foreach { shuffleMode =>
super.test(testName + s" ($shuffleMode shuffle)", testTags: _*) {
withSQLConf(
CometConf.COMET_PARQUET_UNSIGNED_SMALL_INT_CHECK.key -> "false",
CometConf.COMET_SHUFFLE_MODE.key -> shuffleMode) {
testFun
Seq(("native", "false"), ("jvm", "true"), ("jvm", "false")).foreach {
case (shuffleMode, nativeC2R) =>
super.test(testName + s" ($shuffleMode shuffle, nativeC2R=$nativeC2R)", testTags: _*) {
withSQLConf(
CometConf.COMET_NATIVE_COLUMNAR_TO_ROW_ENABLED.key -> nativeC2R,
CometConf.COMET_PARQUET_UNSIGNED_SMALL_INT_CHECK.key -> "false",
CometConf.COMET_SHUFFLE_MODE.key -> shuffleMode) {
testFun
}
}
}
}
}

Expand Down
Loading