jcsherin commented on code in PR #17003:
URL: https://github.com/apache/datafusion/pull/17003#discussion_r2247556364
##########
datafusion/functions-aggregate/src/approx_percentile_cont.rs:
##########
@@ -360,9 +366,14 @@ impl ApproxPercentileAccumulator {
}
}
- // public for approx_percentile_cont_with_weight
+ // Merge new TDigests into this accumulator. Public for
approx_percentile_cont_with_weight.
+ //
+ // Important: max_size Preservation
+ // TDigest::merge_digests uses the max_size from the first digest in the
iterator.
+ // By putting self.digest first, we ensure the accumulator's configured
max_size
+ // is preserved rather than being overridden by the new digests' max_size.
Review Comment:
The `ApproxPercentileWithWeightAccumulator::update_batch` continues to use
`DEFAULT_MAX_SIZE` to create new `TDigest`s. Shouldn't it instead be updated to
use the configured `max_size` value?
Then merging digests will not override `max_size`, and the ordering
dependency goes away.
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
