jonahgao commented on code in PR #13806:
URL: https://github.com/apache/datafusion/pull/13806#discussion_r1888673981


##########
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:
   I'm also concerned about the predictability problem.



-- 
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

Reply via email to