Skip to content

Commit 1772097

Browse files
authored
feat: enable debug assertions in CI profile, fix unaligned memory access bug (#3652)
1 parent f57e54a commit 1772097

File tree

12 files changed

+41
-10
lines changed

12 files changed

+41
-10
lines changed

.github/workflows/iceberg_spark_test.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ on:
4848

4949
env:
5050
RUST_VERSION: stable
51+
RUST_BACKTRACE: 1
5152

5253
jobs:
5354
# Build native library once and share with all test jobs

.github/workflows/pr_benchmark_check.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ on:
4141

4242
env:
4343
RUST_VERSION: stable
44+
RUST_BACKTRACE: 1
4445

4546
jobs:
4647
benchmark-check:

.github/workflows/pr_build_linux.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ on:
4848

4949
env:
5050
RUST_VERSION: stable
51+
RUST_BACKTRACE: 1
5152

5253
jobs:
5354

.github/workflows/pr_build_macos.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ on:
4848

4949
env:
5050
RUST_VERSION: stable
51+
RUST_BACKTRACE: 1
5152

5253
jobs:
5354

.github/workflows/spark_sql_test.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ on:
5454

5555
env:
5656
RUST_VERSION: stable
57+
RUST_BACKTRACE: 1
5758

5859
jobs:
5960

.github/workflows/spark_sql_test_native_datafusion.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ on:
2828

2929
env:
3030
RUST_VERSION: stable
31+
RUST_BACKTRACE: 1
3132

3233
jobs:
3334
spark-sql-catalyst-native-datafusion:

.github/workflows/spark_sql_test_native_iceberg_compat.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ on:
2828

2929
env:
3030
RUST_VERSION: stable
31+
RUST_BACKTRACE: 1
3132

3233
jobs:
3334
spark-sql-catalyst-native-iceberg-compat:

native/Cargo.toml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,4 +71,6 @@ strip = "debuginfo"
7171
inherits = "release"
7272
lto = false # Skip LTO for faster linking
7373
codegen-units = 16 # Parallel codegen (faster compile, slightly larger binary)
74+
debug-assertions = true
75+
panic = "unwind" # Allow panics to be caught and logged across FFI boundary
7476
# overflow-checks inherited as false from release

native/core/src/errors.rs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,10 @@ pub enum CometError {
145145
}
146146

147147
pub fn init() {
148-
std::panic::set_hook(Box::new(|_panic_info| {
148+
std::panic::set_hook(Box::new(|panic_info| {
149+
// Log the panic message and location to stderr so it is visible in CI logs
150+
// even if the exception message is lost crossing the FFI boundary
151+
eprintln!("Comet native panic: {panic_info}");
149152
// Capture the backtrace for a panic
150153
*PANIC_BACKTRACE.lock().unwrap() =
151154
Some(std::backtrace::Backtrace::force_capture().to_string());

native/core/src/execution/jni_api.rs

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ use crate::{
2929
use arrow::array::{Array, RecordBatch, UInt32Array};
3030
use arrow::compute::{take, TakeOptions};
3131
use arrow::datatypes::DataType as ArrowDataType;
32-
use datafusion::common::{Result as DataFusionResult, ScalarValue};
32+
use datafusion::common::{DataFusionError, Result as DataFusionResult, ScalarValue};
3333
use datafusion::execution::disk_manager::DiskManagerMode;
3434
use datafusion::execution::memory_pool::MemoryPool;
3535
use datafusion::execution::runtime_env::RuntimeEnvBuilder;
@@ -59,6 +59,7 @@ use datafusion_spark::function::string::concat::SparkConcat;
5959
use datafusion_spark::function::string::space::SparkSpace;
6060
use futures::poll;
6161
use futures::stream::StreamExt;
62+
use futures::FutureExt;
6263
use jni::objects::JByteBuffer;
6364
use jni::sys::{jlongArray, JNI_FALSE};
6465
use jni::{
@@ -570,10 +571,29 @@ pub unsafe extern "system" fn Java_org_apache_comet_Native_executePlan(
570571
let (tx, rx) = mpsc::channel(2);
571572
let mut stream = stream;
572573
get_runtime().spawn(async move {
573-
while let Some(batch) = stream.next().await {
574-
if tx.send(batch).await.is_err() {
575-
break;
574+
let result = std::panic::AssertUnwindSafe(async {
575+
while let Some(batch) = stream.next().await {
576+
if tx.send(batch).await.is_err() {
577+
break;
578+
}
576579
}
580+
})
581+
.catch_unwind()
582+
.await;
583+
584+
if let Err(panic) = result {
585+
let msg = match panic.downcast_ref::<&str>() {
586+
Some(s) => s.to_string(),
587+
None => match panic.downcast_ref::<String>() {
588+
Some(s) => s.clone(),
589+
None => "unknown panic".to_string(),
590+
},
591+
};
592+
let _ = tx
593+
.send(Err(DataFusionError::Execution(format!(
594+
"native panic: {msg}"
595+
))))
596+
.await;
577597
}
578598
});
579599
exec_context.batch_receiver = Some(rx);

0 commit comments

Comments
 (0)