alamb commented on code in PR #23737:
URL: https://github.com/apache/datafusion/pull/23737#discussion_r3634179971
##########
datafusion/functions-aggregate-common/src/tdigest.rs:
##########
@@ -611,6 +628,83 @@ impl TDigest {
centroids,
}
}
+
+ /// Construct a [`TDigest`] directly from its constituent parts, validating
+ /// the inputs.
+ ///
+ /// This is the non-Arrow counterpart to [`Self::from_scalar_state()`].
+ /// [`Self::to_scalar_state()`]/[`Self::from_scalar_state()`] exist so that
+ /// external systems can persist and restore digest state, but they require
+ /// the caller to pack and unpack that state through a
[`ScalarValue::List`]
+ /// of [`ScalarValue::Float64`] (which `from_scalar_state` immediately
+ /// downcasts back to an `&[f64]`). `try_from_parts`, together with the
+ /// [`Self::centroids()`] and [`Self::sum()`] accessors (and the existing
+ /// [`Self::max_size()`], [`Self::count()`], [`Self::max()`], and
+ /// [`Self::min()`] accessors), exposes that state without requiring the
+ /// caller to round-trip through `ScalarValue`, so a digest can be
serialized
+ /// into and restored from a caller's own format.
+ ///
+ /// Unlike [`Self::from_scalar_state()`], which trusts its input because it
+ /// is only ever fed the output of [`Self::to_scalar_state()`] on the
+ /// intra-query hot path, this is the door for *external* persisters: it
may
+ /// be handed bytes decoded from a caller's own format, so it validates the
+ /// invariants that later [`Self::estimate_quantile()`] / merge math relies
+ /// on and returns an error instead of producing a silently wrong digest.
+ /// Callers who trust their data can `unwrap()`.
Review Comment:
I feel like a lot of this comment is just noise as it is describing how
to/from scalar state works
I think this paragraph could be summarized in a single sentence
```
/// Along with
/// [`Self::centroids()`] and [`Self::sum()`]
/// [`Self::max_size()`], [`Self::count()`], [`Self::max()`].
/// [`Self::min()`], allows a digest can be serialized
/// into and restored from a caller's own format.
```
##########
datafusion/functions-aggregate-common/src/tdigest.rs:
##########
@@ -611,6 +628,83 @@ impl TDigest {
centroids,
}
}
+
+ /// Construct a [`TDigest`] directly from its constituent parts, validating
+ /// the inputs.
+ ///
+ /// This is the non-Arrow counterpart to [`Self::from_scalar_state()`].
+ /// [`Self::to_scalar_state()`]/[`Self::from_scalar_state()`] exist so that
+ /// external systems can persist and restore digest state, but they require
+ /// the caller to pack and unpack that state through a
[`ScalarValue::List`]
+ /// of [`ScalarValue::Float64`] (which `from_scalar_state` immediately
+ /// downcasts back to an `&[f64]`). `try_from_parts`, together with the
+ /// [`Self::centroids()`] and [`Self::sum()`] accessors (and the existing
+ /// [`Self::max_size()`], [`Self::count()`], [`Self::max()`], and
+ /// [`Self::min()`] accessors), exposes that state without requiring the
+ /// caller to round-trip through `ScalarValue`, so a digest can be
serialized
+ /// into and restored from a caller's own format.
+ ///
+ /// Unlike [`Self::from_scalar_state()`], which trusts its input because it
+ /// is only ever fed the output of [`Self::to_scalar_state()`] on the
+ /// intra-query hot path, this is the door for *external* persisters: it
may
+ /// be handed bytes decoded from a caller's own format, so it validates the
Review Comment:
This pargraph also talks a lot about internal details when I think the key
point is "this method validates its inputs"
--
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]
