adriangb commented on PR #25173: URL: https://github.com/apache/datafusion/pull/25173#issuecomment-5626558661
Self-review QA pass on my own PR. I built `datafusion-cli` from this branch and from `main`, and I tested the boundaries myself instead of trust in the tests in the diff. The core change is correct. One finding is substantive. ## 1. The misleading-error family is not closed, and the PR body claims it is This is the same defect shape that https://github.com/apache/datafusion/issues/25169 names: an error that names the wrong type and dumps a raw `Expr`. On this branch: ``` > SELECT * FROM generate_series(NULL, TIMESTAMP '2024-01-03', INTERVAL '1 day'); Error: Error during planning: Argument #2 must be an INTEGER or NULL, got Literal(TimestampNanosecond(1704240000000000000, None), None) ``` The cause is the dispatch in `call_with_args`, which looks at `exprs[0]` alone. An untyped `NULL` first argument routes to `call_int64`, so `call_timestamp` never runs and none of the new messages apply. Both reference engines return an empty series here, so an error is also the wrong answer: ``` -- PostgreSQL 17.11 => SELECT * FROM generate_series(NULL, TIMESTAMP '2024-01-03', INTERVAL '1 day'); (0 rows) -- DuckDB 1.5.2 D SELECT * FROM generate_series(NULL, TIMESTAMP '2024-01-03', INTERVAL 1 DAY); 0 rows ``` The behaviour is pre-existing, and a precision fix does not have to fix it. But the PR body says error messages now "name the offending type", and this path proves that untrue for the function the PR is about. Pick one: - widen the dispatch to look at the first argument that is not an untyped `NULL`, or - state the gap in the body and file it. ## 2. The `ScalarValue::Null` arm is dead for the first argument `timestamp_arg_to_nanos` opens with: ```rust ScalarValue::Null => return Ok((None, None)), ``` Finding 1 explains why no SQL reaches that arm for `exprs[0]`. It is reachable for `exprs[1]` only. The unit test `timestamp_arg_handles_nulls` calls the helper directly with `"First argument"`, so it reports coverage that no query can exercise. Change the test to `"Second argument"` for that case, or add a comment that says the arm is defensive. ## 3. The mixed-zone slt case covers one order only The slt has a start in `+05:00` with an end in `America/New_York`. Swap the two and the row count changes: ``` > SELECT arrow_typeof(value), value FROM generate_series( arrow_cast(TIMESTAMP '2024-01-01T00:00:00Z','Timestamp(Microsecond, Some("America/New_York"))'), arrow_cast(TIMESTAMP '2024-01-03T00:00:00Z','Timestamp(Second, Some("+05:00"))'), INTERVAL '1 day'); +-----------------------------------+---------------------------+ | Timestamp(ns, "America/New_York") | 2024-01-01T00:00:00-05:00 | | Timestamp(ns, "America/New_York") | 2024-01-02T00:00:00-05:00 | +-----------------------------------+---------------------------+ ``` Two rows, against three in the direction the slt covers. That is correct. The two bounds denote different instants once the zones swap, and the comparison is on instants. It is also the single clearest proof of the claim in the new comment, so it earns a place in the file next to the case that is already there. ## 4. Cross-type bounds still fail, and both reference engines accept them Adjacent to the change, and worth one line in the body as a stated non-goal: ``` > SELECT * FROM generate_series(arrow_cast(TIMESTAMP '2024-01-01','Timestamp(Second, None)'), DATE '2024-01-03', INTERVAL '1 day'); Error: Error during planning: Second argument for generate_series must be a TIMESTAMP or NULL, got Date32 ``` PostgreSQL and DuckDB both return the three-row series for the same call. The new message is at least accurate now, which the old one was not. ## 5. Nit: the comment at the discard site is nine lines The block that explains `_end_tz` is good content in a place that splits `call_timestamp` in half. Two lines plus a pointer to the helper doc would read better in a function a reviewer must follow end to end. ## What I checked, and it is correct **The overflow guard is load-bearing, and the bounds are exact.** Without `checked_mul` a release build wraps in silence and emits a plausible but wrong series. I tested each coarse unit at its true boundary in both directions, and one unit past it: ``` > SELECT * FROM generate_series(arrow_cast(9223372036,'Timestamp(Second, None)'), arrow_cast(9223372036,'Timestamp(Second, None)'), INTERVAL '1 day'); 2262-04-11T23:47:16 > SELECT * FROM generate_series(arrow_cast(9223372037,'Timestamp(Second, None)'), ...); Error: Error during planning: First argument for generate_series is out of range of nanosecond timestamps: 9223372037 (Timestamp(Second, None)) is outside 1677-09-21T00:12:43.145224192 to 2262-04-11T23:47:16.854775807 > SELECT * FROM generate_series(arrow_cast(-9223372036854,'Timestamp(Millisecond, None)'), ...); 1677-09-21T00:12:43.146 > SELECT * FROM generate_series(arrow_cast(-9223372036855,'Timestamp(Millisecond, None)'), ...); Error: ... -9223372036855 (Timestamp(Millisecond, None)) is outside ... ``` Rust truncates integer division toward zero, so `i64::MIN / nanos_per_unit` is the true minimum and not one unit short of it. I checked that trap and the test gets it right. `NANOS_RANGE_MIN` and `NANOS_RANGE_MAX` are hard-coded strings, so I recomputed them from `i64::MIN` and `i64::MAX`. Both are exact to the nanosecond. The advertised window is also tight: `-9223372036855` milliseconds renders as `1677-09-21T00:12:43.145`, which is below `.145224192`, so the message never rejects a value that renders inside its own range. **The series advance was already safe.** `TimestampValue::advance` and `advance_with_end` both handle the `None` from `add_month_day_nano`, so the new multiplication is the only unchecked arithmetic the wider inputs could reach. The body's claim that no panic was reachable before holds: `call_timestamp` did no arithmetic on the bounds, and `call_date` already used `checked_mul`. **`range` shares the path.** `RangeFunc::call_with_args` builds a `GenerateSeriesFuncImpl { name: "range", include_end: false }` and delegates, so it is the same code. Confirmed end to end, including the error text: ``` > SELECT * FROM range(arrow_cast(TIMESTAMP '2024-01-01','Timestamp(Second, None)'), arrow_cast(TIMESTAMP '2024-01-03','Timestamp(Second, None)'), INTERVAL '1 day'); 2024-01-01T00:00:00 2024-01-02T00:00:00 > SELECT * FROM range(arrow_cast(9223372036854775807,'Timestamp(Microsecond, None)'), ...); Error: Error during planning: First argument for range is out of range of nanosecond timestamps: ... ``` **Mixed precisions work, and the start's zone is the right choice.** The two bounds compare as instants, so a unit difference cannot change the answer. The start's zone anchors the month and day arithmetic, and I confirmed it follows DST in that zone: ``` > SELECT value FROM generate_series( arrow_cast(TIMESTAMP '2024-03-09T00:00:00','Timestamp(Second, Some("America/Denver"))'), arrow_cast(TIMESTAMP '2024-03-12T00:00:00','Timestamp(Second, Some("America/Denver"))'), INTERVAL '1 day'); 2024-03-09T00:00:00-07:00 2024-03-10T00:00:00-07:00 2024-03-11T00:00:00-06:00 2024-03-12T00:00:00-06:00 ``` Local midnight holds across the transition. The end's zone carries no information the comparison can use, so the discard is right and no rejection is needed. **The error messages are accurate now.** On `main` the same calls give: ``` Error: Error during planning: First argument must be a timestamp or NULL, got Literal(TimestampSecond(1704067200, None), None) Error: Error during planning: Second argument must be a date or NULL, got Literal(Int64(5), None) ``` On this branch: ``` Error: Error during planning: Second argument for generate_series must be a TIMESTAMP or NULL, got Int64 Error: Error during planning: Third argument for generate_series must be an INTERVAL or NULL, got Utf8 Error: Error during planning: Second argument for generate_series must be a DATE or NULL, got Int64 Error: Error during planning: Second argument for generate_series must be a literal TIMESTAMP or NULL, got (<subquery>) ``` **The step restriction really is unreachable**, as the body claims: ``` > SELECT arrow_cast(INTERVAL '1 day', 'Interval(DayTime)'); Error: This feature is not implemented: Unsupported CAST from Interval(MonthDayNano) to Interval(DayTime) ``` **Field research supports the change.** Both engines accept coarser bounds and produce the series: ``` -- PostgreSQL 17.11 => SELECT pg_typeof(g), g FROM generate_series('2024-01-01'::timestamp(0), '2024-01-03'::timestamp(0), INTERVAL '1 day') g; timestamp without time zone | 2024-01-01 00:00:00 (3 rows) -- DuckDB 1.5.2 D SELECT * FROM generate_series(TIMESTAMP_S '2024-01-01', TIMESTAMP_S '2024-01-03', INTERVAL 1 DAY); -- 3 rows, TIMESTAMP D SELECT * FROM range(TIMESTAMP_S '2024-01-01', TIMESTAMP_S '2024-01-03', INTERVAL 1 DAY); -- 2 rows D SELECT * FROM generate_series(TIMESTAMP_S '2024-01-01', TIMESTAMP_MS '2024-01-03', INTERVAL 1 DAY); -- 3 rows, mixed units ``` Neither engine rejects a coarser input, and neither one lets the input unit reach the result type. That is the shape this PR adopts. **Suites are green on this branch**: `cargo test -p datafusion-functions-table` (13 passed), the `table_functions` sqllogictest file, and `cargo clippy --all-targets -- -D warnings`. -- 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]
