findepi commented on code in PR #13806:
URL: https://github.com/apache/datafusion/pull/13806#discussion_r1888394404
##########
datafusion/sql/src/expr/value.rs:
##########
@@ -315,45 +321,80 @@ const fn try_decode_hex_char(c: u8) -> Option<u8> {
}
}
-/// Parse Decimal128 from a string
-///
-/// TODO: support parsing from scientific notation
-fn parse_decimal_128(unsigned_number: &str, negative: bool) -> Result<Expr> {
- // remove leading zeroes
- let trimmed = unsigned_number.trim_start_matches('0');
- // Parse precision and scale, remove decimal point if exists
- let (precision, scale, replaced_str) = if trimmed == "." {
- // Special cases for numbers such as “0.”, “000.”, and so on.
- (1, 0, Cow::Borrowed("0"))
- } else if let Some(i) = trimmed.find('.') {
- (
- trimmed.len() - 1,
- trimmed.len() - i - 1,
- Cow::Owned(trimmed.replace('.', "")),
- )
- } else {
- // No decimal point, keep as is
- (trimmed.len(), 0, Cow::Borrowed(trimmed))
- };
+/// Callers ensure the value is within i256 range
+/// Modified from
<https://github.com/apache/arrow-rs/blob/c4dbf0d8af6ca5a19b8b2ea777da3c276807fc5e/arrow-buffer/src/bigint/mod.rs#L303>
+fn bigint_to_i256(v: BigInt) -> Result<i256> {
+ let v_bytes = v.to_signed_bytes_le();
+ match v_bytes.len().cmp(&32) {
+ Ordering::Less => {
+ let mut bytes = if v.is_negative() {
+ [255_u8; 32]
+ } else {
+ [0; 32]
+ };
+ bytes[0..v_bytes.len()].copy_from_slice(&v_bytes[..v_bytes.len()]);
+ Ok(i256::from_le_bytes(bytes))
+ }
+ Ordering::Equal =>
Ok(i256::from_le_bytes(v_bytes.try_into().unwrap())),
+ Ordering::Greater => {
+ internal_err!("Unexpected overflow when converting {} to i256", v)
+ }
+ }
+}
- let number = replaced_str.parse::<i128>().map_err(|e| {
+fn parse_decimal(unsigned_number: &str, negative: bool) -> Result<Expr> {
+ let mut dec = BigDecimal::from_str(unsigned_number).map_err(|e| {
DataFusionError::from(ParserError(format!(
- "Cannot parse {replaced_str} as i128 when building decimal: {e}"
+ "Cannot parse {unsigned_number} as BigDecimal: {e}"
)))
})?;
-
- // Check precision overflow
- if precision as u8 > DECIMAL128_MAX_PRECISION {
- return Err(DataFusionError::from(ParserError(format!(
- "Cannot parse {replaced_str} as i128 when building decimal:
precision overflow"
- ))));
+ if negative {
+ dec = dec.neg();
}
- Ok(Expr::Literal(ScalarValue::Decimal128(
- Some(if negative { -number } else { number }),
- precision as u8,
- scale as i8,
- )))
+ let digits = dec.digits();
+ let (int_val, scale) = dec.into_bigint_and_exponent();
+ if scale < i8::MIN as i64 {
+ return not_impl_err!(
+ "Decimal scale {} exceeds the minimum supported scale: {}",
+ scale,
+ i8::MIN
+ );
+ }
+ let precision = if scale > 0 {
+ // arrow-rs requires the precision to include the positive scale.
+ // See
<https://github.com/apache/arrow-rs/blob/123045cc766d42d1eb06ee8bb3f09e39ea995ddc/arrow-array/src/types.rs#L1230>
+ std::cmp::max(digits, scale.unsigned_abs())
+ } else {
+ digits
+ };
+ if precision <= DECIMAL128_MAX_PRECISION as u64 {
+ let val = int_val.to_i128().ok_or_else(|| {
+ // Failures are unexpected here as we have already checked the
precision
+ internal_datafusion_err!(
+ "Unexpected overflow when converting {} to i128",
+ int_val
+ )
+ })?;
+ Ok(Expr::Literal(ScalarValue::Decimal128(
+ Some(val),
+ precision as u8,
+ scale as i8,
+ )))
+ } else if precision <= DECIMAL256_MAX_PRECISION as u64 {
+ let val = bigint_to_i256(int_val)?;
+ Ok(Expr::Literal(ScalarValue::Decimal256(
+ Some(val),
+ precision as u8,
+ scale as i8,
+ )))
+ } else {
+ not_impl_err!(
Review Comment:
let's not do Javascript :)
the value semantics should be predicable to the user, so they should not
really depend on number of digits ("many or too many digits?")
##########
datafusion/sql/src/expr/value.rs:
##########
@@ -315,45 +321,80 @@ const fn try_decode_hex_char(c: u8) -> Option<u8> {
}
}
-/// Parse Decimal128 from a string
-///
-/// TODO: support parsing from scientific notation
-fn parse_decimal_128(unsigned_number: &str, negative: bool) -> Result<Expr> {
- // remove leading zeroes
- let trimmed = unsigned_number.trim_start_matches('0');
- // Parse precision and scale, remove decimal point if exists
- let (precision, scale, replaced_str) = if trimmed == "." {
- // Special cases for numbers such as “0.”, “000.”, and so on.
- (1, 0, Cow::Borrowed("0"))
- } else if let Some(i) = trimmed.find('.') {
- (
- trimmed.len() - 1,
- trimmed.len() - i - 1,
- Cow::Owned(trimmed.replace('.', "")),
- )
- } else {
- // No decimal point, keep as is
- (trimmed.len(), 0, Cow::Borrowed(trimmed))
- };
+/// Callers ensure the value is within i256 range
+/// Modified from
<https://github.com/apache/arrow-rs/blob/c4dbf0d8af6ca5a19b8b2ea777da3c276807fc5e/arrow-buffer/src/bigint/mod.rs#L303>
+fn bigint_to_i256(v: BigInt) -> Result<i256> {
+ let v_bytes = v.to_signed_bytes_le();
+ match v_bytes.len().cmp(&32) {
+ Ordering::Less => {
+ let mut bytes = if v.is_negative() {
+ [255_u8; 32]
+ } else {
+ [0; 32]
+ };
+ bytes[0..v_bytes.len()].copy_from_slice(&v_bytes[..v_bytes.len()]);
+ Ok(i256::from_le_bytes(bytes))
+ }
+ Ordering::Equal =>
Ok(i256::from_le_bytes(v_bytes.try_into().unwrap())),
+ Ordering::Greater => {
+ internal_err!("Unexpected overflow when converting {} to i256", v)
+ }
+ }
+}
- let number = replaced_str.parse::<i128>().map_err(|e| {
+fn parse_decimal(unsigned_number: &str, negative: bool) -> Result<Expr> {
+ let mut dec = BigDecimal::from_str(unsigned_number).map_err(|e| {
DataFusionError::from(ParserError(format!(
- "Cannot parse {replaced_str} as i128 when building decimal: {e}"
+ "Cannot parse {unsigned_number} as BigDecimal: {e}"
)))
})?;
-
- // Check precision overflow
- if precision as u8 > DECIMAL128_MAX_PRECISION {
- return Err(DataFusionError::from(ParserError(format!(
- "Cannot parse {replaced_str} as i128 when building decimal:
precision overflow"
- ))));
+ if negative {
+ dec = dec.neg();
}
- Ok(Expr::Literal(ScalarValue::Decimal128(
- Some(if negative { -number } else { number }),
- precision as u8,
- scale as i8,
- )))
+ let digits = dec.digits();
+ let (int_val, scale) = dec.into_bigint_and_exponent();
+ if scale < i8::MIN as i64 {
+ return not_impl_err!(
+ "Decimal scale {} exceeds the minimum supported scale: {}",
+ scale,
+ i8::MIN
+ );
+ }
+ let precision = if scale > 0 {
+ // arrow-rs requires the precision to include the positive scale.
+ // See
<https://github.com/apache/arrow-rs/blob/123045cc766d42d1eb06ee8bb3f09e39ea995ddc/arrow-array/src/types.rs#L1230>
+ std::cmp::max(digits, scale.unsigned_abs())
+ } else {
+ digits
+ };
+ if precision <= DECIMAL128_MAX_PRECISION as u64 {
+ let val = int_val.to_i128().ok_or_else(|| {
+ // Failures are unexpected here as we have already checked the
precision
+ internal_datafusion_err!(
+ "Unexpected overflow when converting {} to i128",
+ int_val
+ )
+ })?;
+ Ok(Expr::Literal(ScalarValue::Decimal128(
+ Some(val),
+ precision as u8,
+ scale as i8,
+ )))
+ } else if precision <= DECIMAL256_MAX_PRECISION as u64 {
+ let val = bigint_to_i256(int_val)?;
Review Comment:
either add comment like above (`// Failures are unexpected here as we have
already checked the precision`), or handle error nicely.
##########
datafusion/sql/src/expr/value.rs:
##########
@@ -315,45 +321,80 @@ const fn try_decode_hex_char(c: u8) -> Option<u8> {
}
}
-/// Parse Decimal128 from a string
-///
-/// TODO: support parsing from scientific notation
-fn parse_decimal_128(unsigned_number: &str, negative: bool) -> Result<Expr> {
- // remove leading zeroes
- let trimmed = unsigned_number.trim_start_matches('0');
- // Parse precision and scale, remove decimal point if exists
- let (precision, scale, replaced_str) = if trimmed == "." {
- // Special cases for numbers such as “0.”, “000.”, and so on.
- (1, 0, Cow::Borrowed("0"))
- } else if let Some(i) = trimmed.find('.') {
- (
- trimmed.len() - 1,
- trimmed.len() - i - 1,
- Cow::Owned(trimmed.replace('.', "")),
- )
- } else {
- // No decimal point, keep as is
- (trimmed.len(), 0, Cow::Borrowed(trimmed))
- };
+/// Callers ensure the value is within i256 range
+/// Modified from
<https://github.com/apache/arrow-rs/blob/c4dbf0d8af6ca5a19b8b2ea777da3c276807fc5e/arrow-buffer/src/bigint/mod.rs#L303>
+fn bigint_to_i256(v: BigInt) -> Result<i256> {
+ let v_bytes = v.to_signed_bytes_le();
+ match v_bytes.len().cmp(&32) {
+ Ordering::Less => {
+ let mut bytes = if v.is_negative() {
+ [255_u8; 32]
+ } else {
+ [0; 32]
+ };
+ bytes[0..v_bytes.len()].copy_from_slice(&v_bytes[..v_bytes.len()]);
+ Ok(i256::from_le_bytes(bytes))
+ }
+ Ordering::Equal =>
Ok(i256::from_le_bytes(v_bytes.try_into().unwrap())),
+ Ordering::Greater => {
+ internal_err!("Unexpected overflow when converting {} to i256", v)
+ }
+ }
+}
- let number = replaced_str.parse::<i128>().map_err(|e| {
+fn parse_decimal(unsigned_number: &str, negative: bool) -> Result<Expr> {
+ let mut dec = BigDecimal::from_str(unsigned_number).map_err(|e| {
DataFusionError::from(ParserError(format!(
- "Cannot parse {replaced_str} as i128 when building decimal: {e}"
+ "Cannot parse {unsigned_number} as BigDecimal: {e}"
)))
})?;
-
- // Check precision overflow
- if precision as u8 > DECIMAL128_MAX_PRECISION {
- return Err(DataFusionError::from(ParserError(format!(
- "Cannot parse {replaced_str} as i128 when building decimal:
precision overflow"
- ))));
+ if negative {
+ dec = dec.neg();
}
- Ok(Expr::Literal(ScalarValue::Decimal128(
- Some(if negative { -number } else { number }),
- precision as u8,
- scale as i8,
- )))
+ let digits = dec.digits();
+ let (int_val, scale) = dec.into_bigint_and_exponent();
+ if scale < i8::MIN as i64 {
Review Comment:
For what inputs the scale can be negative? It's not for `123000` input but
it's eg for `123e3` input.
The scientific notation syntax should not end up in the "parse decimal"
function. It should go to Float64.
So i propose that we dispatch on scientic notation vs decimal form and
handle only the latter in this function.
Then we can only assert that `scale >= 0` here.
##########
datafusion/sql/src/expr/value.rs:
##########
@@ -315,45 +321,80 @@ const fn try_decode_hex_char(c: u8) -> Option<u8> {
}
}
-/// Parse Decimal128 from a string
-///
-/// TODO: support parsing from scientific notation
-fn parse_decimal_128(unsigned_number: &str, negative: bool) -> Result<Expr> {
- // remove leading zeroes
- let trimmed = unsigned_number.trim_start_matches('0');
- // Parse precision and scale, remove decimal point if exists
- let (precision, scale, replaced_str) = if trimmed == "." {
- // Special cases for numbers such as “0.”, “000.”, and so on.
- (1, 0, Cow::Borrowed("0"))
- } else if let Some(i) = trimmed.find('.') {
- (
- trimmed.len() - 1,
- trimmed.len() - i - 1,
- Cow::Owned(trimmed.replace('.', "")),
- )
- } else {
- // No decimal point, keep as is
- (trimmed.len(), 0, Cow::Borrowed(trimmed))
- };
+/// Callers ensure the value is within i256 range
Review Comment:
Don't have to, the function is fallible
--
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-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
