kosiew commented on code in PR #23723:
URL: https://github.com/apache/datafusion/pull/23723#discussion_r3680678120
##########
datafusion/functions-table/src/generate_series.rs:
##########
@@ -80,7 +80,12 @@ pub trait SeriesValue: fmt::Debug + Clone + Send + Sync +
'static {
fn should_stop(&self, end: Self, step: &Self::StepType, include_end: bool)
-> bool;
/// Advance to the next value in the series
- fn advance(&mut self, step: &Self::StepType) -> Result<()>;
+ ///
+ /// If advancing would overflow the value range, `end` is updated so that
+ /// the series terminates after the current value (matching the behavior
+ /// of PostgreSQL and DuckDB, which return the reachable values instead of
+ /// erroring).
+ fn advance(&mut self, end: &mut Self, step: &Self::StepType) -> Result<()>;
Review Comment:
Could we preserve the existing public trait method signature here?
Changing `advance` from `fn advance(&mut self, step: &Self::StepType)` to
`fn advance(&mut self, end: &mut Self, step: &Self::StepType)` will break
downstream crates that implement `SeriesValue`, which matches the current
`cargo-semver-checks` failure.
One option would be to keep the existing required method and add a provided
method, or use a private helper for the overflow-aware behaviour. The `i64`
implementation could then override that path without introducing a breaking
public API change.
--
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]
