parthchandra commented on code in PR #4997:
URL: https://github.com/apache/datafusion-comet/pull/4997#discussion_r3648177493
##########
native/spark-expr/src/datetime_funcs/extract_date_part.rs:
##########
@@ -16,13 +16,21 @@
// under the License.
use crate::utils::array_with_timezone;
+use arrow::array::{Array, Decimal128Builder, Int32Array,
Time64NanosecondArray};
use arrow::compute::{date_part, DatePart};
use arrow::datatypes::{DataType, TimeUnit::Microsecond};
-use datafusion::common::{internal_datafusion_err, DataFusionError};
+use datafusion::common::{
+ internal_datafusion_err, utils::take_function_args, DataFusionError,
Result,
+};
use datafusion::logical_expr::{
ColumnarValue, ScalarFunctionArgs, ScalarUDFImpl, Signature, Volatility,
};
-use std::fmt::Debug;
+use std::{fmt::Debug, sync::Arc};
+
+const NANOS_PER_SECOND: i64 = 1_000_000_000;
+const MICROS_PER_SECOND: i128 = 1_000_000;
+const SECOND_DECIMAL_PRECISION: u8 = 8;
+const SECOND_DECIMAL_SCALE: i8 = 6;
/// Returns true when the type is a timestamp without a timezone (Spark's
TimestampNTZType),
/// including when wrapped in a dictionary. Such values store local wall-clock
time and must not
Review Comment:
Nit: `MICROS_PER_SECOND` is technically correct (1M µs/s) but here it serves
as the Decimal(8,6) unscaled-unit factor. A name like
`UNSCALED_UNITS_PER_SECOND` or just inlining `10_i128.pow(SECOND_DECIMAL_SCALE
as u32)` would make the intent clearer — currently readers have to reason about
why "micros" appears in a nanosecond-based function.
##########
native/spark-expr/src/datetime_funcs/extract_date_part.rs:
##########
@@ -117,10 +125,113 @@ extract_date_part!(SparkHour, "hour", Hour);
extract_date_part!(SparkMinute, "minute", Minute);
extract_date_part!(SparkSecond, "second", Second);
+fn second_with_fraction(nanos: i64, precision: i32) -> Result<i128> {
+ if !(0..=SECOND_DECIMAL_SCALE as i32).contains(&precision) {
+ return Err(DataFusionError::Execution(format!(
+ "second_with_fraction: precision must be between 0 and 6, found
{precision}"
Review Comment:
Defensive suggestion: `second_with_fraction` accepts `i64` but silently
produces wrong results for negative nanos (Rust `%` preserves sign). TimeType
values are always non-negative today, so this is not a live bug, but a
`debug_assert!(nanos >= 0)` would guard against future misuse.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
