-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Minor: reduce code duplication in date_bin_impl
#8528
Conversation
DataType::Timestamp(TimeUnit::Second, tz_opt) => { | ||
let array = as_timestamp_second_array(array)? | ||
ColumnarValue::Array(array) => { | ||
fn process_array<T>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we name a function more precisely, process_array
is too generic and requires the reader go to the functions internal code to figure what exactly it does
} | ||
ColumnarValue::Scalar(ScalarValue::TimestampMicrosecond(v, tz_opt)) => { | ||
let f = stride_map_fn::<TimestampMicrosecondType>(origin, stride, stride_fn); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please rename f
to more precise name, like apply_stride
, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Weijun-H please check some minors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm thanks @Weijun-H
Which issue does this PR close?
Closes #.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?