[SQL] Support for range aggregates sorting on intervals, booleans#6310
Conversation
mythical-fred
left a comment
There was a problem hiding this comment.
Two real findings worth fixing inline; rest are nits. The NULLS FIRST sub-cases in issue457b/c don't actually exercise NULLS FIRST (no-op string replace), so the new nullable-interval radix-sort path is only covered by the default ordering. Otherwise the wiring looks consistent with the existing INTERVAL/UUID pattern.
|
|
||
| #[doc(hidden)] | ||
| #[inline(always)] | ||
| pub fn wrap_bool(b: Option<bool>) -> bool { |
There was a problem hiding this comment.
many of these functions were moved from lib.rs
mythical-fred
left a comment
There was a problem hiding this comment.
Quick follow-up after the force-push. Most of my prior nits are addressed (the HOURS/MONTHS replace targets now actually match the program text, the five-digit-year typo is gone, the LONG_INTERVAL_TO_INTEGER comment is correct). Two from last round are still live:
issue457cNULLS FIRST sub-case is still a no-op (replace target doesn't match program text).error.rs:from_strngtypo and the"NULL result produced"message survived. These are now on the hot path for nullable BOOL/INTERVAL range aggs, so worth a once-over.
Nothing here blocks the behaviour change; tests cover the substantive new code paths.
| var ccs = this.getCCS(program); | ||
| ccs.stepWeightOne(data, expected); | ||
|
|
||
| var program1 = program.replace("ORDER BY seq", "ORDER BY seq NULLS FIRST"); |
There was a problem hiding this comment.
Same issue I flagged last round, still here: this program's ORDER BY clause is ORDER BY (d1 - d2) MONTHS, not ORDER BY seq. The replace is a no-op and the NULLS-FIRST sub-case silently re-runs the default-ordering case. The MONTHS -> MONTHS NULLS LAST substitution on line 281 is the correct shape; line 277 should follow suit (MONTHS -> MONTHS NULLS FIRST).
| pub fn unwrap_sql_result<T>(data: SqlResult<Option<T>>) -> SqlResult<T> { | ||
| match data { | ||
| Err(e) => Err(e), | ||
| Ok(None) => Err(SqlRuntimeError::from_strng("NULL result produced")), |
There was a problem hiding this comment.
Re-flagging from last round (not a blocker): from_strng is a typo for from_string, and "NULL result produced" is non-actionable for end users now that nullable BOOL/INTERVAL ordering routes through here. Worth tightening the message (which SQL op, or at least "NULL result in non-nullable context") while we're in the area.
mythical-fred
left a comment
There was a problem hiding this comment.
LGTM — all prior findings addressed.
39ddf8b to
6196492
Compare
Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
Part of #457
Range aggregates use radix-sort on unsigned values up to 128 bits. Any other type must be converted back and forth to some unsigned type (including the NULL value if the type is nullable). This PR adds support for BOOL, short and long intervals.
Checklist