You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2024/04/23 15:17:04 UTC
[PR] feat: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
andygrove opened a new pull request, #307:
URL: https://github.com/apache/datafusion-comet/pull/307
## 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.
-->
Part of https://github.com/apache/datafusion-comet/issues/286
## 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.
-->
Improve compatibility with Apache Spark
## 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.
-->
Add custom implementation of CAST from string to integral rather than delegate to DataFusion
## 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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1576817617
##########
spark/src/test/scala/org/apache/comet/CometCastSuite.scala:
##########
@@ -66,19 +66,22 @@ class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper {
castTest(testValues, DataTypes.BooleanType)
}
- ignore("cast string to byte") {
- castTest(generateStrings(numericPattern, 8).toDF("a"), DataTypes.ByteType)
+ test("cast string to byte") {
+ val testValues =
+ Seq("", ".", "0", "-0", "+1", "-1", ".2", "-.2", "1e1", "127", "128", "-128", "-129") ++
+ generateStrings(numericPattern, 8)
+ castTest(testValues.toDF("a"), DataTypes.ByteType)
}
- ignore("cast string to short") {
+ test("cast string to short") {
Review Comment:
We should probably have some negative tests with invalid strings.
Also, curious, what does `cast(".")` yield?
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1581637504
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -142,6 +226,311 @@ impl Cast {
}
}
+fn cast_string_to_i8(str: &str, eval_mode: EvalMode) -> CometResult<Option<i8>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "TINYINT",
+ i8::MIN as i32,
+ i8::MAX as i32,
+ )?
+ .map(|v| v as i8))
+}
+
+fn cast_string_to_i16(str: &str, eval_mode: EvalMode) -> CometResult<Option<i16>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "SMALLINT",
+ i16::MIN as i32,
+ i16::MAX as i32,
+ )?
+ .map(|v| v as i16))
+}
+
+fn cast_string_to_i32(str: &str, eval_mode: EvalMode) -> CometResult<Option<i32>> {
+ let mut accum = CastStringToInt32::default();
+ do_cast_string_to_int(&mut accum, str, eval_mode, "INT")?;
+ Ok(accum.result)
+}
+
+fn cast_string_to_i64(str: &str, eval_mode: EvalMode) -> CometResult<Option<i64>> {
+ let mut accum = CastStringToInt64::default();
+ do_cast_string_to_int(&mut accum, str, eval_mode, "BIGINT")?;
+ Ok(accum.result)
+}
+
+fn cast_string_to_int_with_range_check(
+ str: &str,
+ eval_mode: EvalMode,
+ type_name: &str,
+ min: i32,
+ max: i32,
+) -> CometResult<Option<i32>> {
+ let mut accum = CastStringToInt32::default();
Review Comment:
I don't this should cause any heap allocation at all. I would expect this to be allocated on the stack which shouldn't be expensive. But a benchmark is always a more convincing argument.
##########
spark/src/test/scala/org/apache/comet/CometCastSuite.scala:
##########
@@ -66,19 +66,22 @@ class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper {
castTest(testValues, DataTypes.BooleanType)
}
- ignore("cast string to byte") {
- castTest(generateStrings(numericPattern, 8).toDF("a"), DataTypes.ByteType)
+ test("cast string to byte") {
+ val testValues =
+ Seq("", ".", "0", "-0", "+1", "-1", ".2", "-.2", "1e1", "127", "128", "-128", "-129") ++
+ generateStrings(numericPattern, 8)
+ castTest(testValues.toDF("a"), DataTypes.ByteType)
}
- ignore("cast string to short") {
+ test("cast string to short") {
Review Comment:
+1 for these test btw
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1583667605
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -103,10 +125,76 @@ impl Cast {
(DataType::LargeUtf8, DataType::Boolean) => {
Self::spark_cast_utf8_to_boolean::<i64>(&array, self.eval_mode)?
}
- _ => cast_with_options(&array, to_type, &CAST_OPTIONS)?,
+ (
+ DataType::Utf8,
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) => Self::cast_string_to_int::<i32>(to_type, &array, self.eval_mode)?,
+ (
+ DataType::LargeUtf8,
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) => Self::cast_string_to_int::<i64>(to_type, &array, self.eval_mode)?,
+ (
+ DataType::Dictionary(key_type, value_type),
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) if key_type.as_ref() == &DataType::Int32
+ && (value_type.as_ref() == &DataType::Utf8
+ || value_type.as_ref() == &DataType::LargeUtf8) =>
+ {
+ // TODO: we are unpacking a dictionary-encoded array and then performing
+ // the cast. We could potentially improve performance here by casting the
+ // dictionary values directly without unpacking the array first, although this
+ // would add more complexity to the code
+ match value_type.as_ref() {
+ DataType::Utf8 => {
+ let unpacked_array =
+ cast_with_options(&array, &DataType::Utf8, &CAST_OPTIONS)?;
+ Self::cast_string_to_int::<i32>(to_type, &unpacked_array, self.eval_mode)?
+ }
+ DataType::LargeUtf8 => {
+ let unpacked_array =
+ cast_with_options(&array, &DataType::LargeUtf8, &CAST_OPTIONS)?;
+ Self::cast_string_to_int::<i64>(to_type, &unpacked_array, self.eval_mode)?
+ }
+ _ => unreachable!("invalid value type for dictionary-encoded string array"),
Review Comment:
```suggestion
dt => unreachable!(
"{}",
format!("invalid value type {dt} for dictionary-encoded string array")
),
```
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1579680869
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -222,3 +581,34 @@ impl PhysicalExpr for Cast {
self.hash(&mut s);
}
}
+
+#[cfg(test)]
+mod test {
+ use super::{cast_string_to_i8, EvalMode};
+
+ #[test]
+ fn test_cast_string_as_i8() {
Review Comment:
I have now added tests for all min/max boundary values in the Scala 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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1584909396
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -142,6 +230,193 @@ impl Cast {
}
}
+fn cast_string_to_i8(str: &str, eval_mode: EvalMode) -> CometResult<Option<i8>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "TINYINT",
+ i8::MIN as i32,
+ i8::MAX as i32,
+ )?
+ .map(|v| v as i8))
+}
+
+fn cast_string_to_i16(str: &str, eval_mode: EvalMode) -> CometResult<Option<i16>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "SMALLINT",
+ i16::MIN as i32,
+ i16::MAX as i32,
+ )?
+ .map(|v| v as i16))
+}
+
+fn cast_string_to_i32(str: &str, eval_mode: EvalMode) -> CometResult<Option<i32>> {
+ do_cast_string_to_int::<i32>(str, eval_mode, "INT", i32::MIN)
+}
+
+fn cast_string_to_i64(str: &str, eval_mode: EvalMode) -> CometResult<Option<i64>> {
+ do_cast_string_to_int::<i64>(str, eval_mode, "BIGINT", i64::MIN)
+}
Review Comment:
I pushed a commit to add some comments referencing the Spark code that this code is based on
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1586606093
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -142,6 +233,202 @@ impl Cast {
}
}
+/// Equivalent to org.apache.spark.unsafe.types.UTF8String.toByte
+fn cast_string_to_i8(str: &str, eval_mode: EvalMode) -> CometResult<Option<i8>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "TINYINT",
+ i8::MIN as i32,
+ i8::MAX as i32,
+ )?
+ .map(|v| v as i8))
+}
+
+/// Equivalent to org.apache.spark.unsafe.types.UTF8String.toShort
+fn cast_string_to_i16(str: &str, eval_mode: EvalMode) -> CometResult<Option<i16>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "SMALLINT",
+ i16::MIN as i32,
+ i16::MAX as i32,
+ )?
+ .map(|v| v as i16))
+}
+
+/// Equivalent to org.apache.spark.unsafe.types.UTF8String.toInt(IntWrapper intWrapper)
+fn cast_string_to_i32(str: &str, eval_mode: EvalMode) -> CometResult<Option<i32>> {
+ do_cast_string_to_int::<i32>(str, eval_mode, "INT", i32::MIN)
+}
+
+/// Equivalent to org.apache.spark.unsafe.types.UTF8String.toLong(LongWrapper intWrapper)
+fn cast_string_to_i64(str: &str, eval_mode: EvalMode) -> CometResult<Option<i64>> {
+ do_cast_string_to_int::<i64>(str, eval_mode, "BIGINT", i64::MIN)
+}
+
+fn cast_string_to_int_with_range_check(
+ str: &str,
+ eval_mode: EvalMode,
+ type_name: &str,
+ min: i32,
+ max: i32,
+) -> CometResult<Option<i32>> {
+ match do_cast_string_to_int(str, eval_mode, type_name, i32::MIN)? {
+ None => Ok(None),
+ Some(v) if v >= min && v <= max => Ok(Some(v)),
+ _ if eval_mode == EvalMode::Ansi => Err(invalid_value(str, "STRING", type_name)),
+ _ => Ok(None),
+ }
+}
+
+#[derive(PartialEq)]
+enum State {
+ SkipLeadingWhiteSpace,
+ SkipTrailingWhiteSpace,
+ ParseSignAndDigits,
+ ParseFractionalDigits,
+}
+
+/// Equivalent to
+/// - org.apache.spark.unsafe.types.UTF8String.toInt(IntWrapper intWrapper, boolean allowDecimal)
+/// - org.apache.spark.unsafe.types.UTF8String.toLong(LongWrapper longWrapper, boolean allowDecimal)
+fn do_cast_string_to_int<
+ T: Num + PartialOrd + Integer + CheckedSub + CheckedNeg + From<i32> + Copy,
+>(
+ str: &str,
+ eval_mode: EvalMode,
+ type_name: &str,
+ min_value: T,
+) -> CometResult<Option<T>> {
+ let len = str.len();
+ if str.is_empty() {
+ return none_or_err(eval_mode, type_name, str);
+ }
+
+ let mut result: T = T::zero();
+ let mut negative = false;
+ let radix = T::from(10);
+ let stop_value = min_value / radix;
+ let mut state = State::SkipLeadingWhiteSpace;
+ let mut parsed_sign = false;
+
+ for (i, ch) in str.char_indices() {
+ // skip leading whitespace
+ if state == State::SkipLeadingWhiteSpace {
+ if ch.is_whitespace() {
+ // consume this char
+ continue;
+ }
+ // change state and fall through to next section
+ state = State::ParseSignAndDigits;
+ }
+
+ if state == State::ParseSignAndDigits {
+ if !parsed_sign {
+ negative = ch == '-';
+ let positive = ch == '+';
+ parsed_sign = true;
+ if negative || positive {
+ if i + 1 == len {
+ // input string is just "+" or "-"
+ return none_or_err(eval_mode, type_name, str);
+ }
+ // consume this char
+ continue;
+ }
+ }
+
+ if ch == '.' {
+ if eval_mode == EvalMode::Legacy {
+ // truncate decimal in legacy mode
+ state = State::ParseFractionalDigits;
+ continue;
+ } else {
+ return none_or_err(eval_mode, type_name, str);
+ }
+ }
+
+ let digit = if ch.is_ascii_digit() {
+ (ch as u32) - ('0' as u32)
+ } else {
+ return none_or_err(eval_mode, type_name, str);
+ };
+
+ // We are going to process the new digit and accumulate the result. However, before
Review Comment:
A comment to explain why we're using subtraction instead of addition would make it easier to understand this part of the code.
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1576969288
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -103,10 +126,78 @@ impl Cast {
(DataType::LargeUtf8, DataType::Boolean) => {
Self::spark_cast_utf8_to_boolean::<i64>(&array, self.eval_mode)?
}
- _ => cast_with_options(&array, to_type, &CAST_OPTIONS)?,
+ (
+ DataType::Utf8,
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) => Self::spark_cast_string_to_integral(to_type, &array, self.eval_mode)?,
+ (
+ DataType::Dictionary(key_type, value_type),
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) if key_type.as_ref() == &DataType::Int32
Review Comment:
Except for that any operator or expression during execution produce a dictionary with keys other than Int32 type. But for that I think it should be considered a bug for us to fix because I don't think it makes sense to change dictionary key type.
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1576810386
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -103,10 +126,78 @@ impl Cast {
(DataType::LargeUtf8, DataType::Boolean) => {
Self::spark_cast_utf8_to_boolean::<i64>(&array, self.eval_mode)?
}
- _ => cast_with_options(&array, to_type, &CAST_OPTIONS)?,
+ (
+ DataType::Utf8,
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) => Self::spark_cast_string_to_integral(to_type, &array, self.eval_mode)?,
+ (
+ DataType::Dictionary(key_type, value_type),
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) if key_type.as_ref() == &DataType::Int32
Review Comment:
I've been assuming it to be so, though @viirya can give us the definitive answer
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1578925501
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -103,10 +125,72 @@ impl Cast {
(DataType::LargeUtf8, DataType::Boolean) => {
Self::spark_cast_utf8_to_boolean::<i64>(&array, self.eval_mode)?
}
- _ => cast_with_options(&array, to_type, &CAST_OPTIONS)?,
+ (
+ DataType::Utf8,
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) => Self::cast_string_to_int(to_type, &array, self.eval_mode)?,
+ (
+ DataType::Dictionary(key_type, value_type),
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) if key_type.as_ref() == &DataType::Int32
+ && value_type.as_ref() == &DataType::Utf8 =>
+ {
+ // Note that we are unpacking a dictionary-encoded array and then performing
+ // the cast. We could potentially improve performance here by casting the
+ // dictionary values directly without unpacking the array first, although this
+ // would add more complexity to the code
Review Comment:
I think we can leave a TODO to cast dictionary directly?
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -142,6 +226,281 @@ impl Cast {
}
}
+fn cast_string_to_i8(str: &str, eval_mode: EvalMode) -> CometResult<Option<i8>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "TINYINT",
+ i8::MIN as i32,
+ i8::MAX as i32,
+ )?
+ .map(|v| v as i8))
+}
+
+fn cast_string_to_i16(str: &str, eval_mode: EvalMode) -> CometResult<Option<i16>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "SMALLINT",
+ i16::MIN as i32,
+ i16::MAX as i32,
+ )?
+ .map(|v| v as i16))
+}
+
+fn cast_string_to_i32(str: &str, eval_mode: EvalMode) -> CometResult<Option<i32>> {
+ let mut accum = CastStringToInt32::default();
+ do_cast_string_to_int(&mut accum, str, eval_mode, "INT")?;
+ Ok(accum.result)
+}
+
+fn cast_string_to_i64(str: &str, eval_mode: EvalMode) -> CometResult<Option<i64>> {
+ let mut accum = CastStringToInt64::default();
+ do_cast_string_to_int(&mut accum, str, eval_mode, "BIGINT")?;
+ Ok(accum.result)
+}
+
+fn cast_string_to_int_with_range_check(
+ str: &str,
+ eval_mode: EvalMode,
+ type_name: &str,
+ min: i32,
+ max: i32,
+) -> CometResult<Option<i32>> {
+ let mut accum = CastStringToInt32::default();
+ do_cast_string_to_int(&mut accum, str, eval_mode, type_name)?;
+ match accum.result {
+ None => Ok(None),
+ Some(v) if v >= min && v <= max => Ok(Some(v)),
+ _ if eval_mode == EvalMode::Ansi => Err(invalid_value(str, "STRING", type_name)),
+ _ => Ok(None),
+ }
+}
+
+/// We support parsing strings to i32 and i64 to match Spark's logic. Support for i8 and i16 is
+/// implemented by first parsing as i32 and then downcasting. The CastStringToInt trait is
+/// introduced so that we can have the parsing logic delegate either to an i32 or i64 accumulator
+/// and avoid the need to use macros here.
+trait CastStringToInt {
+ fn accumulate(
+ &mut self,
+ eval_mode: EvalMode,
+ type_name: &str,
+ str: &str,
+ digit: u32,
+ ) -> CometResult<()>;
+
+ fn reset(&mut self);
+
+ fn finish(
+ &mut self,
+ eval_mode: EvalMode,
+ type_name: &str,
+ str: &str,
+ negative: bool,
+ ) -> CometResult<()>;
+}
+struct CastStringToInt32 {
+ negative: bool,
+ result: Option<i32>,
+ radix: i32,
+}
+
+impl Default for CastStringToInt32 {
+ fn default() -> Self {
+ Self {
+ negative: false,
+ result: Some(0),
+ radix: 10,
+ }
+ }
+}
+
+impl CastStringToInt for CastStringToInt32 {
+ fn accumulate(
+ &mut self,
+ eval_mode: EvalMode,
+ type_name: &str,
+ str: &str,
+ digit: u32,
+ ) -> CometResult<()> {
+ if self.result.is_some() && self.result.unwrap() < i32::MIN / self.radix {
+ self.reset();
+ return none_or_err(eval_mode, type_name, str);
+ }
+ self.result = Some(self.result.unwrap_or(0) * self.radix - digit as i32);
+ if self.result.unwrap() > 0 {
+ self.reset();
+ return none_or_err(eval_mode, type_name, str);
+ }
+ Ok(())
+ }
+ fn reset(&mut self) {
+ self.result = None;
+ }
+
+ fn finish(
+ &mut self,
+ eval_mode: EvalMode,
+ type_name: &str,
+ str: &str,
+ negative: bool,
+ ) -> CometResult<()> {
+ if self.result.is_some() && !negative {
+ self.result = Some(-self.result.unwrap());
+ if self.result.unwrap() < 0 {
+ return none_or_err(eval_mode, type_name, str);
+ }
+ }
+ Ok(())
+ }
+}
+
+struct CastStringToInt64 {
+ negative: bool,
+ result: Option<i64>,
+ radix: i64,
+}
+
+impl Default for CastStringToInt64 {
+ fn default() -> Self {
+ Self {
+ negative: false,
+ result: Some(0),
+ radix: 10,
+ }
+ }
+}
+
+impl CastStringToInt for CastStringToInt64 {
+ fn accumulate(
+ &mut self,
+ eval_mode: EvalMode,
+ type_name: &str,
+ str: &str,
+ digit: u32,
+ ) -> CometResult<()> {
+ if self.result.unwrap_or(0) < i64::MIN / self.radix {
+ self.reset();
+ return none_or_err(eval_mode, type_name, str);
+ }
+ self.result = Some(self.result.unwrap_or(0) * self.radix - digit as i64);
+ if self.result.unwrap() > 0 {
+ self.reset();
+ return none_or_err(eval_mode, type_name, str);
+ }
+ Ok(())
+ }
+
+ fn reset(&mut self) {
+ self.result = None;
+ }
+
+ fn finish(
+ &mut self,
+ eval_mode: EvalMode,
+ type_name: &str,
+ str: &str,
+ negative: bool,
+ ) -> CometResult<()> {
+ if self.result.is_some() && !negative {
+ self.result = Some(-self.result.unwrap());
+ if self.result.unwrap() < 0 {
+ return none_or_err(eval_mode, type_name, str);
+ }
+ }
+ Ok(())
+ }
+}
+
+fn do_cast_string_to_int(
+ accumulator: &mut dyn CastStringToInt,
+ str: &str,
+ eval_mode: EvalMode,
+ type_name: &str,
+) -> CometResult<()> {
+ let chars: Vec<char> = str.chars().collect();
+ let mut i = 0;
+ let mut end = chars.len();
+
+ // skip leading whitespace
+ while i < end && chars[i].is_whitespace() {
+ i += 1;
+ }
+
+ // skip trailing whitespace
+ while end > i && chars[end - 1].is_whitespace() {
+ end -= 1;
+ }
+
+ // check for empty string
+ if i == end {
+ accumulator.reset();
+ return Ok(());
+ }
+
+ // skip + or -
+ let negative = chars[0] == '-';
+ if negative || chars[0] == '+' {
Review Comment:
This seems wrong.
It should be `chars[i] == '-'` instead? Otherwise, this cast doesn't work for ` -124`
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -103,10 +125,72 @@ impl Cast {
(DataType::LargeUtf8, DataType::Boolean) => {
Self::spark_cast_utf8_to_boolean::<i64>(&array, self.eval_mode)?
Review Comment:
Not part of this pr. But if we are going to name the added method as `cast_string_to_int`.
This method should be renamed to `cast_utf8_to_boolean` as well in a follow-up PR?
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -222,3 +581,34 @@ impl PhysicalExpr for Cast {
self.hash(&mut s);
}
}
+
+#[cfg(test)]
+mod test {
+ use super::{cast_string_to_i8, EvalMode};
+
+ #[test]
+ fn test_cast_string_as_i8() {
Review Comment:
how about add more tests about `i32` and `i64` with its min/max and zero input?
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#issuecomment-2073292355
Thanks @andygrove btw I'm wondering if this PR should cover scope with formatting https://spark.apache.org/docs/latest/sql-ref-number-pattern.html#the-to_number-function
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1576844996
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -64,6 +68,25 @@ pub struct Cast {
pub timezone: String,
}
+macro_rules! spark_cast_utf8_to_integral {
+ ($string_array:expr, $eval_mode:expr, $array_type:ty, $cast_method:ident) => {{
+ let mut cast_array = PrimitiveArray::<$array_type>::builder($string_array.len());
+ for i in 0..$string_array.len() {
Review Comment:
I see them on lines 73,74 🤔
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1576944454
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -64,6 +68,25 @@ pub struct Cast {
pub timezone: String,
}
+macro_rules! spark_cast_utf8_to_integral {
+ ($string_array:expr, $eval_mode:expr, $array_type:ty, $cast_method:ident) => {{
+ let mut cast_array = PrimitiveArray::<$array_type>::builder($string_array.len());
+ for i in 0..$string_array.len() {
Review Comment:
Fixed
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1581740802
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -103,10 +125,72 @@ impl Cast {
(DataType::LargeUtf8, DataType::Boolean) => {
Self::spark_cast_utf8_to_boolean::<i64>(&array, self.eval_mode)?
}
- _ => cast_with_options(&array, to_type, &CAST_OPTIONS)?,
+ (
+ DataType::Utf8,
Review Comment:
Don't we need to handle LargeUtf8?
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1581872458
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -103,10 +125,72 @@ impl Cast {
(DataType::LargeUtf8, DataType::Boolean) => {
Self::spark_cast_utf8_to_boolean::<i64>(&array, self.eval_mode)?
}
- _ => cast_with_options(&array, to_type, &CAST_OPTIONS)?,
+ (
+ DataType::Utf8,
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) => Self::cast_string_to_int(to_type, &array, self.eval_mode)?,
+ (
+ DataType::Dictionary(key_type, value_type),
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) if key_type.as_ref() == &DataType::Int32
+ && value_type.as_ref() == &DataType::Utf8 =>
+ {
+ // TODO: we are unpacking a dictionary-encoded array and then performing
+ // the cast. We could potentially improve performance here by casting the
+ // dictionary values directly without unpacking the array first, although this
+ // would add more complexity to the code
+ let unpacked_array = Self::unpack_dict_string_array::<Int32Type>(&array)?;
+ Self::cast_string_to_int(to_type, &unpacked_array, self.eval_mode)?
+ }
+ _ => {
+ // when we have no Spark-specific casting we delegate to DataFusion
+ cast_with_options(&array, to_type, &CAST_OPTIONS)?
+ }
};
- let result = spark_cast(cast_result, from_type, to_type);
- Ok(result)
+ Ok(spark_cast(cast_result, from_type, to_type))
+ }
+
+ fn cast_string_to_int(
+ to_type: &DataType,
+ array: &ArrayRef,
+ eval_mode: EvalMode,
+ ) -> CometResult<ArrayRef> {
+ let string_array = array
+ .as_any()
+ .downcast_ref::<GenericStringArray<i32>>()
+ .expect("cast_string_to_int expected a string array");
+
+ let cast_array: ArrayRef = match to_type {
+ DataType::Int8 => {
+ cast_utf8_to_int!(string_array, eval_mode, Int8Type, cast_string_to_i8)?
+ }
+ DataType::Int16 => {
+ cast_utf8_to_int!(string_array, eval_mode, Int16Type, cast_string_to_i16)?
+ }
+ DataType::Int32 => {
+ cast_utf8_to_int!(string_array, eval_mode, Int32Type, cast_string_to_i32)?
+ }
+ DataType::Int64 => {
+ cast_utf8_to_int!(string_array, eval_mode, Int64Type, cast_string_to_i64)?
+ }
+ dt => unreachable!(
+ "{}",
+ format!("invalid integer type {dt} in cast from string")
+ ),
+ };
+ Ok(cast_array)
+ }
+
+ fn unpack_dict_string_array<T: ArrowDictionaryKeyType>(
+ array: &ArrayRef,
+ ) -> DataFusionResult<ArrayRef> {
+ let dict_array = array
+ .as_any()
+ .downcast_ref::<DictionaryArray<T>>()
+ .expect("DictionaryArray<T>");
+
+ let unpacked_array = take(dict_array.values().as_ref(), dict_array.keys(), None)?;
+ Ok(unpacked_array)
}
Review Comment:
Just call `cast_with_options` from arrow-rs? For dictionary type, it will call `unpack_dictionary`
https://github.com/apache/arrow-rs/blob/a61f1dc8ea132add731e4426e11d341a1de5ca92/arrow-cast/src/cast/dictionary.rs#L93
which looks like the same as this implementation.
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1579561967
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -103,10 +125,72 @@ impl Cast {
(DataType::LargeUtf8, DataType::Boolean) => {
Self::spark_cast_utf8_to_boolean::<i64>(&array, self.eval_mode)?
Review Comment:
I agree. I didn't want to start making unrelated changes in this PR, but we should rename this.
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1583889008
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -142,6 +230,193 @@ impl Cast {
}
}
+fn cast_string_to_i8(str: &str, eval_mode: EvalMode) -> CometResult<Option<i8>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "TINYINT",
+ i8::MIN as i32,
+ i8::MAX as i32,
+ )?
+ .map(|v| v as i8))
+}
+
+fn cast_string_to_i16(str: &str, eval_mode: EvalMode) -> CometResult<Option<i16>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "SMALLINT",
+ i16::MIN as i32,
+ i16::MAX as i32,
+ )?
+ .map(|v| v as i16))
+}
+
+fn cast_string_to_i32(str: &str, eval_mode: EvalMode) -> CometResult<Option<i32>> {
+ do_cast_string_to_int::<i32>(str, eval_mode, "INT", i32::MIN)
+}
+
+fn cast_string_to_i64(str: &str, eval_mode: EvalMode) -> CometResult<Option<i64>> {
+ do_cast_string_to_int::<i64>(str, eval_mode, "BIGINT", i64::MIN)
+}
+
+fn cast_string_to_int_with_range_check(
+ str: &str,
+ eval_mode: EvalMode,
+ type_name: &str,
+ min: i32,
+ max: i32,
+) -> CometResult<Option<i32>> {
+ match do_cast_string_to_int(str, eval_mode, type_name, i32::MIN)? {
+ None => Ok(None),
+ Some(v) if v >= min && v <= max => Ok(Some(v)),
+ _ if eval_mode == EvalMode::Ansi => Err(invalid_value(str, "STRING", type_name)),
+ _ => Ok(None),
+ }
+}
+
+#[derive(PartialEq)]
+enum State {
+ SkipLeadingWhiteSpace,
+ SkipTrailingWhiteSpace,
+ ParseSignAndDigits,
+ ParseFractionalDigits,
+}
+
+fn do_cast_string_to_int<
+ T: Num + PartialOrd + Integer + CheckedSub + CheckedNeg + From<i32> + Copy,
+>(
+ str: &str,
+ eval_mode: EvalMode,
+ type_name: &str,
+ min_value: T,
+) -> CometResult<Option<T>> {
+ let len = str.len();
+ if len == 0 {
+ return none_or_err(eval_mode, type_name, str);
+ }
+
+ let mut result: T = T::zero();
+ let mut negative = false;
+ let radix = T::from(10);
+ let stop_value = min_value / radix;
+ let mut state = State::SkipLeadingWhiteSpace;
+ let mut parsed_sign = false;
+
+ for (i, ch) in str.char_indices() {
+ // skip leading whitespace
+ if state == State::SkipLeadingWhiteSpace {
+ if ch.is_whitespace() {
+ // consume this char
+ continue;
+ }
+ // change state and fall through to next section
+ state = State::ParseSignAndDigits;
+ }
+
+ if state == State::ParseSignAndDigits {
+ if !parsed_sign {
+ negative = ch == '-';
+ let positive = ch == '+';
+ parsed_sign = true;
+ if negative || positive {
+ if i + 1 == len {
+ // input string is just "+" or "-"
+ return none_or_err(eval_mode, type_name, str);
+ }
+ // consume this char
+ continue;
+ }
+ }
+
+ if ch == '.' {
+ if eval_mode == EvalMode::Legacy {
+ // truncate decimal in legacy mode
+ state = State::ParseFractionalDigits;
+ continue;
+ } else {
+ return none_or_err(eval_mode, type_name, str);
+ }
+ }
+
+ let digit = if ch.is_ascii_digit() {
+ (ch as u32) - ('0' as u32)
+ } else {
+ return none_or_err(eval_mode, type_name, str);
+ };
+
+ // We are going to process the new digit and accumulate the result. However, before
+ // doing this, if the result is already smaller than the
+ // stopValue(Integer.MIN_VALUE / radix), then result * 10 will definitely be
+ // smaller than minValue, and we can stop
+ if result < stop_value {
+ return none_or_err(eval_mode, type_name, str);
+ }
+
+ // Since the previous result is less than or equal to stopValue(Integer.MIN_VALUE /
+ // radix), we can just use `result > 0` to check overflow. If result
+ // overflows, we should stop
Review Comment:
The comment was copied from the Spark code in `org/apache/spark/unsafe/types/UTF8String.java`, but I agree that it seems incorrect. I have updated it.
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1583738069
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -142,6 +230,193 @@ impl Cast {
}
}
+fn cast_string_to_i8(str: &str, eval_mode: EvalMode) -> CometResult<Option<i8>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "TINYINT",
+ i8::MIN as i32,
+ i8::MAX as i32,
+ )?
+ .map(|v| v as i8))
+}
+
+fn cast_string_to_i16(str: &str, eval_mode: EvalMode) -> CometResult<Option<i16>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "SMALLINT",
+ i16::MIN as i32,
+ i16::MAX as i32,
+ )?
+ .map(|v| v as i16))
+}
+
+fn cast_string_to_i32(str: &str, eval_mode: EvalMode) -> CometResult<Option<i32>> {
+ do_cast_string_to_int::<i32>(str, eval_mode, "INT", i32::MIN)
+}
+
+fn cast_string_to_i64(str: &str, eval_mode: EvalMode) -> CometResult<Option<i64>> {
+ do_cast_string_to_int::<i64>(str, eval_mode, "BIGINT", i64::MIN)
+}
Review Comment:
Why only i8 and i16 have range check?
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1581140743
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -103,10 +126,78 @@ impl Cast {
(DataType::LargeUtf8, DataType::Boolean) => {
Self::spark_cast_utf8_to_boolean::<i64>(&array, self.eval_mode)?
}
- _ => cast_with_options(&array, to_type, &CAST_OPTIONS)?,
+ (
+ DataType::Utf8,
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) => Self::spark_cast_string_to_integral(to_type, &array, self.eval_mode)?,
+ (
+ DataType::Dictionary(key_type, value_type),
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) if key_type.as_ref() == &DataType::Int32
Review Comment:
Thanks @sunchao for confirming it.
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1581891542
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -103,10 +125,72 @@ impl Cast {
(DataType::LargeUtf8, DataType::Boolean) => {
Self::spark_cast_utf8_to_boolean::<i64>(&array, self.eval_mode)?
}
- _ => cast_with_options(&array, to_type, &CAST_OPTIONS)?,
+ (
+ DataType::Utf8,
Review Comment:
As far as I know, we don't have a way to have LargeUtf8 to test in Scala tests. If possible, maybe just unit test the function. If no, I think the logic is same as Utf8 so it should be fine to test Utf8 path only.
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1581740471
##########
spark/src/test/scala/org/apache/comet/CometCastSuite.scala:
##########
@@ -66,20 +71,65 @@ class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper {
castTest(testValues, DataTypes.BooleanType)
}
- ignore("cast string to byte") {
- castTest(generateStrings(numericPattern, 8).toDF("a"), DataTypes.ByteType)
- }
-
- ignore("cast string to short") {
- castTest(generateStrings(numericPattern, 8).toDF("a"), DataTypes.ShortType)
- }
-
- ignore("cast string to int") {
- castTest(generateStrings(numericPattern, 8).toDF("a"), DataTypes.IntegerType)
- }
-
- ignore("cast string to long") {
- castTest(generateStrings(numericPattern, 8).toDF("a"), DataTypes.LongType)
+ private val castStringToIntegralInputs: Seq[String] = Seq(
+ "",
+ ".",
+ "+",
+ "-",
+ "+.",
+ "-.",
+ "-0",
+ "+1",
+ "-1",
+ ".2",
+ "-.2",
+ "1e1",
+ "1.1d",
+ "1.1f",
+ Byte.MinValue.toString,
+ (Byte.MinValue.toShort - 1).toString,
+ Byte.MaxValue.toString,
+ (Byte.MaxValue.toShort + 1).toString,
+ Short.MinValue.toString,
+ (Short.MinValue.toInt - 1).toString,
+ Short.MaxValue.toString,
+ (Short.MaxValue.toInt + 1).toString,
+ Int.MinValue.toString,
+ (Int.MinValue.toLong - 1).toString,
+ Int.MaxValue.toString,
+ (Int.MaxValue.toLong + 1).toString,
+ Long.MinValue.toString,
+ Long.MaxValue.toString,
+ "-9223372036854775809", // Long.MinValue -1
+ "9223372036854775808" // Long.MaxValue + 1
+ )
+
+ test("cast string to byte") {
+ // test with hand-picked values
+ castTest(castStringToIntegralInputs.toDF("a"), DataTypes.ByteType)
+ // fuzz test
+ castTest(generateStrings(numericPattern, 5).toDF("a"), DataTypes.ByteType)
+ }
+
+ test("cast string to short") {
+ // test with hand-picked values
+ castTest(castStringToIntegralInputs.toDF("a"), DataTypes.ByteType)
+ // fuzz test
+ castTest(generateStrings(numericPattern, 5).toDF("a"), DataTypes.ByteType)
+ }
+
+ test("cast string to int") {
+ // test with hand-picked values
+ castTest(castStringToIntegralInputs.toDF("a"), DataTypes.ByteType)
+ // fuzz test
+ castTest(generateStrings(numericPattern, 5).toDF("a"), DataTypes.ByteType)
+ }
+
+ test("cast string to long") {
+ // test with hand-picked values
+ castTest(castStringToIntegralInputs.toDF("a"), DataTypes.ByteType)
+ // fuzz test
+ castTest(generateStrings(numericPattern, 5).toDF("a"), DataTypes.ByteType)
Review Comment:
```suggestion
castTest(castStringToIntegralInputs.toDF("a"), DataTypes.LongType)
// fuzz test
castTest(generateStrings(numericPattern, 5).toDF("a"), DataTypes.LongType)
```
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1583667605
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -103,10 +125,76 @@ impl Cast {
(DataType::LargeUtf8, DataType::Boolean) => {
Self::spark_cast_utf8_to_boolean::<i64>(&array, self.eval_mode)?
}
- _ => cast_with_options(&array, to_type, &CAST_OPTIONS)?,
+ (
+ DataType::Utf8,
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) => Self::cast_string_to_int::<i32>(to_type, &array, self.eval_mode)?,
+ (
+ DataType::LargeUtf8,
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) => Self::cast_string_to_int::<i64>(to_type, &array, self.eval_mode)?,
+ (
+ DataType::Dictionary(key_type, value_type),
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) if key_type.as_ref() == &DataType::Int32
+ && (value_type.as_ref() == &DataType::Utf8
+ || value_type.as_ref() == &DataType::LargeUtf8) =>
+ {
+ // TODO: we are unpacking a dictionary-encoded array and then performing
+ // the cast. We could potentially improve performance here by casting the
+ // dictionary values directly without unpacking the array first, although this
+ // would add more complexity to the code
+ match value_type.as_ref() {
+ DataType::Utf8 => {
+ let unpacked_array =
+ cast_with_options(&array, &DataType::Utf8, &CAST_OPTIONS)?;
+ Self::cast_string_to_int::<i32>(to_type, &unpacked_array, self.eval_mode)?
+ }
+ DataType::LargeUtf8 => {
+ let unpacked_array =
+ cast_with_options(&array, &DataType::LargeUtf8, &CAST_OPTIONS)?;
+ Self::cast_string_to_int::<i64>(to_type, &unpacked_array, self.eval_mode)?
+ }
+ _ => unreachable!("invalid value type for dictionary-encoded string array"),
Review Comment:
```suggestion
dt => unreachable!("invalid value type {dt} for dictionary-encoded string array"),
```
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1576795634
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -64,6 +68,25 @@ pub struct Cast {
pub timezone: String,
}
+macro_rules! spark_cast_utf8_to_integral {
+ ($string_array:expr, $eval_mode:expr, $array_type:ty, $cast_method:ident) => {{
+ let mut cast_array = PrimitiveArray::<$array_type>::builder($string_array.len());
+ for i in 0..$string_array.len() {
Review Comment:
we probably can use iterator instead of for loop?
and lets calc `$string_array.len()` once
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1579562716
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -222,3 +581,34 @@ impl PhysicalExpr for Cast {
self.hash(&mut s);
}
}
+
+#[cfg(test)]
+mod test {
+ use super::{cast_string_to_i8, EvalMode};
+
+ #[test]
+ fn test_cast_string_as_i8() {
Review Comment:
I am going to focus on improving the tests in this PR today
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1576796459
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -103,10 +126,78 @@ impl Cast {
(DataType::LargeUtf8, DataType::Boolean) => {
Self::spark_cast_utf8_to_boolean::<i64>(&array, self.eval_mode)?
}
- _ => cast_with_options(&array, to_type, &CAST_OPTIONS)?,
+ (
+ DataType::Utf8,
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) => Self::spark_cast_string_to_integral(to_type, &array, self.eval_mode)?,
+ (
+ DataType::Dictionary(key_type, value_type),
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) if key_type.as_ref() == &DataType::Int32
+ && value_type.as_ref() == &DataType::Utf8 =>
+ {
+ // Note that we are unpacking a dictionary-encoded array and then performing
+ // the cast. We could potentially improve performance here by casting the
+ // dictionary values directly without unpacking the array first, although this
+ // would add more complexity to the code
+ let unpacked_array = Self::unpack_dict_string_array::<Int32Type>(&array)?;
+ Self::spark_cast_string_to_integral(to_type, &unpacked_array, self.eval_mode)?
+ }
+ _ => {
+ // when we have no Spark-specific casting we delegate to DataFusion
+ cast_with_options(&array, to_type, &CAST_OPTIONS)?
+ }
+ };
+ Ok(spark_cast(cast_result, from_type, to_type))
+ }
+
+ fn spark_cast_string_to_integral(
Review Comment:
`string_to_int`?
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1581740415
##########
spark/src/test/scala/org/apache/comet/CometCastSuite.scala:
##########
@@ -66,20 +71,65 @@ class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper {
castTest(testValues, DataTypes.BooleanType)
}
- ignore("cast string to byte") {
- castTest(generateStrings(numericPattern, 8).toDF("a"), DataTypes.ByteType)
- }
-
- ignore("cast string to short") {
- castTest(generateStrings(numericPattern, 8).toDF("a"), DataTypes.ShortType)
- }
-
- ignore("cast string to int") {
- castTest(generateStrings(numericPattern, 8).toDF("a"), DataTypes.IntegerType)
- }
-
- ignore("cast string to long") {
- castTest(generateStrings(numericPattern, 8).toDF("a"), DataTypes.LongType)
+ private val castStringToIntegralInputs: Seq[String] = Seq(
+ "",
+ ".",
+ "+",
+ "-",
+ "+.",
+ "-.",
+ "-0",
+ "+1",
+ "-1",
+ ".2",
+ "-.2",
+ "1e1",
+ "1.1d",
+ "1.1f",
+ Byte.MinValue.toString,
+ (Byte.MinValue.toShort - 1).toString,
+ Byte.MaxValue.toString,
+ (Byte.MaxValue.toShort + 1).toString,
+ Short.MinValue.toString,
+ (Short.MinValue.toInt - 1).toString,
+ Short.MaxValue.toString,
+ (Short.MaxValue.toInt + 1).toString,
+ Int.MinValue.toString,
+ (Int.MinValue.toLong - 1).toString,
+ Int.MaxValue.toString,
+ (Int.MaxValue.toLong + 1).toString,
+ Long.MinValue.toString,
+ Long.MaxValue.toString,
+ "-9223372036854775809", // Long.MinValue -1
+ "9223372036854775808" // Long.MaxValue + 1
+ )
+
+ test("cast string to byte") {
+ // test with hand-picked values
+ castTest(castStringToIntegralInputs.toDF("a"), DataTypes.ByteType)
+ // fuzz test
+ castTest(generateStrings(numericPattern, 5).toDF("a"), DataTypes.ByteType)
+ }
+
+ test("cast string to short") {
+ // test with hand-picked values
+ castTest(castStringToIntegralInputs.toDF("a"), DataTypes.ByteType)
+ // fuzz test
+ castTest(generateStrings(numericPattern, 5).toDF("a"), DataTypes.ByteType)
+ }
+
+ test("cast string to int") {
+ // test with hand-picked values
+ castTest(castStringToIntegralInputs.toDF("a"), DataTypes.ByteType)
Review Comment:
```suggestion
castTest(castStringToIntegralInputs.toDF("a"), DataTypes.IntegerType)
```
##########
spark/src/test/scala/org/apache/comet/CometCastSuite.scala:
##########
@@ -66,20 +71,65 @@ class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper {
castTest(testValues, DataTypes.BooleanType)
}
- ignore("cast string to byte") {
- castTest(generateStrings(numericPattern, 8).toDF("a"), DataTypes.ByteType)
- }
-
- ignore("cast string to short") {
- castTest(generateStrings(numericPattern, 8).toDF("a"), DataTypes.ShortType)
- }
-
- ignore("cast string to int") {
- castTest(generateStrings(numericPattern, 8).toDF("a"), DataTypes.IntegerType)
- }
-
- ignore("cast string to long") {
- castTest(generateStrings(numericPattern, 8).toDF("a"), DataTypes.LongType)
+ private val castStringToIntegralInputs: Seq[String] = Seq(
+ "",
+ ".",
+ "+",
+ "-",
+ "+.",
+ "-.",
+ "-0",
+ "+1",
+ "-1",
+ ".2",
+ "-.2",
+ "1e1",
+ "1.1d",
+ "1.1f",
+ Byte.MinValue.toString,
+ (Byte.MinValue.toShort - 1).toString,
+ Byte.MaxValue.toString,
+ (Byte.MaxValue.toShort + 1).toString,
+ Short.MinValue.toString,
+ (Short.MinValue.toInt - 1).toString,
+ Short.MaxValue.toString,
+ (Short.MaxValue.toInt + 1).toString,
+ Int.MinValue.toString,
+ (Int.MinValue.toLong - 1).toString,
+ Int.MaxValue.toString,
+ (Int.MaxValue.toLong + 1).toString,
+ Long.MinValue.toString,
+ Long.MaxValue.toString,
+ "-9223372036854775809", // Long.MinValue -1
+ "9223372036854775808" // Long.MaxValue + 1
+ )
+
+ test("cast string to byte") {
+ // test with hand-picked values
+ castTest(castStringToIntegralInputs.toDF("a"), DataTypes.ByteType)
+ // fuzz test
+ castTest(generateStrings(numericPattern, 5).toDF("a"), DataTypes.ByteType)
+ }
+
+ test("cast string to short") {
+ // test with hand-picked values
+ castTest(castStringToIntegralInputs.toDF("a"), DataTypes.ByteType)
+ // fuzz test
+ castTest(generateStrings(numericPattern, 5).toDF("a"), DataTypes.ByteType)
+ }
+
+ test("cast string to int") {
+ // test with hand-picked values
+ castTest(castStringToIntegralInputs.toDF("a"), DataTypes.ByteType)
+ // fuzz test
+ castTest(generateStrings(numericPattern, 5).toDF("a"), DataTypes.ByteType)
Review Comment:
```suggestion
castTest(generateStrings(numericPattern, 5).toDF("a"), DataTypes.IntegerType)
```
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1581871855
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -103,10 +125,72 @@ impl Cast {
(DataType::LargeUtf8, DataType::Boolean) => {
Self::spark_cast_utf8_to_boolean::<i64>(&array, self.eval_mode)?
}
- _ => cast_with_options(&array, to_type, &CAST_OPTIONS)?,
+ (
+ DataType::Utf8,
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) => Self::cast_string_to_int(to_type, &array, self.eval_mode)?,
+ (
+ DataType::Dictionary(key_type, value_type),
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) if key_type.as_ref() == &DataType::Int32
+ && value_type.as_ref() == &DataType::Utf8 =>
+ {
+ // TODO: we are unpacking a dictionary-encoded array and then performing
+ // the cast. We could potentially improve performance here by casting the
+ // dictionary values directly without unpacking the array first, although this
+ // would add more complexity to the code
+ let unpacked_array = Self::unpack_dict_string_array::<Int32Type>(&array)?;
+ Self::cast_string_to_int(to_type, &unpacked_array, self.eval_mode)?
+ }
+ _ => {
+ // when we have no Spark-specific casting we delegate to DataFusion
+ cast_with_options(&array, to_type, &CAST_OPTIONS)?
+ }
};
- let result = spark_cast(cast_result, from_type, to_type);
- Ok(result)
+ Ok(spark_cast(cast_result, from_type, to_type))
+ }
+
+ fn cast_string_to_int(
+ to_type: &DataType,
+ array: &ArrayRef,
+ eval_mode: EvalMode,
+ ) -> CometResult<ArrayRef> {
+ let string_array = array
+ .as_any()
+ .downcast_ref::<GenericStringArray<i32>>()
+ .expect("cast_string_to_int expected a string array");
+
+ let cast_array: ArrayRef = match to_type {
+ DataType::Int8 => {
+ cast_utf8_to_int!(string_array, eval_mode, Int8Type, cast_string_to_i8)?
+ }
+ DataType::Int16 => {
+ cast_utf8_to_int!(string_array, eval_mode, Int16Type, cast_string_to_i16)?
+ }
+ DataType::Int32 => {
+ cast_utf8_to_int!(string_array, eval_mode, Int32Type, cast_string_to_i32)?
+ }
+ DataType::Int64 => {
+ cast_utf8_to_int!(string_array, eval_mode, Int64Type, cast_string_to_i64)?
+ }
+ dt => unreachable!(
+ "{}",
+ format!("invalid integer type {dt} in cast from string")
+ ),
+ };
+ Ok(cast_array)
+ }
+
+ fn unpack_dict_string_array<T: ArrowDictionaryKeyType>(
+ array: &ArrayRef,
+ ) -> DataFusionResult<ArrayRef> {
+ let dict_array = array
+ .as_any()
+ .downcast_ref::<DictionaryArray<T>>()
+ .expect("DictionaryArray<T>");
+
+ let unpacked_array = take(dict_array.values().as_ref(), dict_array.keys(), None)?;
+ Ok(unpacked_array)
}
Review Comment:
Could you show me how to do this?
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1581876228
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -103,10 +125,72 @@ impl Cast {
(DataType::LargeUtf8, DataType::Boolean) => {
Self::spark_cast_utf8_to_boolean::<i64>(&array, self.eval_mode)?
}
- _ => cast_with_options(&array, to_type, &CAST_OPTIONS)?,
+ (
+ DataType::Utf8,
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) => Self::cast_string_to_int(to_type, &array, self.eval_mode)?,
+ (
+ DataType::Dictionary(key_type, value_type),
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) if key_type.as_ref() == &DataType::Int32
+ && value_type.as_ref() == &DataType::Utf8 =>
+ {
+ // TODO: we are unpacking a dictionary-encoded array and then performing
+ // the cast. We could potentially improve performance here by casting the
+ // dictionary values directly without unpacking the array first, although this
+ // would add more complexity to the code
+ let unpacked_array = Self::unpack_dict_string_array::<Int32Type>(&array)?;
+ Self::cast_string_to_int(to_type, &unpacked_array, self.eval_mode)?
+ }
+ _ => {
+ // when we have no Spark-specific casting we delegate to DataFusion
+ cast_with_options(&array, to_type, &CAST_OPTIONS)?
+ }
};
- let result = spark_cast(cast_result, from_type, to_type);
- Ok(result)
+ Ok(spark_cast(cast_result, from_type, to_type))
+ }
+
+ fn cast_string_to_int(
+ to_type: &DataType,
+ array: &ArrayRef,
+ eval_mode: EvalMode,
+ ) -> CometResult<ArrayRef> {
+ let string_array = array
+ .as_any()
+ .downcast_ref::<GenericStringArray<i32>>()
+ .expect("cast_string_to_int expected a string array");
+
+ let cast_array: ArrayRef = match to_type {
+ DataType::Int8 => {
+ cast_utf8_to_int!(string_array, eval_mode, Int8Type, cast_string_to_i8)?
+ }
+ DataType::Int16 => {
+ cast_utf8_to_int!(string_array, eval_mode, Int16Type, cast_string_to_i16)?
+ }
+ DataType::Int32 => {
+ cast_utf8_to_int!(string_array, eval_mode, Int32Type, cast_string_to_i32)?
+ }
+ DataType::Int64 => {
+ cast_utf8_to_int!(string_array, eval_mode, Int64Type, cast_string_to_i64)?
+ }
+ dt => unreachable!(
+ "{}",
+ format!("invalid integer type {dt} in cast from string")
+ ),
+ };
+ Ok(cast_array)
+ }
+
+ fn unpack_dict_string_array<T: ArrowDictionaryKeyType>(
+ array: &ArrayRef,
+ ) -> DataFusionResult<ArrayRef> {
+ let dict_array = array
+ .as_any()
+ .downcast_ref::<DictionaryArray<T>>()
+ .expect("DictionaryArray<T>");
+
+ let unpacked_array = take(dict_array.values().as_ref(), dict_array.keys(), None)?;
+ Ok(unpacked_array)
}
Review Comment:
Thanks. Updated.
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1583752306
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -142,6 +230,193 @@ impl Cast {
}
}
+fn cast_string_to_i8(str: &str, eval_mode: EvalMode) -> CometResult<Option<i8>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "TINYINT",
+ i8::MIN as i32,
+ i8::MAX as i32,
+ )?
+ .map(|v| v as i8))
+}
+
+fn cast_string_to_i16(str: &str, eval_mode: EvalMode) -> CometResult<Option<i16>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "SMALLINT",
+ i16::MIN as i32,
+ i16::MAX as i32,
+ )?
+ .map(|v| v as i16))
+}
+
+fn cast_string_to_i32(str: &str, eval_mode: EvalMode) -> CometResult<Option<i32>> {
+ do_cast_string_to_int::<i32>(str, eval_mode, "INT", i32::MIN)
+}
+
+fn cast_string_to_i64(str: &str, eval_mode: EvalMode) -> CometResult<Option<i64>> {
+ do_cast_string_to_int::<i64>(str, eval_mode, "BIGINT", i64::MIN)
+}
+
+fn cast_string_to_int_with_range_check(
+ str: &str,
+ eval_mode: EvalMode,
+ type_name: &str,
+ min: i32,
+ max: i32,
+) -> CometResult<Option<i32>> {
+ match do_cast_string_to_int(str, eval_mode, type_name, i32::MIN)? {
+ None => Ok(None),
+ Some(v) if v >= min && v <= max => Ok(Some(v)),
+ _ if eval_mode == EvalMode::Ansi => Err(invalid_value(str, "STRING", type_name)),
+ _ => Ok(None),
+ }
+}
+
+#[derive(PartialEq)]
+enum State {
+ SkipLeadingWhiteSpace,
+ SkipTrailingWhiteSpace,
+ ParseSignAndDigits,
+ ParseFractionalDigits,
+}
+
+fn do_cast_string_to_int<
+ T: Num + PartialOrd + Integer + CheckedSub + CheckedNeg + From<i32> + Copy,
+>(
+ str: &str,
+ eval_mode: EvalMode,
+ type_name: &str,
+ min_value: T,
+) -> CometResult<Option<T>> {
+ let len = str.len();
+ if len == 0 {
+ return none_or_err(eval_mode, type_name, str);
+ }
+
+ let mut result: T = T::zero();
+ let mut negative = false;
+ let radix = T::from(10);
+ let stop_value = min_value / radix;
+ let mut state = State::SkipLeadingWhiteSpace;
+ let mut parsed_sign = false;
+
+ for (i, ch) in str.char_indices() {
+ // skip leading whitespace
+ if state == State::SkipLeadingWhiteSpace {
+ if ch.is_whitespace() {
+ // consume this char
+ continue;
+ }
+ // change state and fall through to next section
+ state = State::ParseSignAndDigits;
+ }
+
+ if state == State::ParseSignAndDigits {
+ if !parsed_sign {
+ negative = ch == '-';
+ let positive = ch == '+';
+ parsed_sign = true;
+ if negative || positive {
+ if i + 1 == len {
+ // input string is just "+" or "-"
+ return none_or_err(eval_mode, type_name, str);
+ }
+ // consume this char
+ continue;
+ }
+ }
+
+ if ch == '.' {
+ if eval_mode == EvalMode::Legacy {
+ // truncate decimal in legacy mode
+ state = State::ParseFractionalDigits;
+ continue;
+ } else {
+ return none_or_err(eval_mode, type_name, str);
+ }
+ }
+
+ let digit = if ch.is_ascii_digit() {
+ (ch as u32) - ('0' as u32)
+ } else {
+ return none_or_err(eval_mode, type_name, str);
+ };
+
+ // We are going to process the new digit and accumulate the result. However, before
+ // doing this, if the result is already smaller than the
+ // stopValue(Integer.MIN_VALUE / radix), then result * 10 will definitely be
+ // smaller than minValue, and we can stop
+ if result < stop_value {
+ return none_or_err(eval_mode, type_name, str);
+ }
+
+ // Since the previous result is less than or equal to stopValue(Integer.MIN_VALUE /
+ // radix), we can just use `result > 0` to check overflow. If result
+ // overflows, we should stop
Review Comment:
Do you mean "more than or equal to"? I think the above condition (L352) is already for `result < stop_value`?
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1583883651
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -142,6 +230,193 @@ impl Cast {
}
}
+fn cast_string_to_i8(str: &str, eval_mode: EvalMode) -> CometResult<Option<i8>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "TINYINT",
+ i8::MIN as i32,
+ i8::MAX as i32,
+ )?
+ .map(|v| v as i8))
+}
+
+fn cast_string_to_i16(str: &str, eval_mode: EvalMode) -> CometResult<Option<i16>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "SMALLINT",
+ i16::MIN as i32,
+ i16::MAX as i32,
+ )?
+ .map(|v| v as i16))
+}
+
+fn cast_string_to_i32(str: &str, eval_mode: EvalMode) -> CometResult<Option<i32>> {
+ do_cast_string_to_int::<i32>(str, eval_mode, "INT", i32::MIN)
+}
+
+fn cast_string_to_i64(str: &str, eval_mode: EvalMode) -> CometResult<Option<i64>> {
+ do_cast_string_to_int::<i64>(str, eval_mode, "BIGINT", i64::MIN)
+}
Review Comment:
The code is ported directly from Spark. This is the approach that is used there.
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1583885384
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -64,6 +68,24 @@ pub struct Cast {
pub timezone: String,
}
+macro_rules! cast_utf8_to_int {
+ ($array:expr, $eval_mode:expr, $array_type:ty, $cast_method:ident) => {{
+ let len = $array.len();
+ let mut cast_array = PrimitiveArray::<$array_type>::builder(len);
+ for i in 0..len {
+ if $array.is_null(i) {
Review Comment:
If there is a null input then we will always want a null output and we don't want to add the overhead of calling the cast logic in this case.
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#issuecomment-2082781370
@viirya @sunchao @parthchandra @comphead I did quite a bit of refactoring and performance tuning over the weekend. Please take another look when you can.
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1585533880
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -142,6 +230,193 @@ impl Cast {
}
}
+fn cast_string_to_i8(str: &str, eval_mode: EvalMode) -> CometResult<Option<i8>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "TINYINT",
+ i8::MIN as i32,
+ i8::MAX as i32,
+ )?
+ .map(|v| v as i8))
+}
+
+fn cast_string_to_i16(str: &str, eval_mode: EvalMode) -> CometResult<Option<i16>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "SMALLINT",
+ i16::MIN as i32,
+ i16::MAX as i32,
+ )?
+ .map(|v| v as i16))
+}
+
+fn cast_string_to_i32(str: &str, eval_mode: EvalMode) -> CometResult<Option<i32>> {
+ do_cast_string_to_int::<i32>(str, eval_mode, "INT", i32::MIN)
+}
+
+fn cast_string_to_i64(str: &str, eval_mode: EvalMode) -> CometResult<Option<i64>> {
+ do_cast_string_to_int::<i64>(str, eval_mode, "BIGINT", i64::MIN)
+}
Review Comment:
@viirya Let me know if there is anything else to address. I have upmerged with latest from main branch so this PR is a little smaller 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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1576919256
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -64,6 +68,25 @@ pub struct Cast {
pub timezone: String,
}
+macro_rules! spark_cast_utf8_to_integral {
+ ($string_array:expr, $eval_mode:expr, $array_type:ty, $cast_method:ident) => {{
+ let mut cast_array = PrimitiveArray::<$array_type>::builder($string_array.len());
+ for i in 0..$string_array.len() {
Review Comment:
I missed that! Thanks
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#issuecomment-2073364294
> > Thanks @andygrove btw I'm wondering if this PR should cover scope with formatting https://spark.apache.org/docs/latest/sql-ref-number-pattern.html#the-to_number-function
>
> Sorry, I'm not sure I understand. You are referring to the error message formatting?
Oh it covers just cast string to integers, I thought `to_number()` is also covered as it has casting behind the scenes
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1576894523
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -103,10 +126,78 @@ impl Cast {
(DataType::LargeUtf8, DataType::Boolean) => {
Self::spark_cast_utf8_to_boolean::<i64>(&array, self.eval_mode)?
}
- _ => cast_with_options(&array, to_type, &CAST_OPTIONS)?,
+ (
+ DataType::Utf8,
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) => Self::spark_cast_string_to_integral(to_type, &array, self.eval_mode)?,
+ (
+ DataType::Dictionary(key_type, value_type),
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) if key_type.as_ref() == &DataType::Int32
Review Comment:
Oh, I see. I think the assumption comes from native scan side where the Parquet dictionary indices is always of integer type so dictionary keys read from native scan is always Int32 type.
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1579646263
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -103,10 +126,78 @@ impl Cast {
(DataType::LargeUtf8, DataType::Boolean) => {
Self::spark_cast_utf8_to_boolean::<i64>(&array, self.eval_mode)?
}
- _ => cast_with_options(&array, to_type, &CAST_OPTIONS)?,
+ (
+ DataType::Utf8,
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) => Self::spark_cast_string_to_integral(to_type, &array, self.eval_mode)?,
+ (
+ DataType::Dictionary(key_type, value_type),
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) if key_type.as_ref() == &DataType::Int32
Review Comment:
> Oh, I see. I think the assumption comes from native scan side where the Parquet dictionary indices is always of integer type so dictionary keys read from native scan is always Int32 type.
Yes that is exactly right.
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1579561300
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -142,6 +226,281 @@ impl Cast {
}
}
+fn cast_string_to_i8(str: &str, eval_mode: EvalMode) -> CometResult<Option<i8>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "TINYINT",
+ i8::MIN as i32,
+ i8::MAX as i32,
+ )?
+ .map(|v| v as i8))
+}
+
+fn cast_string_to_i16(str: &str, eval_mode: EvalMode) -> CometResult<Option<i16>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "SMALLINT",
+ i16::MIN as i32,
+ i16::MAX as i32,
+ )?
+ .map(|v| v as i16))
+}
+
+fn cast_string_to_i32(str: &str, eval_mode: EvalMode) -> CometResult<Option<i32>> {
+ let mut accum = CastStringToInt32::default();
+ do_cast_string_to_int(&mut accum, str, eval_mode, "INT")?;
+ Ok(accum.result)
+}
+
+fn cast_string_to_i64(str: &str, eval_mode: EvalMode) -> CometResult<Option<i64>> {
+ let mut accum = CastStringToInt64::default();
+ do_cast_string_to_int(&mut accum, str, eval_mode, "BIGINT")?;
+ Ok(accum.result)
+}
+
+fn cast_string_to_int_with_range_check(
+ str: &str,
+ eval_mode: EvalMode,
+ type_name: &str,
+ min: i32,
+ max: i32,
+) -> CometResult<Option<i32>> {
+ let mut accum = CastStringToInt32::default();
+ do_cast_string_to_int(&mut accum, str, eval_mode, type_name)?;
+ match accum.result {
+ None => Ok(None),
+ Some(v) if v >= min && v <= max => Ok(Some(v)),
+ _ if eval_mode == EvalMode::Ansi => Err(invalid_value(str, "STRING", type_name)),
+ _ => Ok(None),
+ }
+}
+
+/// We support parsing strings to i32 and i64 to match Spark's logic. Support for i8 and i16 is
+/// implemented by first parsing as i32 and then downcasting. The CastStringToInt trait is
+/// introduced so that we can have the parsing logic delegate either to an i32 or i64 accumulator
+/// and avoid the need to use macros here.
+trait CastStringToInt {
+ fn accumulate(
+ &mut self,
+ eval_mode: EvalMode,
+ type_name: &str,
+ str: &str,
+ digit: u32,
+ ) -> CometResult<()>;
+
+ fn reset(&mut self);
+
+ fn finish(
+ &mut self,
+ eval_mode: EvalMode,
+ type_name: &str,
+ str: &str,
+ negative: bool,
+ ) -> CometResult<()>;
+}
+struct CastStringToInt32 {
+ negative: bool,
+ result: Option<i32>,
+ radix: i32,
+}
+
+impl Default for CastStringToInt32 {
+ fn default() -> Self {
+ Self {
+ negative: false,
+ result: Some(0),
+ radix: 10,
+ }
+ }
+}
+
+impl CastStringToInt for CastStringToInt32 {
+ fn accumulate(
+ &mut self,
+ eval_mode: EvalMode,
+ type_name: &str,
+ str: &str,
+ digit: u32,
+ ) -> CometResult<()> {
+ if self.result.is_some() && self.result.unwrap() < i32::MIN / self.radix {
+ self.reset();
+ return none_or_err(eval_mode, type_name, str);
+ }
+ self.result = Some(self.result.unwrap_or(0) * self.radix - digit as i32);
+ if self.result.unwrap() > 0 {
+ self.reset();
+ return none_or_err(eval_mode, type_name, str);
+ }
+ Ok(())
+ }
+ fn reset(&mut self) {
+ self.result = None;
+ }
+
+ fn finish(
+ &mut self,
+ eval_mode: EvalMode,
+ type_name: &str,
+ str: &str,
+ negative: bool,
+ ) -> CometResult<()> {
+ if self.result.is_some() && !negative {
+ self.result = Some(-self.result.unwrap());
+ if self.result.unwrap() < 0 {
+ return none_or_err(eval_mode, type_name, str);
+ }
+ }
+ Ok(())
+ }
+}
+
+struct CastStringToInt64 {
+ negative: bool,
+ result: Option<i64>,
+ radix: i64,
+}
+
+impl Default for CastStringToInt64 {
+ fn default() -> Self {
+ Self {
+ negative: false,
+ result: Some(0),
+ radix: 10,
+ }
+ }
+}
+
+impl CastStringToInt for CastStringToInt64 {
+ fn accumulate(
+ &mut self,
+ eval_mode: EvalMode,
+ type_name: &str,
+ str: &str,
+ digit: u32,
+ ) -> CometResult<()> {
+ if self.result.unwrap_or(0) < i64::MIN / self.radix {
+ self.reset();
+ return none_or_err(eval_mode, type_name, str);
+ }
+ self.result = Some(self.result.unwrap_or(0) * self.radix - digit as i64);
+ if self.result.unwrap() > 0 {
+ self.reset();
+ return none_or_err(eval_mode, type_name, str);
+ }
+ Ok(())
+ }
+
+ fn reset(&mut self) {
+ self.result = None;
+ }
+
+ fn finish(
+ &mut self,
+ eval_mode: EvalMode,
+ type_name: &str,
+ str: &str,
+ negative: bool,
+ ) -> CometResult<()> {
+ if self.result.is_some() && !negative {
+ self.result = Some(-self.result.unwrap());
+ if self.result.unwrap() < 0 {
+ return none_or_err(eval_mode, type_name, str);
+ }
+ }
+ Ok(())
+ }
+}
+
+fn do_cast_string_to_int(
+ accumulator: &mut dyn CastStringToInt,
+ str: &str,
+ eval_mode: EvalMode,
+ type_name: &str,
+) -> CometResult<()> {
+ let chars: Vec<char> = str.chars().collect();
+ let mut i = 0;
+ let mut end = chars.len();
+
+ // skip leading whitespace
+ while i < end && chars[i].is_whitespace() {
+ i += 1;
+ }
+
+ // skip trailing whitespace
+ while end > i && chars[end - 1].is_whitespace() {
+ end -= 1;
+ }
+
+ // check for empty string
+ if i == end {
+ accumulator.reset();
+ return Ok(());
+ }
+
+ // skip + or -
+ let negative = chars[0] == '-';
+ if negative || chars[0] == '+' {
Review Comment:
Thank you! The code was originally trimming the string before this point and I missed updating this when I removed the trim. I have now fixed this.
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#issuecomment-2082911871
> Thanks for your effort @andygrove, the new code is well crafted.
Thank you for the thorough review @advancedxy!
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1581740218
##########
spark/src/test/scala/org/apache/comet/CometCastSuite.scala:
##########
@@ -66,20 +71,65 @@ class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper {
castTest(testValues, DataTypes.BooleanType)
}
- ignore("cast string to byte") {
- castTest(generateStrings(numericPattern, 8).toDF("a"), DataTypes.ByteType)
- }
-
- ignore("cast string to short") {
- castTest(generateStrings(numericPattern, 8).toDF("a"), DataTypes.ShortType)
- }
-
- ignore("cast string to int") {
- castTest(generateStrings(numericPattern, 8).toDF("a"), DataTypes.IntegerType)
- }
-
- ignore("cast string to long") {
- castTest(generateStrings(numericPattern, 8).toDF("a"), DataTypes.LongType)
+ private val castStringToIntegralInputs: Seq[String] = Seq(
+ "",
+ ".",
+ "+",
+ "-",
+ "+.",
+ "-.",
+ "-0",
+ "+1",
+ "-1",
+ ".2",
+ "-.2",
+ "1e1",
+ "1.1d",
+ "1.1f",
+ Byte.MinValue.toString,
+ (Byte.MinValue.toShort - 1).toString,
+ Byte.MaxValue.toString,
+ (Byte.MaxValue.toShort + 1).toString,
+ Short.MinValue.toString,
+ (Short.MinValue.toInt - 1).toString,
+ Short.MaxValue.toString,
+ (Short.MaxValue.toInt + 1).toString,
+ Int.MinValue.toString,
+ (Int.MinValue.toLong - 1).toString,
+ Int.MaxValue.toString,
+ (Int.MaxValue.toLong + 1).toString,
+ Long.MinValue.toString,
+ Long.MaxValue.toString,
+ "-9223372036854775809", // Long.MinValue -1
+ "9223372036854775808" // Long.MaxValue + 1
+ )
+
+ test("cast string to byte") {
+ // test with hand-picked values
+ castTest(castStringToIntegralInputs.toDF("a"), DataTypes.ByteType)
+ // fuzz test
+ castTest(generateStrings(numericPattern, 5).toDF("a"), DataTypes.ByteType)
+ }
+
+ test("cast string to short") {
+ // test with hand-picked values
+ castTest(castStringToIntegralInputs.toDF("a"), DataTypes.ByteType)
Review Comment:
```suggestion
castTest(castStringToIntegralInputs.toDF("a"), DataTypes.ShortType)
```
##########
spark/src/test/scala/org/apache/comet/CometCastSuite.scala:
##########
@@ -66,20 +71,65 @@ class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper {
castTest(testValues, DataTypes.BooleanType)
}
- ignore("cast string to byte") {
- castTest(generateStrings(numericPattern, 8).toDF("a"), DataTypes.ByteType)
- }
-
- ignore("cast string to short") {
- castTest(generateStrings(numericPattern, 8).toDF("a"), DataTypes.ShortType)
- }
-
- ignore("cast string to int") {
- castTest(generateStrings(numericPattern, 8).toDF("a"), DataTypes.IntegerType)
- }
-
- ignore("cast string to long") {
- castTest(generateStrings(numericPattern, 8).toDF("a"), DataTypes.LongType)
+ private val castStringToIntegralInputs: Seq[String] = Seq(
+ "",
+ ".",
+ "+",
+ "-",
+ "+.",
+ "-.",
+ "-0",
+ "+1",
+ "-1",
+ ".2",
+ "-.2",
+ "1e1",
+ "1.1d",
+ "1.1f",
+ Byte.MinValue.toString,
+ (Byte.MinValue.toShort - 1).toString,
+ Byte.MaxValue.toString,
+ (Byte.MaxValue.toShort + 1).toString,
+ Short.MinValue.toString,
+ (Short.MinValue.toInt - 1).toString,
+ Short.MaxValue.toString,
+ (Short.MaxValue.toInt + 1).toString,
+ Int.MinValue.toString,
+ (Int.MinValue.toLong - 1).toString,
+ Int.MaxValue.toString,
+ (Int.MaxValue.toLong + 1).toString,
+ Long.MinValue.toString,
+ Long.MaxValue.toString,
+ "-9223372036854775809", // Long.MinValue -1
+ "9223372036854775808" // Long.MaxValue + 1
+ )
+
+ test("cast string to byte") {
+ // test with hand-picked values
+ castTest(castStringToIntegralInputs.toDF("a"), DataTypes.ByteType)
+ // fuzz test
+ castTest(generateStrings(numericPattern, 5).toDF("a"), DataTypes.ByteType)
+ }
+
+ test("cast string to short") {
+ // test with hand-picked values
+ castTest(castStringToIntegralInputs.toDF("a"), DataTypes.ByteType)
+ // fuzz test
+ castTest(generateStrings(numericPattern, 5).toDF("a"), DataTypes.ByteType)
Review Comment:
```suggestion
castTest(generateStrings(numericPattern, 5).toDF("a"), DataTypes.ShortType)
```
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1581836049
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -142,6 +226,311 @@ impl Cast {
}
}
+fn cast_string_to_i8(str: &str, eval_mode: EvalMode) -> CometResult<Option<i8>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "TINYINT",
+ i8::MIN as i32,
+ i8::MAX as i32,
+ )?
+ .map(|v| v as i8))
+}
+
+fn cast_string_to_i16(str: &str, eval_mode: EvalMode) -> CometResult<Option<i16>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "SMALLINT",
+ i16::MIN as i32,
+ i16::MAX as i32,
+ )?
+ .map(|v| v as i16))
+}
+
+fn cast_string_to_i32(str: &str, eval_mode: EvalMode) -> CometResult<Option<i32>> {
+ let mut accum = CastStringToInt32::default();
+ do_cast_string_to_int(&mut accum, str, eval_mode, "INT")?;
+ Ok(accum.result)
+}
+
+fn cast_string_to_i64(str: &str, eval_mode: EvalMode) -> CometResult<Option<i64>> {
+ let mut accum = CastStringToInt64::default();
+ do_cast_string_to_int(&mut accum, str, eval_mode, "BIGINT")?;
+ Ok(accum.result)
+}
+
+fn cast_string_to_int_with_range_check(
+ str: &str,
+ eval_mode: EvalMode,
+ type_name: &str,
+ min: i32,
+ max: i32,
+) -> CometResult<Option<i32>> {
+ let mut accum = CastStringToInt32::default();
+ do_cast_string_to_int(&mut accum, str, eval_mode, type_name)?;
+ match accum.result {
+ None => Ok(None),
+ Some(v) if v >= min && v <= max => Ok(Some(v)),
+ _ if eval_mode == EvalMode::Ansi => Err(invalid_value(str, "STRING", type_name)),
+ _ => Ok(None),
+ }
+}
+
+/// We support parsing strings to i32 and i64 to match Spark's logic. Support for i8 and i16 is
+/// implemented by first parsing as i32 and then downcasting. The CastStringToInt trait is
+/// introduced so that we can have the parsing logic delegate either to an i32 or i64 accumulator
+/// and avoid the need to use macros here.
+trait CastStringToInt {
+ fn accumulate(
+ &mut self,
+ eval_mode: EvalMode,
+ type_name: &str,
+ str: &str,
+ digit: u32,
+ ) -> CometResult<()>;
+
+ fn reset(&mut self);
+
+ fn finish(
+ &mut self,
+ eval_mode: EvalMode,
+ type_name: &str,
+ str: &str,
+ negative: bool,
+ ) -> CometResult<()>;
+}
+struct CastStringToInt32 {
+ negative: bool,
+ result: Option<i32>,
+ radix: i32,
+}
+
+impl Default for CastStringToInt32 {
+ fn default() -> Self {
+ Self {
+ negative: false,
+ result: Some(0),
+ radix: 10,
+ }
+ }
+}
+
+impl CastStringToInt for CastStringToInt32 {
+ fn accumulate(
+ &mut self,
+ eval_mode: EvalMode,
+ type_name: &str,
+ str: &str,
+ digit: u32,
+ ) -> CometResult<()> {
+ // We are going to process the new digit and accumulate the result. However, before doing
+ // this, if the result is already smaller than the stopValue(Integer.MIN_VALUE / radix),
+ // then result * 10 will definitely be smaller than minValue, and we can stop
+ if let Some(r) = self.result {
+ let stop_value = i32::MIN / self.radix;
+ if r < stop_value {
+ self.reset();
+ return none_or_err(eval_mode, type_name, str);
+ }
+ }
+ // Since the previous result is less than or equal to stopValue(Integer.MIN_VALUE / radix),
+ // we can just use `result > 0` to check overflow. If result overflows, we should stop
+ let v = self.result.unwrap_or(0) * self.radix;
+ match v.checked_sub(digit as i32) {
+ Some(x) if x <= 0 => self.result = Some(x),
+ _ => {
+ self.reset();
+ return none_or_err(eval_mode, type_name, str);
+ }
+ }
+ Ok(())
+ }
+ fn reset(&mut self) {
+ self.result = None;
+ }
+
+ fn finish(
+ &mut self,
+ eval_mode: EvalMode,
+ type_name: &str,
+ str: &str,
+ negative: bool,
+ ) -> CometResult<()> {
+ if !negative {
+ if let Some(r) = self.result {
+ let negated = r.checked_neg().unwrap_or(-1);
+ if negated < 0 {
+ self.reset();
+ return none_or_err(eval_mode, type_name, str);
+ }
+ self.result = Some(negated);
+ }
+ }
+ Ok(())
+ }
+}
+
+struct CastStringToInt64 {
+ negative: bool,
+ result: Option<i64>,
+ radix: i64,
+}
+
+impl Default for CastStringToInt64 {
+ fn default() -> Self {
+ Self {
+ negative: false,
+ result: Some(0),
+ radix: 10,
+ }
+ }
+}
+
+impl CastStringToInt for CastStringToInt64 {
+ fn accumulate(
+ &mut self,
+ eval_mode: EvalMode,
+ type_name: &str,
+ str: &str,
+ digit: u32,
+ ) -> CometResult<()> {
Review Comment:
I think that the code is already correct in the case that result is `None`. The first section has `if let Some(r) = self.result` and the second section has `self.result.unwrap_or(0)`.
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1585729713
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -142,6 +230,193 @@ impl Cast {
}
}
+fn cast_string_to_i8(str: &str, eval_mode: EvalMode) -> CometResult<Option<i8>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "TINYINT",
+ i8::MIN as i32,
+ i8::MAX as i32,
+ )?
+ .map(|v| v as i8))
+}
+
+fn cast_string_to_i16(str: &str, eval_mode: EvalMode) -> CometResult<Option<i16>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "SMALLINT",
+ i16::MIN as i32,
+ i16::MAX as i32,
+ )?
+ .map(|v| v as i16))
+}
+
+fn cast_string_to_i32(str: &str, eval_mode: EvalMode) -> CometResult<Option<i32>> {
+ do_cast_string_to_int::<i32>(str, eval_mode, "INT", i32::MIN)
+}
+
+fn cast_string_to_i64(str: &str, eval_mode: EvalMode) -> CometResult<Option<i64>> {
+ do_cast_string_to_int::<i64>(str, eval_mode, "BIGINT", i64::MIN)
+}
Review Comment:
I will go to look at this again tonight.
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1576814855
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -64,6 +68,25 @@ pub struct Cast {
pub timezone: String,
}
+macro_rules! spark_cast_utf8_to_integral {
+ ($string_array:expr, $eval_mode:expr, $array_type:ty, $cast_method:ident) => {{
+ let mut cast_array = PrimitiveArray::<$array_type>::builder($string_array.len());
+ for i in 0..$string_array.len() {
Review Comment:
I think that `$string_array.len()` is already only computed once?
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1576797068
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -103,10 +126,78 @@ impl Cast {
(DataType::LargeUtf8, DataType::Boolean) => {
Self::spark_cast_utf8_to_boolean::<i64>(&array, self.eval_mode)?
}
- _ => cast_with_options(&array, to_type, &CAST_OPTIONS)?,
+ (
+ DataType::Utf8,
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) => Self::spark_cast_string_to_integral(to_type, &array, self.eval_mode)?,
+ (
+ DataType::Dictionary(key_type, value_type),
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) if key_type.as_ref() == &DataType::Int32
+ && value_type.as_ref() == &DataType::Utf8 =>
+ {
+ // Note that we are unpacking a dictionary-encoded array and then performing
+ // the cast. We could potentially improve performance here by casting the
+ // dictionary values directly without unpacking the array first, although this
+ // would add more complexity to the code
+ let unpacked_array = Self::unpack_dict_string_array::<Int32Type>(&array)?;
+ Self::spark_cast_string_to_integral(to_type, &unpacked_array, self.eval_mode)?
+ }
+ _ => {
+ // when we have no Spark-specific casting we delegate to DataFusion
+ cast_with_options(&array, to_type, &CAST_OPTIONS)?
+ }
+ };
+ Ok(spark_cast(cast_result, from_type, to_type))
+ }
+
+ fn spark_cast_string_to_integral(
+ to_type: &DataType,
+ array: &ArrayRef,
+ eval_mode: EvalMode,
+ ) -> CometResult<ArrayRef> {
+ let string_array = array
+ .as_any()
+ .downcast_ref::<GenericStringArray<i32>>()
+ .expect("spark_cast_string_to_integral expected a string array");
+
+ let cast_array: ArrayRef = match to_type {
+ DataType::Int8 => {
+ spark_cast_utf8_to_integral!(string_array, eval_mode, Int8Type, cast_string_to_i8)?
+ }
+ DataType::Int16 => spark_cast_utf8_to_integral!(
+ string_array,
+ eval_mode,
+ Int16Type,
+ cast_string_to_i16
+ )?,
+ DataType::Int32 => spark_cast_utf8_to_integral!(
+ string_array,
+ eval_mode,
+ Int32Type,
+ cast_string_to_i32
+ )?,
+ DataType::Int64 => spark_cast_utf8_to_integral!(
+ string_array,
+ eval_mode,
+ Int64Type,
+ cast_string_to_i64
+ )?,
+ _ => unreachable!("invalid integral type in cast from string"),
Review Comment:
```suggestion
dt => unreachable!(format!("invalid integral type {dt} in cast from 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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1576792898
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -103,10 +126,78 @@ impl Cast {
(DataType::LargeUtf8, DataType::Boolean) => {
Self::spark_cast_utf8_to_boolean::<i64>(&array, self.eval_mode)?
}
- _ => cast_with_options(&array, to_type, &CAST_OPTIONS)?,
+ (
+ DataType::Utf8,
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) => Self::spark_cast_string_to_integral(to_type, &array, self.eval_mode)?,
+ (
+ DataType::Dictionary(key_type, value_type),
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) if key_type.as_ref() == &DataType::Int32
Review Comment:
@viirya do you know if dictionary keys will always be i32?
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1576794138
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -64,6 +68,25 @@ pub struct Cast {
pub timezone: String,
}
+macro_rules! spark_cast_utf8_to_integral {
Review Comment:
maybe `utf8_to_integer`?
spark not involved in native exec, not sure why spark is needed.
Integral type also includes booleans and this scope limited by integers 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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1580453298
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -142,6 +226,311 @@ impl Cast {
}
}
+fn cast_string_to_i8(str: &str, eval_mode: EvalMode) -> CometResult<Option<i8>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "TINYINT",
+ i8::MIN as i32,
+ i8::MAX as i32,
+ )?
+ .map(|v| v as i8))
+}
+
+fn cast_string_to_i16(str: &str, eval_mode: EvalMode) -> CometResult<Option<i16>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "SMALLINT",
+ i16::MIN as i32,
+ i16::MAX as i32,
+ )?
+ .map(|v| v as i16))
+}
+
+fn cast_string_to_i32(str: &str, eval_mode: EvalMode) -> CometResult<Option<i32>> {
+ let mut accum = CastStringToInt32::default();
+ do_cast_string_to_int(&mut accum, str, eval_mode, "INT")?;
+ Ok(accum.result)
+}
+
+fn cast_string_to_i64(str: &str, eval_mode: EvalMode) -> CometResult<Option<i64>> {
+ let mut accum = CastStringToInt64::default();
+ do_cast_string_to_int(&mut accum, str, eval_mode, "BIGINT")?;
+ Ok(accum.result)
+}
+
+fn cast_string_to_int_with_range_check(
+ str: &str,
+ eval_mode: EvalMode,
+ type_name: &str,
+ min: i32,
+ max: i32,
+) -> CometResult<Option<i32>> {
+ let mut accum = CastStringToInt32::default();
+ do_cast_string_to_int(&mut accum, str, eval_mode, type_name)?;
+ match accum.result {
+ None => Ok(None),
+ Some(v) if v >= min && v <= max => Ok(Some(v)),
+ _ if eval_mode == EvalMode::Ansi => Err(invalid_value(str, "STRING", type_name)),
+ _ => Ok(None),
+ }
+}
+
+/// We support parsing strings to i32 and i64 to match Spark's logic. Support for i8 and i16 is
+/// implemented by first parsing as i32 and then downcasting. The CastStringToInt trait is
+/// introduced so that we can have the parsing logic delegate either to an i32 or i64 accumulator
+/// and avoid the need to use macros here.
+trait CastStringToInt {
+ fn accumulate(
+ &mut self,
+ eval_mode: EvalMode,
+ type_name: &str,
+ str: &str,
+ digit: u32,
+ ) -> CometResult<()>;
+
+ fn reset(&mut self);
+
+ fn finish(
+ &mut self,
+ eval_mode: EvalMode,
+ type_name: &str,
+ str: &str,
+ negative: bool,
+ ) -> CometResult<()>;
+}
+struct CastStringToInt32 {
+ negative: bool,
+ result: Option<i32>,
+ radix: i32,
+}
+
+impl Default for CastStringToInt32 {
+ fn default() -> Self {
+ Self {
+ negative: false,
+ result: Some(0),
+ radix: 10,
+ }
+ }
+}
+
+impl CastStringToInt for CastStringToInt32 {
+ fn accumulate(
+ &mut self,
+ eval_mode: EvalMode,
+ type_name: &str,
+ str: &str,
+ digit: u32,
+ ) -> CometResult<()> {
+ // We are going to process the new digit and accumulate the result. However, before doing
+ // this, if the result is already smaller than the stopValue(Integer.MIN_VALUE / radix),
+ // then result * 10 will definitely be smaller than minValue, and we can stop
+ if let Some(r) = self.result {
+ let stop_value = i32::MIN / self.radix;
+ if r < stop_value {
+ self.reset();
+ return none_or_err(eval_mode, type_name, str);
+ }
+ }
+ // Since the previous result is less than or equal to stopValue(Integer.MIN_VALUE / radix),
+ // we can just use `result > 0` to check overflow. If result overflows, we should stop
+ let v = self.result.unwrap_or(0) * self.radix;
+ match v.checked_sub(digit as i32) {
+ Some(x) if x <= 0 => self.result = Some(x),
+ _ => {
+ self.reset();
+ return none_or_err(eval_mode, type_name, str);
+ }
+ }
+ Ok(())
+ }
+ fn reset(&mut self) {
+ self.result = None;
+ }
+
+ fn finish(
+ &mut self,
+ eval_mode: EvalMode,
+ type_name: &str,
+ str: &str,
+ negative: bool,
+ ) -> CometResult<()> {
+ if !negative {
+ if let Some(r) = self.result {
+ let negated = r.checked_neg().unwrap_or(-1);
+ if negated < 0 {
+ self.reset();
+ return none_or_err(eval_mode, type_name, str);
+ }
+ self.result = Some(negated);
+ }
+ }
+ Ok(())
+ }
+}
+
+struct CastStringToInt64 {
+ negative: bool,
+ result: Option<i64>,
+ radix: i64,
+}
+
+impl Default for CastStringToInt64 {
+ fn default() -> Self {
+ Self {
+ negative: false,
+ result: Some(0),
+ radix: 10,
+ }
+ }
+}
+
+impl CastStringToInt for CastStringToInt64 {
+ fn accumulate(
+ &mut self,
+ eval_mode: EvalMode,
+ type_name: &str,
+ str: &str,
+ digit: u32,
+ ) -> CometResult<()> {
Review Comment:
do we need to check the `result` is None first as it may already been reset?
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -142,6 +226,311 @@ impl Cast {
}
}
+fn cast_string_to_i8(str: &str, eval_mode: EvalMode) -> CometResult<Option<i8>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "TINYINT",
+ i8::MIN as i32,
+ i8::MAX as i32,
+ )?
+ .map(|v| v as i8))
+}
+
+fn cast_string_to_i16(str: &str, eval_mode: EvalMode) -> CometResult<Option<i16>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "SMALLINT",
+ i16::MIN as i32,
+ i16::MAX as i32,
+ )?
+ .map(|v| v as i16))
+}
+
+fn cast_string_to_i32(str: &str, eval_mode: EvalMode) -> CometResult<Option<i32>> {
+ let mut accum = CastStringToInt32::default();
+ do_cast_string_to_int(&mut accum, str, eval_mode, "INT")?;
+ Ok(accum.result)
+}
+
+fn cast_string_to_i64(str: &str, eval_mode: EvalMode) -> CometResult<Option<i64>> {
+ let mut accum = CastStringToInt64::default();
+ do_cast_string_to_int(&mut accum, str, eval_mode, "BIGINT")?;
+ Ok(accum.result)
+}
+
+fn cast_string_to_int_with_range_check(
+ str: &str,
+ eval_mode: EvalMode,
+ type_name: &str,
+ min: i32,
+ max: i32,
+) -> CometResult<Option<i32>> {
+ let mut accum = CastStringToInt32::default();
Review Comment:
I'm not very familiar with Rust... Will this bring a lot of object allocation as a new accum is allocated per input?
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1581001480
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -142,6 +226,311 @@ impl Cast {
}
}
+fn cast_string_to_i8(str: &str, eval_mode: EvalMode) -> CometResult<Option<i8>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "TINYINT",
+ i8::MIN as i32,
+ i8::MAX as i32,
+ )?
+ .map(|v| v as i8))
+}
+
+fn cast_string_to_i16(str: &str, eval_mode: EvalMode) -> CometResult<Option<i16>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "SMALLINT",
+ i16::MIN as i32,
+ i16::MAX as i32,
+ )?
+ .map(|v| v as i16))
+}
+
+fn cast_string_to_i32(str: &str, eval_mode: EvalMode) -> CometResult<Option<i32>> {
+ let mut accum = CastStringToInt32::default();
+ do_cast_string_to_int(&mut accum, str, eval_mode, "INT")?;
+ Ok(accum.result)
+}
+
+fn cast_string_to_i64(str: &str, eval_mode: EvalMode) -> CometResult<Option<i64>> {
+ let mut accum = CastStringToInt64::default();
+ do_cast_string_to_int(&mut accum, str, eval_mode, "BIGINT")?;
+ Ok(accum.result)
+}
+
+fn cast_string_to_int_with_range_check(
+ str: &str,
+ eval_mode: EvalMode,
+ type_name: &str,
+ min: i32,
+ max: i32,
+) -> CometResult<Option<i32>> {
+ let mut accum = CastStringToInt32::default();
Review Comment:
That is a good point. I don't know either. I will file a follow-up issue for benchmarking this vs Spark and looking into optimizations. This PR at least fixes the current correctness issue.
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1581740802
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -103,10 +125,72 @@ impl Cast {
(DataType::LargeUtf8, DataType::Boolean) => {
Self::spark_cast_utf8_to_boolean::<i64>(&array, self.eval_mode)?
}
- _ => cast_with_options(&array, to_type, &CAST_OPTIONS)?,
+ (
+ DataType::Utf8,
Review Comment:
Should we also handle LargeUtf8?
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1581876329
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -103,10 +125,72 @@ impl Cast {
(DataType::LargeUtf8, DataType::Boolean) => {
Self::spark_cast_utf8_to_boolean::<i64>(&array, self.eval_mode)?
}
- _ => cast_with_options(&array, to_type, &CAST_OPTIONS)?,
+ (
+ DataType::Utf8,
Review Comment:
I have added this, but there are no tests for it. Do we have a way to test this?
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1583665422
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -64,6 +68,24 @@ pub struct Cast {
pub timezone: String,
}
+macro_rules! cast_utf8_to_int {
+ ($array:expr, $eval_mode:expr, $array_type:ty, $cast_method:ident) => {{
+ let len = $array.len();
+ let mut cast_array = PrimitiveArray::<$array_type>::builder(len);
+ for i in 0..len {
+ if $array.is_null(i) {
Review Comment:
maybe it can be simplified to
```
if let Some(cast_value) = $cast_method($array.value(i).trim(), $eval_mode)? {
cast_array.append_value(cast_value);
} else {
cast_array.append_null()
}
```
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1583736840
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -142,6 +230,193 @@ impl Cast {
}
}
+fn cast_string_to_i8(str: &str, eval_mode: EvalMode) -> CometResult<Option<i8>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "TINYINT",
+ i8::MIN as i32,
+ i8::MAX as i32,
+ )?
+ .map(|v| v as i8))
+}
+
+fn cast_string_to_i16(str: &str, eval_mode: EvalMode) -> CometResult<Option<i16>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "SMALLINT",
+ i16::MIN as i32,
+ i16::MAX as i32,
+ )?
+ .map(|v| v as i16))
+}
+
+fn cast_string_to_i32(str: &str, eval_mode: EvalMode) -> CometResult<Option<i32>> {
+ do_cast_string_to_int::<i32>(str, eval_mode, "INT", i32::MIN)
+}
+
+fn cast_string_to_i64(str: &str, eval_mode: EvalMode) -> CometResult<Option<i64>> {
+ do_cast_string_to_int::<i64>(str, eval_mode, "BIGINT", i64::MIN)
+}
+
+fn cast_string_to_int_with_range_check(
+ str: &str,
+ eval_mode: EvalMode,
+ type_name: &str,
+ min: i32,
+ max: i32,
+) -> CometResult<Option<i32>> {
+ match do_cast_string_to_int(str, eval_mode, type_name, i32::MIN)? {
+ None => Ok(None),
+ Some(v) if v >= min && v <= max => Ok(Some(v)),
+ _ if eval_mode == EvalMode::Ansi => Err(invalid_value(str, "STRING", type_name)),
+ _ => Ok(None),
+ }
+}
+
+#[derive(PartialEq)]
+enum State {
+ SkipLeadingWhiteSpace,
+ SkipTrailingWhiteSpace,
+ ParseSignAndDigits,
+ ParseFractionalDigits,
+}
+
+fn do_cast_string_to_int<
+ T: Num + PartialOrd + Integer + CheckedSub + CheckedNeg + From<i32> + Copy,
+>(
+ str: &str,
+ eval_mode: EvalMode,
+ type_name: &str,
+ min_value: T,
+) -> CometResult<Option<T>> {
+ let len = str.len();
+ if len == 0 {
+ return none_or_err(eval_mode, type_name, str);
+ }
+
+ let mut result: T = T::zero();
+ let mut negative = false;
+ let radix = T::from(10);
+ let stop_value = min_value / radix;
+ let mut state = State::SkipLeadingWhiteSpace;
+ let mut parsed_sign = false;
+
+ for (i, ch) in str.char_indices() {
+ // skip leading whitespace
+ if state == State::SkipLeadingWhiteSpace {
+ if ch.is_whitespace() {
+ // consume this char
+ continue;
+ }
+ // change state and fall through to next section
+ state = State::ParseSignAndDigits;
+ }
+
+ if state == State::ParseSignAndDigits {
+ if !parsed_sign {
+ negative = ch == '-';
+ let positive = ch == '+';
+ parsed_sign = true;
+ if negative || positive {
+ if i + 1 == len {
+ // input string is just "+" or "-"
+ return none_or_err(eval_mode, type_name, str);
+ }
+ // consume this char
+ continue;
+ }
+ }
+
+ if ch == '.' {
+ if eval_mode == EvalMode::Legacy {
+ // truncate decimal in legacy mode
+ state = State::ParseFractionalDigits;
+ continue;
+ } else {
+ return none_or_err(eval_mode, type_name, str);
+ }
+ }
+
+ let digit = if ch.is_ascii_digit() {
+ (ch as u32) - ('0' as u32)
+ } else {
+ return none_or_err(eval_mode, type_name, str);
+ };
+
+ // We are going to process the new digit and accumulate the result. However, before
+ // doing this, if the result is already smaller than the
+ // stopValue(Integer.MIN_VALUE / radix), then result * 10 will definitely be
+ // smaller than minValue, and we can stop
+ if result < stop_value {
+ return none_or_err(eval_mode, type_name, str);
+ }
+
+ // Since the previous result is less than or equal to stopValue(Integer.MIN_VALUE /
+ // radix), we can just use `result > 0` to check overflow. If result
+ // overflows, we should stop
+ let v = result * radix;
+ let digit = (digit as i32).into();
+ match v.checked_sub(&digit) {
+ Some(x) if x <= T::zero() => result = x,
+ _ => {
+ return none_or_err(eval_mode, type_name, str);
+ }
+ }
+ }
+
+ if state == State::ParseFractionalDigits {
+ // This is the case when we've encountered a decimal separator. The fractional
+ // part will not change the number, but we will verify that the fractional part
+ // is well-formed.
+ if ch.is_whitespace() {
+ // finished parsing fractional digits, now need to skip trailing whitespace
+ state = State::SkipTrailingWhiteSpace;
+ // consume this char
+ continue;
+ }
+ if !ch.is_ascii_digit() {
+ return none_or_err(eval_mode, type_name, str);
+ }
+ }
+
+ // skip trailing whitespace
+ if state == State::SkipTrailingWhiteSpace && !ch.is_whitespace() {
+ return none_or_err(eval_mode, type_name, str);
+ }
+ }
+
+ if !negative {
+ if let Some(neg) = result.checked_neg() {
+ if neg < T::zero() {
+ return none_or_err(eval_mode, type_name, str);
+ }
+ result = neg;
+ } else {
+ return none_or_err(eval_mode, type_name, str);
+ }
+ }
+
+ Ok(Some(result))
+}
+
+/// Either return Ok(None) or Err(CometError::CastInvalidValue) depending on the evaluation mode
+fn none_or_err<T>(eval_mode: EvalMode, type_name: &str, str: &str) -> CometResult<Option<T>> {
+ match eval_mode {
+ EvalMode::Ansi => Err(invalid_value(str, "STRING", type_name)),
+ _ => Ok(None),
+ }
+}
+
+fn invalid_value(value: &str, from_type: &str, to_type: &str) -> CometError {
+ CometError::CastInvalidValue {
+ value: value.to_string(),
+ from_type: from_type.to_string(),
+ to_type: to_type.to_string(),
+ }
+}
Review Comment:
I think these can be `inline` function?
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#issuecomment-2077582671
@parthchandra @advancedxy @viirya I have improved the tests and fixed some more issues. Please take a look when you get a chance.
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1576894815
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -103,10 +126,78 @@ impl Cast {
(DataType::LargeUtf8, DataType::Boolean) => {
Self::spark_cast_utf8_to_boolean::<i64>(&array, self.eval_mode)?
}
- _ => cast_with_options(&array, to_type, &CAST_OPTIONS)?,
+ (
+ DataType::Utf8,
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) => Self::spark_cast_string_to_integral(to_type, &array, self.eval_mode)?,
+ (
+ DataType::Dictionary(key_type, value_type),
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) if key_type.as_ref() == &DataType::Int32
Review Comment:
You can check the `DictDecoder` in native scan implementation.
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1576823136
##########
spark/src/test/scala/org/apache/comet/CometCastSuite.scala:
##########
@@ -66,19 +66,22 @@ class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper {
castTest(testValues, DataTypes.BooleanType)
}
- ignore("cast string to byte") {
- castTest(generateStrings(numericPattern, 8).toDF("a"), DataTypes.ByteType)
+ test("cast string to byte") {
+ val testValues =
+ Seq("", ".", "0", "-0", "+1", "-1", ".2", "-.2", "1e1", "127", "128", "-128", "-129") ++
+ generateStrings(numericPattern, 8)
+ castTest(testValues.toDF("a"), DataTypes.ByteType)
}
- ignore("cast string to short") {
+ test("cast string to short") {
Review Comment:
The fuzz testing does generate many invalid inputs. I can add some more explicit ones to these tests, though.
`cast(".")` will yield different results depending on the eval mode:
- `LEGACY` -> `0`
- `TRY` -> null
- `ANSI` -> error
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#issuecomment-2073322973
> Thanks @andygrove btw I'm wondering if this PR should cover scope with formatting https://spark.apache.org/docs/latest/sql-ref-number-pattern.html#the-to_number-function
Sorry, I'm not sure I understand. You are referring to the error message formatting?
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1576888006
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -103,10 +126,78 @@ impl Cast {
(DataType::LargeUtf8, DataType::Boolean) => {
Self::spark_cast_utf8_to_boolean::<i64>(&array, self.eval_mode)?
}
- _ => cast_with_options(&array, to_type, &CAST_OPTIONS)?,
+ (
+ DataType::Utf8,
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) => Self::spark_cast_string_to_integral(to_type, &array, self.eval_mode)?,
+ (
+ DataType::Dictionary(key_type, value_type),
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) if key_type.as_ref() == &DataType::Int32
Review Comment:
I remember in many places in native code, we assume that dictionary keys are always Int32 type.
But I forgot that where we make such assumption. 😅
cc @sunchao Do you remember that?
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#issuecomment-2072666692
I am now working on refactoring to reduce code duplication by leveraging macros/generics.
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1581741614
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -103,10 +125,72 @@ impl Cast {
(DataType::LargeUtf8, DataType::Boolean) => {
Self::spark_cast_utf8_to_boolean::<i64>(&array, self.eval_mode)?
}
- _ => cast_with_options(&array, to_type, &CAST_OPTIONS)?,
+ (
+ DataType::Utf8,
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) => Self::cast_string_to_int(to_type, &array, self.eval_mode)?,
+ (
+ DataType::Dictionary(key_type, value_type),
+ DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64,
+ ) if key_type.as_ref() == &DataType::Int32
+ && value_type.as_ref() == &DataType::Utf8 =>
+ {
+ // TODO: we are unpacking a dictionary-encoded array and then performing
+ // the cast. We could potentially improve performance here by casting the
+ // dictionary values directly without unpacking the array first, although this
+ // would add more complexity to the code
+ let unpacked_array = Self::unpack_dict_string_array::<Int32Type>(&array)?;
+ Self::cast_string_to_int(to_type, &unpacked_array, self.eval_mode)?
+ }
+ _ => {
+ // when we have no Spark-specific casting we delegate to DataFusion
+ cast_with_options(&array, to_type, &CAST_OPTIONS)?
+ }
};
- let result = spark_cast(cast_result, from_type, to_type);
- Ok(result)
+ Ok(spark_cast(cast_result, from_type, to_type))
+ }
+
+ fn cast_string_to_int(
+ to_type: &DataType,
+ array: &ArrayRef,
+ eval_mode: EvalMode,
+ ) -> CometResult<ArrayRef> {
+ let string_array = array
+ .as_any()
+ .downcast_ref::<GenericStringArray<i32>>()
+ .expect("cast_string_to_int expected a string array");
+
+ let cast_array: ArrayRef = match to_type {
+ DataType::Int8 => {
+ cast_utf8_to_int!(string_array, eval_mode, Int8Type, cast_string_to_i8)?
+ }
+ DataType::Int16 => {
+ cast_utf8_to_int!(string_array, eval_mode, Int16Type, cast_string_to_i16)?
+ }
+ DataType::Int32 => {
+ cast_utf8_to_int!(string_array, eval_mode, Int32Type, cast_string_to_i32)?
+ }
+ DataType::Int64 => {
+ cast_utf8_to_int!(string_array, eval_mode, Int64Type, cast_string_to_i64)?
+ }
+ dt => unreachable!(
+ "{}",
+ format!("invalid integer type {dt} in cast from string")
+ ),
+ };
+ Ok(cast_array)
+ }
+
+ fn unpack_dict_string_array<T: ArrowDictionaryKeyType>(
+ array: &ArrayRef,
+ ) -> DataFusionResult<ArrayRef> {
+ let dict_array = array
+ .as_any()
+ .downcast_ref::<DictionaryArray<T>>()
+ .expect("DictionaryArray<T>");
+
+ let unpacked_array = take(dict_array.values().as_ref(), dict_array.keys(), None)?;
+ Ok(unpacked_array)
}
Review Comment:
Do we need to have this? I think we can just use arrow-rs's cast kernel.
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1582496611
##########
spark/src/test/scala/org/apache/comet/CometCastSuite.scala:
##########
@@ -66,20 +71,65 @@ class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper {
castTest(testValues, DataTypes.BooleanType)
}
- ignore("cast string to byte") {
- castTest(generateStrings(numericPattern, 8).toDF("a"), DataTypes.ByteType)
- }
-
- ignore("cast string to short") {
- castTest(generateStrings(numericPattern, 8).toDF("a"), DataTypes.ShortType)
- }
-
- ignore("cast string to int") {
- castTest(generateStrings(numericPattern, 8).toDF("a"), DataTypes.IntegerType)
- }
-
- ignore("cast string to long") {
- castTest(generateStrings(numericPattern, 8).toDF("a"), DataTypes.LongType)
+ private val castStringToIntegralInputs: Seq[String] = Seq(
Review Comment:
nit: Since the cast code handles leading and trailing white spaces, I think we can add more input with white spaces.
For example:
```scala
castStringToIntegeralnputs.flatMap { x => Seq(" " + x, x + " ", " " + x + " ") }
```
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1581875517
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -142,6 +226,311 @@ impl Cast {
}
}
+fn cast_string_to_i8(str: &str, eval_mode: EvalMode) -> CometResult<Option<i8>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "TINYINT",
+ i8::MIN as i32,
+ i8::MAX as i32,
+ )?
+ .map(|v| v as i8))
+}
+
+fn cast_string_to_i16(str: &str, eval_mode: EvalMode) -> CometResult<Option<i16>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "SMALLINT",
+ i16::MIN as i32,
+ i16::MAX as i32,
+ )?
+ .map(|v| v as i16))
+}
+
+fn cast_string_to_i32(str: &str, eval_mode: EvalMode) -> CometResult<Option<i32>> {
+ let mut accum = CastStringToInt32::default();
+ do_cast_string_to_int(&mut accum, str, eval_mode, "INT")?;
+ Ok(accum.result)
+}
+
+fn cast_string_to_i64(str: &str, eval_mode: EvalMode) -> CometResult<Option<i64>> {
+ let mut accum = CastStringToInt64::default();
+ do_cast_string_to_int(&mut accum, str, eval_mode, "BIGINT")?;
+ Ok(accum.result)
+}
+
+fn cast_string_to_int_with_range_check(
+ str: &str,
+ eval_mode: EvalMode,
+ type_name: &str,
+ min: i32,
+ max: i32,
+) -> CometResult<Option<i32>> {
+ let mut accum = CastStringToInt32::default();
+ do_cast_string_to_int(&mut accum, str, eval_mode, type_name)?;
+ match accum.result {
+ None => Ok(None),
+ Some(v) if v >= min && v <= max => Ok(Some(v)),
+ _ if eval_mode == EvalMode::Ansi => Err(invalid_value(str, "STRING", type_name)),
+ _ => Ok(None),
+ }
+}
+
+/// We support parsing strings to i32 and i64 to match Spark's logic. Support for i8 and i16 is
+/// implemented by first parsing as i32 and then downcasting. The CastStringToInt trait is
+/// introduced so that we can have the parsing logic delegate either to an i32 or i64 accumulator
+/// and avoid the need to use macros here.
+trait CastStringToInt {
+ fn accumulate(
+ &mut self,
+ eval_mode: EvalMode,
+ type_name: &str,
+ str: &str,
+ digit: u32,
+ ) -> CometResult<()>;
+
+ fn reset(&mut self);
+
+ fn finish(
+ &mut self,
+ eval_mode: EvalMode,
+ type_name: &str,
+ str: &str,
+ negative: bool,
+ ) -> CometResult<()>;
+}
+struct CastStringToInt32 {
+ negative: bool,
+ result: Option<i32>,
+ radix: i32,
+}
+
+impl Default for CastStringToInt32 {
+ fn default() -> Self {
+ Self {
+ negative: false,
+ result: Some(0),
+ radix: 10,
+ }
+ }
+}
+
+impl CastStringToInt for CastStringToInt32 {
+ fn accumulate(
+ &mut self,
+ eval_mode: EvalMode,
+ type_name: &str,
+ str: &str,
+ digit: u32,
+ ) -> CometResult<()> {
+ // We are going to process the new digit and accumulate the result. However, before doing
+ // this, if the result is already smaller than the stopValue(Integer.MIN_VALUE / radix),
+ // then result * 10 will definitely be smaller than minValue, and we can stop
+ if let Some(r) = self.result {
+ let stop_value = i32::MIN / self.radix;
+ if r < stop_value {
+ self.reset();
+ return none_or_err(eval_mode, type_name, str);
+ }
+ }
+ // Since the previous result is less than or equal to stopValue(Integer.MIN_VALUE / radix),
+ // we can just use `result > 0` to check overflow. If result overflows, we should stop
+ let v = self.result.unwrap_or(0) * self.radix;
+ match v.checked_sub(digit as i32) {
+ Some(x) if x <= 0 => self.result = Some(x),
+ _ => {
+ self.reset();
+ return none_or_err(eval_mode, type_name, str);
+ }
+ }
+ Ok(())
+ }
+ fn reset(&mut self) {
+ self.result = None;
+ }
+
+ fn finish(
+ &mut self,
+ eval_mode: EvalMode,
+ type_name: &str,
+ str: &str,
+ negative: bool,
+ ) -> CometResult<()> {
+ if !negative {
+ if let Some(r) = self.result {
+ let negated = r.checked_neg().unwrap_or(-1);
+ if negated < 0 {
+ self.reset();
+ return none_or_err(eval_mode, type_name, str);
+ }
+ self.result = Some(negated);
+ }
+ }
+ Ok(())
+ }
+}
+
+struct CastStringToInt64 {
+ negative: bool,
+ result: Option<i64>,
+ radix: i64,
+}
+
+impl Default for CastStringToInt64 {
+ fn default() -> Self {
+ Self {
+ negative: false,
+ result: Some(0),
+ radix: 10,
+ }
+ }
+}
+
+impl CastStringToInt for CastStringToInt64 {
+ fn accumulate(
+ &mut self,
+ eval_mode: EvalMode,
+ type_name: &str,
+ str: &str,
+ digit: u32,
+ ) -> CometResult<()> {
Review Comment:
I reimplemented this and it no longer uses options
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1581165723
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -142,6 +226,311 @@ impl Cast {
}
}
+fn cast_string_to_i8(str: &str, eval_mode: EvalMode) -> CometResult<Option<i8>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "TINYINT",
+ i8::MIN as i32,
+ i8::MAX as i32,
+ )?
+ .map(|v| v as i8))
+}
+
+fn cast_string_to_i16(str: &str, eval_mode: EvalMode) -> CometResult<Option<i16>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "SMALLINT",
+ i16::MIN as i32,
+ i16::MAX as i32,
+ )?
+ .map(|v| v as i16))
+}
+
+fn cast_string_to_i32(str: &str, eval_mode: EvalMode) -> CometResult<Option<i32>> {
+ let mut accum = CastStringToInt32::default();
+ do_cast_string_to_int(&mut accum, str, eval_mode, "INT")?;
+ Ok(accum.result)
+}
+
+fn cast_string_to_i64(str: &str, eval_mode: EvalMode) -> CometResult<Option<i64>> {
+ let mut accum = CastStringToInt64::default();
+ do_cast_string_to_int(&mut accum, str, eval_mode, "BIGINT")?;
+ Ok(accum.result)
+}
+
+fn cast_string_to_int_with_range_check(
+ str: &str,
+ eval_mode: EvalMode,
+ type_name: &str,
+ min: i32,
+ max: i32,
+) -> CometResult<Option<i32>> {
+ let mut accum = CastStringToInt32::default();
Review Comment:
I filed https://github.com/apache/datafusion-comet/issues/330
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1581876400
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -142,6 +226,311 @@ impl Cast {
}
}
+fn cast_string_to_i8(str: &str, eval_mode: EvalMode) -> CometResult<Option<i8>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "TINYINT",
+ i8::MIN as i32,
+ i8::MAX as i32,
+ )?
+ .map(|v| v as i8))
+}
+
+fn cast_string_to_i16(str: &str, eval_mode: EvalMode) -> CometResult<Option<i16>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "SMALLINT",
+ i16::MIN as i32,
+ i16::MAX as i32,
+ )?
+ .map(|v| v as i16))
+}
+
+fn cast_string_to_i32(str: &str, eval_mode: EvalMode) -> CometResult<Option<i32>> {
+ let mut accum = CastStringToInt32::default();
+ do_cast_string_to_int(&mut accum, str, eval_mode, "INT")?;
+ Ok(accum.result)
+}
+
+fn cast_string_to_i64(str: &str, eval_mode: EvalMode) -> CometResult<Option<i64>> {
+ let mut accum = CastStringToInt64::default();
+ do_cast_string_to_int(&mut accum, str, eval_mode, "BIGINT")?;
+ Ok(accum.result)
+}
+
+fn cast_string_to_int_with_range_check(
+ str: &str,
+ eval_mode: EvalMode,
+ type_name: &str,
+ min: i32,
+ max: i32,
+) -> CometResult<Option<i32>> {
+ let mut accum = CastStringToInt32::default();
Review Comment:
This code is removed now. I eventually figured out how to use generics 😅
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#issuecomment-2087972895
Looks good to me. Thanks @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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1583677411
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -142,6 +230,193 @@ impl Cast {
}
}
+fn cast_string_to_i8(str: &str, eval_mode: EvalMode) -> CometResult<Option<i8>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "TINYINT",
+ i8::MIN as i32,
+ i8::MAX as i32,
+ )?
+ .map(|v| v as i8))
+}
+
+fn cast_string_to_i16(str: &str, eval_mode: EvalMode) -> CometResult<Option<i16>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "SMALLINT",
+ i16::MIN as i32,
+ i16::MAX as i32,
+ )?
+ .map(|v| v as i16))
+}
+
+fn cast_string_to_i32(str: &str, eval_mode: EvalMode) -> CometResult<Option<i32>> {
+ do_cast_string_to_int::<i32>(str, eval_mode, "INT", i32::MIN)
+}
+
+fn cast_string_to_i64(str: &str, eval_mode: EvalMode) -> CometResult<Option<i64>> {
+ do_cast_string_to_int::<i64>(str, eval_mode, "BIGINT", i64::MIN)
+}
+
+fn cast_string_to_int_with_range_check(
+ str: &str,
+ eval_mode: EvalMode,
+ type_name: &str,
+ min: i32,
+ max: i32,
+) -> CometResult<Option<i32>> {
+ match do_cast_string_to_int(str, eval_mode, type_name, i32::MIN)? {
+ None => Ok(None),
+ Some(v) if v >= min && v <= max => Ok(Some(v)),
+ _ if eval_mode == EvalMode::Ansi => Err(invalid_value(str, "STRING", type_name)),
+ _ => Ok(None),
+ }
+}
+
+#[derive(PartialEq)]
+enum State {
+ SkipLeadingWhiteSpace,
+ SkipTrailingWhiteSpace,
+ ParseSignAndDigits,
+ ParseFractionalDigits,
+}
+
+fn do_cast_string_to_int<
+ T: Num + PartialOrd + Integer + CheckedSub + CheckedNeg + From<i32> + Copy,
+>(
+ str: &str,
+ eval_mode: EvalMode,
+ type_name: &str,
+ min_value: T,
+) -> CometResult<Option<T>> {
+ let len = str.len();
+ if len == 0 {
Review Comment:
```suggestion
if str.isEmpty() {
```
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1583889160
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -142,6 +230,193 @@ impl Cast {
}
}
+fn cast_string_to_i8(str: &str, eval_mode: EvalMode) -> CometResult<Option<i8>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "TINYINT",
+ i8::MIN as i32,
+ i8::MAX as i32,
+ )?
+ .map(|v| v as i8))
+}
+
+fn cast_string_to_i16(str: &str, eval_mode: EvalMode) -> CometResult<Option<i16>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "SMALLINT",
+ i16::MIN as i32,
+ i16::MAX as i32,
+ )?
+ .map(|v| v as i16))
+}
+
+fn cast_string_to_i32(str: &str, eval_mode: EvalMode) -> CometResult<Option<i32>> {
+ do_cast_string_to_int::<i32>(str, eval_mode, "INT", i32::MIN)
+}
+
+fn cast_string_to_i64(str: &str, eval_mode: EvalMode) -> CometResult<Option<i64>> {
+ do_cast_string_to_int::<i64>(str, eval_mode, "BIGINT", i64::MIN)
+}
+
+fn cast_string_to_int_with_range_check(
+ str: &str,
+ eval_mode: EvalMode,
+ type_name: &str,
+ min: i32,
+ max: i32,
+) -> CometResult<Option<i32>> {
+ match do_cast_string_to_int(str, eval_mode, type_name, i32::MIN)? {
+ None => Ok(None),
+ Some(v) if v >= min && v <= max => Ok(Some(v)),
+ _ if eval_mode == EvalMode::Ansi => Err(invalid_value(str, "STRING", type_name)),
+ _ => Ok(None),
+ }
+}
+
+#[derive(PartialEq)]
+enum State {
+ SkipLeadingWhiteSpace,
+ SkipTrailingWhiteSpace,
+ ParseSignAndDigits,
+ ParseFractionalDigits,
+}
+
+fn do_cast_string_to_int<
+ T: Num + PartialOrd + Integer + CheckedSub + CheckedNeg + From<i32> + Copy,
+>(
+ str: &str,
+ eval_mode: EvalMode,
+ type_name: &str,
+ min_value: T,
+) -> CometResult<Option<T>> {
+ let len = str.len();
+ if len == 0 {
+ return none_or_err(eval_mode, type_name, str);
+ }
+
+ let mut result: T = T::zero();
+ let mut negative = false;
+ let radix = T::from(10);
+ let stop_value = min_value / radix;
+ let mut state = State::SkipLeadingWhiteSpace;
+ let mut parsed_sign = false;
+
+ for (i, ch) in str.char_indices() {
+ // skip leading whitespace
+ if state == State::SkipLeadingWhiteSpace {
+ if ch.is_whitespace() {
+ // consume this char
+ continue;
+ }
+ // change state and fall through to next section
+ state = State::ParseSignAndDigits;
+ }
+
+ if state == State::ParseSignAndDigits {
+ if !parsed_sign {
+ negative = ch == '-';
+ let positive = ch == '+';
+ parsed_sign = true;
+ if negative || positive {
+ if i + 1 == len {
+ // input string is just "+" or "-"
+ return none_or_err(eval_mode, type_name, str);
+ }
+ // consume this char
+ continue;
+ }
+ }
+
+ if ch == '.' {
+ if eval_mode == EvalMode::Legacy {
+ // truncate decimal in legacy mode
+ state = State::ParseFractionalDigits;
+ continue;
+ } else {
+ return none_or_err(eval_mode, type_name, str);
+ }
+ }
+
+ let digit = if ch.is_ascii_digit() {
+ (ch as u32) - ('0' as u32)
+ } else {
+ return none_or_err(eval_mode, type_name, str);
+ };
+
+ // We are going to process the new digit and accumulate the result. However, before
+ // doing this, if the result is already smaller than the
+ // stopValue(Integer.MIN_VALUE / radix), then result * 10 will definitely be
+ // smaller than minValue, and we can stop
+ if result < stop_value {
+ return none_or_err(eval_mode, type_name, str);
+ }
+
+ // Since the previous result is less than or equal to stopValue(Integer.MIN_VALUE /
+ // radix), we can just use `result > 0` to check overflow. If result
+ // overflows, we should stop
+ let v = result * radix;
+ let digit = (digit as i32).into();
+ match v.checked_sub(&digit) {
+ Some(x) if x <= T::zero() => result = x,
+ _ => {
+ return none_or_err(eval_mode, type_name, str);
+ }
+ }
+ }
+
+ if state == State::ParseFractionalDigits {
+ // This is the case when we've encountered a decimal separator. The fractional
+ // part will not change the number, but we will verify that the fractional part
+ // is well-formed.
+ if ch.is_whitespace() {
+ // finished parsing fractional digits, now need to skip trailing whitespace
+ state = State::SkipTrailingWhiteSpace;
+ // consume this char
+ continue;
+ }
+ if !ch.is_ascii_digit() {
+ return none_or_err(eval_mode, type_name, str);
+ }
+ }
+
+ // skip trailing whitespace
+ if state == State::SkipTrailingWhiteSpace && !ch.is_whitespace() {
+ return none_or_err(eval_mode, type_name, str);
+ }
+ }
+
+ if !negative {
+ if let Some(neg) = result.checked_neg() {
+ if neg < T::zero() {
+ return none_or_err(eval_mode, type_name, str);
+ }
+ result = neg;
+ } else {
+ return none_or_err(eval_mode, type_name, str);
+ }
+ }
+
+ Ok(Some(result))
+}
+
+/// Either return Ok(None) or Err(CometError::CastInvalidValue) depending on the evaluation mode
+fn none_or_err<T>(eval_mode: EvalMode, type_name: &str, str: &str) -> CometResult<Option<T>> {
+ match eval_mode {
+ EvalMode::Ansi => Err(invalid_value(str, "STRING", type_name)),
+ _ => Ok(None),
+ }
+}
+
+fn invalid_value(value: &str, from_type: &str, to_type: &str) -> CometError {
+ CometError::CastInvalidValue {
+ value: value.to_string(),
+ from_type: from_type.to_string(),
+ to_type: to_type.to_string(),
+ }
+}
Review Comment:
Thanks. I have updated this.
--
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: Implement Spark-compatible CAST from string to integral types [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #307:
URL: https://github.com/apache/datafusion-comet/pull/307#discussion_r1583892096
##########
core/src/execution/datafusion/expressions/cast.rs:
##########
@@ -142,6 +230,193 @@ impl Cast {
}
}
+fn cast_string_to_i8(str: &str, eval_mode: EvalMode) -> CometResult<Option<i8>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "TINYINT",
+ i8::MIN as i32,
+ i8::MAX as i32,
+ )?
+ .map(|v| v as i8))
+}
+
+fn cast_string_to_i16(str: &str, eval_mode: EvalMode) -> CometResult<Option<i16>> {
+ Ok(cast_string_to_int_with_range_check(
+ str,
+ eval_mode,
+ "SMALLINT",
+ i16::MIN as i32,
+ i16::MAX as i32,
+ )?
+ .map(|v| v as i16))
+}
+
+fn cast_string_to_i32(str: &str, eval_mode: EvalMode) -> CometResult<Option<i32>> {
+ do_cast_string_to_int::<i32>(str, eval_mode, "INT", i32::MIN)
+}
+
+fn cast_string_to_i64(str: &str, eval_mode: EvalMode) -> CometResult<Option<i64>> {
+ do_cast_string_to_int::<i64>(str, eval_mode, "BIGINT", i64::MIN)
+}
Review Comment:
Spark has `IntWrapper` and `LongWrapper` which are equivalent to `do_cast_string_to_int::<i32>` and `do_cast_string_to_int::<i64>` in this PR.
This is the logic for casting to byte in Spark. It uses `IntWrapper` then casts to `byte`.
```java
public boolean toByte(IntWrapper intWrapper) {
if (toInt(intWrapper)) {
int intValue = intWrapper.value;
byte result = (byte) intValue;
```
--
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