U0001F3A2 opened a new pull request, #23669:
URL: https://github.com/apache/datafusion/pull/23669
## Which issue does this PR close?
- Closes #23640.
## Rationale for this change
The formatted variants of `to_date`, `to_timestamp*`, and `to_unixtime`
mishandle NULL string and format arguments. All three cases come from the
shared helpers in `datetime/common.rs`:
```sql
SELECT to_date(NULL::VARCHAR, '%Y-%m-%d');
-- Internal error: a should not be None.
SELECT to_date('2020-09-08', NULL::VARCHAR, NULL::VARCHAR);
-- Internal error: ret should not be None.
```
and a NULL format inside an array argument was parsed rather than skipped.
The third case is a correctness bug, not only a spurious error
([details](https://github.com/apache/datafusion/issues/23640#issuecomment-5002048618)).
`handle_array_op` read the format with `.value(pos)` without checking
validity. Arrow permits a null slot to retain its backing bytes, so a NULL
format can parse *successfully* and silently produce a wrong value. The `''` in
the reported error is only what a null slot happens to hold when the array is
built the usual way. Reproduced: a row whose format is NULL returned
`2020-09-08` by parsing under the retained `%d/%m/%Y`.
The scalar path is what had drifted. The array path already skipped NULL
scalar formats and already returned NULL when none were left, so this mostly
brings the scalar path in line with behavior the array path has shipped for a
while, and with the NULL propagation of the unformatted paths and `to_char`.
## What changes are included in this PR?
Three changes in `datafusion/functions/src/datetime/common.rs`:
- `handle_multiple` returns NULL for a NULL scalar input instead of
`unwrap_or_internal_err!`. This also resolves the pre-existing `// ASK: Why do
we trust a to be non-null at this point?` comment sitting at that site.
- `handle_multiple` returns NULL when every candidate format is NULL.
- `handle_array_op` skips a null format slot, matching what the scalar arm
at the same site already did (`Some(None) => continue`).
Existing error behavior is preserved when at least one non-NULL format is
present and all non-NULL formats fail to parse.
The second commit documents the NULL rule on `format_n` for `to_date`,
`to_timestamp*`, and `to_unixtime`, and regenerates `scalar_functions.md`.
Worth stating explicitly because skipping NULL formats is not ambient NULL
propagation: `to_date('2020-09-08', NULL::VARCHAR, '%Y-%m-%d')` returns a date
rather than NULL.
Deliberately out of scope, happy to file either as a follow-up:
- `to_time` does not use these helpers. It already skips NULL formats but
still errors when all of them are NULL, so it is now the odd one out in the
family. Its behavior and its docs are unchanged here.
- An untyped `NULL` format (`DataType::Null` rather than `NULL::VARCHAR`)
still errors via `validate_data_types`, unchanged.
- The scalar and array paths implement the same "try formats in order"
semantics twice, which is what let them drift apart. Unifying them is a larger
refactor and a poor fit for a correctness fix.
## Are these changes tested?
Yes.
- sqllogictests in `datetime/dates.slt` and `datetime/timestamps.slt`
covering every case from the issue across `to_date`, `to_timestamp`, and
`to_unixtime`. Confirmed they fail on `main` with the exact reported messages
(`a should not be None`, `ret should not be None`, `using format ''`) and pass
with the fix.
- Unit tests in `to_date.rs`, including one that builds a format array whose
null slot retains `%d/%m/%Y`, pinning the silent-wrong-value case. That one
cannot be expressed in SQL.
- Two guard tests so the fix does not over-reach: a non-NULL format that
fails still errors, and `to_date(NULL::VARCHAR, 12345)` still reports an
invalid format type rather than returning NULL.
`cargo test -p datafusion-functions --lib` (332 passed), the full `datetime`
sqllogictest suite (72 files), `cargo fmt --check`, and `cargo clippy` are
clean.
## Are there any user-facing changes?
Yes, and they are the point of the fix. Queries that raised `Internal error:
a should not be None.` or `Internal error: ret should not be None.` now return
NULL. A NULL format argument is now skipped rather than parsed, which for an
array format argument could previously return a wrong value. No 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]
