Skip to content
Open
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
4 changes: 4 additions & 0 deletions native/spark-expr/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -139,3 +139,7 @@ harness = false
[[bench]]
name = "to_json"
harness = false

[[bench]]
name = "cast_string_to_boolean"
harness = false
95 changes: 95 additions & 0 deletions native/spark-expr/benches/cast_string_to_boolean.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
// Licensed to the Apache Software Foundation (ASF) under one
// or more contributor license agreements. See the NOTICE file
// distributed with this work for additional information
// regarding copyright ownership. The ASF licenses this file
// to you under the Apache License, Version 2.0 (the
// "License"); you may not use this file except in compliance
// with the License. You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing,
// software distributed under the License is distributed on an
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.

use arrow::array::{builder::StringBuilder, ArrayRef};
use arrow::datatypes::DataType;
use criterion::{criterion_group, criterion_main, Criterion};
use datafusion::physical_plan::ColumnarValue;
use datafusion_comet_spark_expr::{spark_cast, EvalMode, SparkCastOptions};
use std::sync::Arc;

const NUM_ROWS: usize = 8192;

/// Strings that Spark accepts as booleans, in the mixed casing and padding that shows
/// up in real data.
const VALID: [&str; 12] = [
"true", "TRUE", "True", "false", "FALSE", " t ", "y", "YES", "no", "N", "1", "0",
];

/// A mix of valid boolean spellings and values that cast to NULL.
fn mixed_batch() -> ArrayRef {
let mut b = StringBuilder::new();
for i in 0..NUM_ROWS {
match i % 10 {
0 => b.append_null(),
1 => b.append_value("maybe"),
2 => b.append_value("2"),
_ => b.append_value(VALID[i % VALID.len()]),
}
}
Arc::new(b.finish())
}

/// Only valid boolean spellings, so every row takes the parsing path.
fn valid_batch() -> ArrayRef {
let mut b = StringBuilder::new();
for i in 0..NUM_ROWS {
b.append_value(VALID[i % VALID.len()]);
}
Arc::new(b.finish())
}

fn criterion_benchmark(c: &mut Criterion) {
let mixed = mixed_batch();
let valid = valid_batch();

let mut group = c.benchmark_group("cast_string_to_boolean");
for (mode, mode_name) in [
(EvalMode::Legacy, "legacy"),
(EvalMode::Try, "try"),
(EvalMode::Ansi, "ansi"),
] {
let options = SparkCastOptions::new(mode, "", false);
// ANSI mode errors on any unparseable value, so it only gets the valid batch.
if mode != EvalMode::Ansi {
group.bench_function(format!("{mode_name}/mixed"), |b| {
b.iter(|| {
spark_cast(
ColumnarValue::Array(Arc::clone(&mixed)),
&DataType::Boolean,
&options,
)
.unwrap()
});
});
}
group.bench_function(format!("{mode_name}/valid"), |b| {
b.iter(|| {
spark_cast(
ColumnarValue::Array(Arc::clone(&valid)),
&DataType::Boolean,
&options,
)
.unwrap()
});
});
}
group.finish();
}

criterion_group!(benches, criterion_benchmark);
criterion_main!(benches);
92 changes: 75 additions & 17 deletions native/spark-expr/src/conversion_funcs/string.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@

use crate::{timezone, EvalMode, SparkError, SparkResult};
use arrow::array::{
Array, ArrayRef, ArrowPrimitiveType, BooleanArray, Decimal128Builder, GenericStringArray,
OffsetSizeTrait, PrimitiveArray, PrimitiveBuilder, StringArray,
Array, ArrayRef, ArrowPrimitiveType, BooleanArray, BooleanBuilder, Decimal128Builder,
GenericStringArray, OffsetSizeTrait, PrimitiveArray, PrimitiveBuilder, StringArray,
};
use arrow::compute::DecimalCast;
use arrow::datatypes::{
Expand Down Expand Up @@ -254,6 +254,28 @@ where
pruned_float_str.parse::<F>().ok()
}

