dbtsai commented on code in PR #110:
URL:
https://github.com/apache/spark-connect-rust/pull/110#discussion_r4406180865
##########
crates/spark-connect/src/row.rs:
##########
@@ -34,6 +34,10 @@ pub enum Value {
Date(i32),
/// Timestamp (microseconds since epoch)
Timestamp(i64),
+ /// Spark day-time interval, in signed microseconds. The full i64 range is
+ /// representable by Python's datetime.timedelta without nanosecond
conversion.
+ /// Display (including Rust DataFrame::show) uses `<value> microseconds`.
+ DayTimeInterval(i64),
Review Comment:
Fixed in 584eaef. Added `LiteralExpression::DayTimeInterval` and an explicit
arm in `value_to_lit_expr`. The new
`pivot_day_time_intervals_are_exact_literals` test builds a group/pivot/count
plan and checks the aggregate proto contains typed interval literals with
exactly `i64::MIN`, -1, 0 and `i64::MAX`. The test passed.
##########
crates/spark-connect/tests/coverage_plan_extra.rs:
##########
@@ -81,6 +82,21 @@ fn nafill_covers_all_value_literal_arms() {
}
}
+#[test]
+fn nafill_preserves_day_time_interval_literal() {
Review Comment:
Removed the dedicated five-value NA-fill test in 584eaef. The interval
variant remains covered by `nafill_covers_all_value_literal_arms`; the boundary
assertions now cover the supported pivot literal path.
##########
crates/spark-connect/src/plan.rs:
##########
@@ -1646,6 +1646,7 @@ fn value_to_proto_literal(v: &crate::row::Value) ->
proto::expression::Literal {
Value::String(s) => LiteralType::String(s.clone()),
Value::Date(d) => LiteralType::Date(*d),
Value::Timestamp(t) => LiteralType::Timestamp(*t),
+ Value::DayTimeInterval(t) => LiteralType::DayTimeInterval(*t),
Review Comment:
Clarified both the code comment and PR description in 584eaef: the typed arm
makes Spark reject interval NA-fill values, preventing the fallback from
silently filling string columns with Debug text. It does not add interval fill
support.
##########
crates/pyspark-rs/src/session.rs:
##########
@@ -896,6 +900,11 @@ fn value_to_datatype(v: &Value) -> DataType {
Value::Binary(_) => DataType::Binary,
Value::Date(_) => DataType::Date,
Value::Timestamp(_) => DataType::Timestamp,
+ Value::DayTimeInterval(_) => {
Review Comment:
Took the infallible-inference option in 584eaef: `value_to_datatype` again
returns `DataType::DayTimeInterval { start_field: 0, end_field: 3 }`, and the
fallible plumbing at its callers is removed. Filed
[SPARK-60659](https://issues.apache.org/jira/browse/SPARK-60659) for core Arrow
interval builders, Python `timedelta` inputs, null-first/explicit-schema cases
and nested collected-row round trips, with coordination noted for #109 and #113.
##########
crates/spark-connect/src/row.rs:
##########
@@ -169,6 +174,7 @@ impl fmt::Display for Value {
Value::Binary(b) => write!(f, "{:?}", b),
Value::Date(d) => write!(f, "{}", d),
Value::Timestamp(t) => write!(f, "{}", t),
+ Value::DayTimeInterval(t) => write!(f, "{} microseconds", t),
Review Comment:
Changed Display and stringified interval map keys to raw microseconds in
584eaef, consistent with Date/Timestamp. Removed the user-facing display-format
discussion. Python `timedelta` map keys remain unsupported, and the
compatibility notes and PR description now state that limitation.
##########
crates/spark-connect/src/row.rs:
##########
@@ -34,6 +34,10 @@ pub enum Value {
Date(i32),
/// Timestamp (microseconds since epoch)
Timestamp(i64),
+ /// Spark day-time interval, in signed microseconds. The full i64 range is
+ /// representable by Python's datetime.timedelta without nanosecond
conversion.
+ /// Display (including Rust DataFrame::show) uses `<value> microseconds`.
+ DayTimeInterval(i64),
Review Comment:
Added a note under “Rust migration for the 4.3 line” in
`docs/compatibility.md` in 584eaef. It calls out the new `DayTimeInterval(i64)`
variants in both public enums (`Value` and `LiteralExpression`) and the
required update to downstream exhaustive matches.
##########
docs/architecture.md:
##########
@@ -79,6 +79,19 @@ flowchart LR
When you call an action such as `count()` or `show()`, the lazily-built plan is
serialized and sent to the server; results stream back as Arrow batches.
+Spark day-time interval results use Arrow `Duration(Microsecond)`. Row decoding
Review Comment:
Trimmed this to two sentences in 584eaef: intervals are returned as
`datetime.timedelta`, including nested values/nulls, and interval inputs are
not yet supported, with a link to SPARK-60659. The representation details
remain in code comments and the Rust migration note.
--
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]
