Copilot commented on code in PR #25161:
URL: https://github.com/apache/datafusion/pull/25161#discussion_r3981759702
##########
datafusion/functions/src/datetime/from_unixtime.rs:
##########
@@ -133,7 +176,7 @@ impl ScalarUDFImpl for FromUnixtimeFunc {
}
match len {
- 1 => args[0].cast_to(&Timestamp(Second, None), None),
+ 1 => args[0].cast_to(&Timestamp(Second, self.timezone.clone()),
None),
Review Comment:
With a named session timezone, this new branch exposes a SQL-triggerable
process panic for previously valid one-argument calls: for example,
`from_unixtime(-8334601211039)` with `America/New_York` succeeds on the old
path but panics when the timezone-aware result is formatted. Please prevent
these out-of-range values from reaching Arrow's named-timezone formatter
(returning a DataFusion error), or land the Arrow formatter fix before widening
the panic to this overload.
##########
datafusion/functions/src/datetime/mod.rs:
##########
@@ -47,12 +47,12 @@ make_udf_function!(date_part::DatePartFunc, date_part);
make_udf_function!(date_trunc::DateTruncFunc, date_trunc);
make_udf_function!(make_date::MakeDateFunc, make_date);
make_udf_function!(make_time::MakeTimeFunc, make_time);
-make_udf_function!(from_unixtime::FromUnixtimeFunc, from_unixtime);
make_udf_function!(to_char::ToCharFunc, to_char);
make_udf_function!(to_date::ToDateFunc, to_date);
make_udf_function!(to_local_time::ToLocalTimeFunc, to_local_time);
make_udf_function!(to_time::ToTimeFunc, to_time);
make_udf_function!(to_unixtime::ToUnixtimeFunc, to_unixtime);
+make_udf_function_with_config!(from_unixtime::FromUnixtimeFunc, from_unixtime);
Review Comment:
Changing to this macro also changes the public Rust factory from
`datetime::from_unixtime()` to `datetime::from_unixtime(&ConfigOptions)`, so
downstream callers fail to compile; deprecating `FromUnixtimeFunc::new()` does
not preserve that API. Please retain the zero-argument factory and introduce a
distinctly named config-aware factory (or otherwise provide a
deprecation/migration path) for internal registration.
##########
datafusion/functions/src/datetime/from_unixtime.rs:
##########
@@ -20,18 +20,26 @@ use std::sync::Arc;
use arrow::datatypes::DataType::{Int64, Timestamp, Utf8};
use arrow::datatypes::TimeUnit::Second;
use arrow::datatypes::{DataType, Field, FieldRef};
+use datafusion_common::config::ConfigOptions;
use datafusion_common::{Result, ScalarValue, exec_err, internal_err};
use datafusion_expr::TypeSignature::Exact;
use datafusion_expr::sort_properties::{ExprProperties, SortProperties};
use datafusion_expr::{
- ColumnarValue, Documentation, ReturnFieldArgs, ScalarFunctionArgs,
ScalarUDFImpl,
- Signature, Volatility,
+ ColumnarValue, Documentation, ReturnFieldArgs, ScalarFunctionArgs,
ScalarUDF,
+ ScalarUDFImpl, Signature, Volatility,
};
use datafusion_macros::user_doc;
#[user_doc(
doc_section(label = "Time and Date Functions"),
- description = "Converts an integer to RFC3339 timestamp format
(`YYYY-MM-DDT00:00:00.000000000Z`). Integers and unsigned integers are
interpreted as seconds since the unix epoch (`1970-01-01T00:00:00Z`) return the
corresponding timestamp.",
+ description = r#"
+Converts an integer to RFC3339 timestamp format
(`YYYY-MM-DDT00:00:00.000000000Z`).
+Integers and unsigned integers are interpreted as seconds since the unix epoch
+(`1970-01-01T00:00:00Z`) return the corresponding timestamp.
Review Comment:
This description is inconsistent with the implementation: the function
returns an Arrow `Timestamp(Second, ...)`, not a format string with
nanoseconds, and the new session-timezone behavior can render an offset rather
than the fixed `Z` shown here. Please describe it as converting epoch seconds
to a second-resolution timestamp in the selected timezone; the generated
scalar-functions page should then be regenerated.
--
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]
