kumarUjjawal commented on code in PR #21948:
URL: https://github.com/apache/datafusion/pull/21948#discussion_r3167310549
##########
datafusion/functions/benches/to_char.rs:
##########
@@ -278,6 +278,65 @@ fn criterion_benchmark(c: &mut Criterion) {
)
})
});
+
+ // These two benchmarks use Date32 data with format strings that contain
Review Comment:
Can we adjust the comment to say that one benchmark covers full fallback,
and the mixed one covers partial fallback? I would also avoid naming a commit
hash here, since it can become stale after squash or rebase.
##########
datafusion/functions/src/datetime/to_char.rs:
##########
@@ -260,19 +265,20 @@ fn to_char_array(args: &[ColumnarValue]) ->
Result<ColumnarValue> {
buffer.clear();
- // We'd prefer to write directly to the StringBuilder's internal
buffer,
- // but the write might fail, and there's no easy way to ensure a
partial
- // write is removed from the buffer. So instead we write to a temporary
- // buffer and `append_value` on success.
match formatter.value(idx).write(&mut buffer) {
Ok(()) => builder.append_value(&buffer),
- // Retry with Date64 (see comment in to_char_scalar).
Err(_) if data_type == &Date32 => {
buffer.clear();
- let date64_value = cast(&data_array.slice(idx, 1), &Date64)?;
- let retry_fmt =
- ArrayFormatter::try_new(date64_value.as_ref(),
&format_options)?;
- retry_fmt.value(0).write(&mut buffer)?;
+ let date64_ref = match &date64_array {
+ Some(arr) => arr.as_ref(),
+ None => {
+ date64_array = Some(cast(data_array.as_ref(),
&Date64)?);
+ date64_array.as_ref().unwrap().as_ref()
+ }
+ };
+ let retry_options = build_format_options(&Date64, format)?;
Review Comment:
add one Date32 array-array test where two fallback rows use different
datetime formats
##########
datafusion/functions/src/datetime/to_char.rs:
##########
@@ -260,19 +265,20 @@ fn to_char_array(args: &[ColumnarValue]) ->
Result<ColumnarValue> {
buffer.clear();
- // We'd prefer to write directly to the StringBuilder's internal
buffer,
- // but the write might fail, and there's no easy way to ensure a
partial
- // write is removed from the buffer. So instead we write to a temporary
- // buffer and `append_value` on success.
Review Comment:
we should keep the old comment about why this code writes into a temporary
String first.
--
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]