/// Parse the boolean literals Spark accepts in a string-to-boolean cast, returning
/// `None` for anything else.
///
/// Trimming before comparing is equivalent to trimming afterwards: ASCII case folding
/// never turns a whitespace character into a non-whitespace one, or vice versa.
#[inline]
fn parse_utf8_to_boolean(value: &str) -> Option<bool> {
let trimmed = value.trim();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

string.rs:264 uses Rust str::trim (Unicode char::is_whitespace). Spark's UTF8String.trimAll() (UTF8String.java:994-1010) trims every byte where Character.isWhitespace(b) || Character.isISOControl(b) (UTF8String.java:201-203). These sets differ on the ASCII control bytes 0x00-0x08, 0x0E-0x1F, and 0x7F: Spark trims them, Rust does not. I confirmed the full set with a byte-by-byte comparison of char::is_whitespace against Java's isWhitespace || isISOControl.

Concretely, " \x01true\x01 " casts to true in Spark but to null (or ANSI-errors) in Comet. This is pre-existing: the old .to_ascii_lowercase().trim() had the same behavior, so this PR is not a regression. It is out of scope to fix here, but the PR adds a fresh test_parse_utf8_to_boolean that tests whitespace trimming (string.rs, " \ttrue\n ") without touching control chars, so it silently locks in the divergence.

The existing integration test at CometCastSuite.scala:806-811 uses whitespaceChars = " \t\r\n" (CometCastSuite.scala:61), all of which agree between Rust and Spark, so nothing catches the control-char gap.

Suggested change: leave the trimming as-is for this PR to keep it a pure perf change, and file a follow-up issue for trimAll parity. Do not add a passing assertion like assert_eq!(parse_utf8_to_boolean("\x01true\x01"), None) to the new test, since that would enshrine incorrect behavior as expected. If you want a marker, add a // TODO(#issue): Spark trimAll also strips ISO control bytes comment near the value.trim() call.

match trimmed.len() {
1 => match trimmed.as_bytes()[0] {
b't' | b'T' | b'y' | b'Y' | b'1' => Some(true),
b'f' | b'F' | b'n' | b'N' | b'0' => Some(false),
_ => None,
},
2 if trimmed.eq_ignore_ascii_case("no") => Some(false),
3 if trimmed.eq_ignore_ascii_case("yes") => Some(true),
4 if trimmed.eq_ignore_ascii_case("true") => Some(true),
5 if trimmed.eq_ignore_ascii_case("false") => Some(false),
_ => None,
}
}

pub(crate) fn spark_cast_utf8_to_boolean<OffsetSize>(
from: &dyn Array,
eval_mode: EvalMode,
Expand All @@ -266,22 +288,26 @@ where
.downcast_ref::<GenericStringArray<OffsetSize>>()
.unwrap();

let output_array = array
// Only ANSI mode can fail, so the other modes avoid the per-row `Result` entirely.
if eval_mode == EvalMode::Ansi {
let mut builder = BooleanBuilder::with_capacity(array.len());
for value in array.iter() {
match value {
Some(value) => match parse_utf8_to_boolean(value) {
Some(parsed) => builder.append_value(parsed),
None => return Err(invalid_value(value, "STRING", "BOOLEAN")),
},
None => builder.append_null(),
}
}

return Ok(Arc::new(builder.finish()));
}

let output_array: BooleanArray = array
.iter()
.map(|value| match value {
Some(value) => match value.to_ascii_lowercase().trim() {
"t" | "true" | "y" | "yes" | "1" => Ok(Some(true)),
"f" | "false" | "n" | "no" | "0" => Ok(Some(false)),
_ if eval_mode == EvalMode::Ansi => Err(SparkError::CastInvalidValue {
value: value.to_string(),
from_type: "STRING".to_string(),
to_type: "BOOLEAN".to_string(),
}),
_ => Ok(None),
},
_ => Ok(None),
})
.collect::<Result<BooleanArray, _>>()?;
.map(|value| value.and_then(parse_utf8_to_boolean))
.collect();

Ok(Arc::new(output_array))
}
Expand Down Expand Up @@ -1989,6 +2015,38 @@ mod tests {
}
}

#[test]
fn test_parse_utf8_to_boolean() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

test_parse_utf8_to_boolean (string.rs, invalid list) exercises "úñî" (6 bytes, falls to the _ arm) and "true" (fullwidth, 15 bytes, _ arm). Neither hits a 2-to-5 byte arm where eq_ignore_ascii_case does the real work against a non-ASCII input of the exact target length. That path is the one most likely to regress if someone later swaps the comparison for something byte-naive.

Suggested change: add a 2-byte non-ASCII value that must not match "no", for example assert_eq!(parse_utf8_to_boolean("ñ"), None) ("ñ" is 2 bytes), and a 4-byte one for the "true" arm such as assert_eq!(parse_utf8_to_boolean("a\u{0300}bc"), None) or simply "tру"-style mixed bytes summing to 4.

for truthy in [
"t", "T", "true", "TRUE", "tRuE", "y", "Y", "yes", "YES", "1",
] {
assert_eq!(parse_utf8_to_boolean(truthy), Some(true), "{truthy}");
}
for falsy in [
"f", "F", "false", "FALSE", "fAlSe", "n", "N", "no", "NO", "0",
] {
assert_eq!(parse_utf8_to_boolean(falsy), Some(false), "{falsy}");
}
// Surrounding whitespace is ignored, as in Spark.
assert_eq!(parse_utf8_to_boolean(" \ttrue\n "), Some(true));
assert_eq!(parse_utf8_to_boolean(" no "), Some(false));
// Anything else casts to NULL.
for invalid in [
"",
" ",
"tru",
"truely",
"2",
"-1",
"on",
"off",
"úñî",
"true",
] {
assert_eq!(parse_utf8_to_boolean(invalid), None, "{invalid}");
}
}

#[test]
#[cfg_attr(miri, ignore)] // test takes too long with miri
fn test_cast_string_to_timestamp() {
Expand Down
Loading