dd-annarose commented on code in PR #25337:
URL: https://github.com/apache/datafusion/pull/25337#discussion_r4234580939
##########
datafusion/functions-aggregate/src/utils.rs:
##########
@@ -37,41 +41,215 @@ pub(crate) fn get_scalar_value(expr: &Arc<dyn
PhysicalExpr>) -> Result<ScalarVal
}
}
-/// Validates that a percentile expression is a literal float value between
0.0 and 1.0.
-///
-/// Used by both `percentile_cont` and `approx_percentile_cont` to validate
their
-/// percentile parameters.
-pub(crate) fn validate_percentile_expr(
- expr: &Arc<dyn PhysicalExpr>,
- fn_name: &str,
-) -> Result<f64> {
- let scalar_value = get_scalar_value(expr).map_err(|_e| {
- DataFusionError::Plan(format!(
- "Percentile value for '{fn_name}' must be a literal"
- ))
- })?;
-
+/// Validates that a percentile scalar is a Float32/Float64 value between 0.0
and 1.0.
+fn scalar_to_percentile(scalar_value: ScalarValue, fn_name: &str) ->
Result<f64> {
let percentile = match scalar_value {
ScalarValue::Float32(Some(value)) => value as f64,
ScalarValue::Float64(Some(value)) => value,
ScalarValue::Float32(None) | ScalarValue::Float64(None) => {
return plan_err!(
- "Percentile value for '{fn_name}' must be Float32 or Float64
literal (got null)"
+ "Percentile value for '{fn_name}' must be Float32 or Float64
(got null)"
);
}
sv => {
return plan_err!(
- "Percentile value for '{fn_name}' must be Float32 or Float64
literal (got data type {})",
+ "Percentile value for '{fn_name}' must be Float32 or Float64
(got data type {})",
sv.data_type()
);
}
};
- // Ensure the percentile is between 0 and 1.
+ check_percentile_range(percentile)
+}
+
+/// Ensures the percentile is between 0 and 1.
+fn check_percentile_range(percentile: f64) -> Result<f64> {
if !(0.0..=1.0).contains(&percentile) {
return plan_err!(
"Percentile value must be between 0.0 and 1.0 inclusive,
{percentile} is invalid"
);
}
Ok(percentile)
}
+
+/// State of the PercentileParam resolution.
+/// Either already resolved or still requiring a non-empty record batch.
+#[derive(Debug, Clone)]
+pub(crate) enum PercentileParamState {
+ Resolved(f64),
+ Pending,
+}
+
+/// Percentile argument for `aggregate_fn_name` and its state.
+#[derive(Debug)]
+pub struct PercentileParam {
Review Comment:
I decided to keep it as pub and make `PercentileParam::try_new` pub too in
order to preserve API contract for `ApproxPercentileAccumulator`'s constructor.
--
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]
