Conversation
mythical-fred
left a comment
There was a problem hiding this comment.
Two issues to address before merge:
1. ROWS frame is documented as unsupported (see docs.feldera.com/docs/sql/unsupported-operations.md). The rolling_sum view uses ROWS BETWEEN 2 PRECEDING AND CURRENT ROW. Either add expected_error = ... if it's expected to fail, or update the docs if it now works.
2. No expected data on any view. All TstView subclasses omit self.data, so the framework compares against []. The tests verify the SQL compiles and runs without crashing, but not that the results are correct. The comment at the top of the file should say this explicitly. Ideally at least one view has expected output.
|
FYI: PR #5876 (just merged) adds a compiler error for ROWS frame window queries, which means in this test will now fail at compile time. You'll want to rebase and either remove that view or add |
|
Fixed by PR #5876 which just merged — compiler now rejects ROWS frames at compile time. The test likely passed before because the Python test runner did not catch the silent Rust compilation failure. Once rivudhk rebases, the issue will surface as a proper compiler error and can be annotated with |
09f1e61 to
3202fa8
Compare
mythical-fred
left a comment
There was a problem hiding this comment.
Both blockers addressed: ROWS view now has expected_error, and the docstring explicitly documents that these are compile-only tests. Nit: the commit subject still has a typo ("propagatation" → "propagation") — worth fixing with git commit --amend before merge. LGTM otherwise.
Signed-off-by: rivudhk <rivudhkr@gmail.com>
| WITH ('emit_final' = 'w_start') | ||
| AS | ||
| SELECT | ||
| TUMBLE_START(ts, INTERVAL '1' DAY) AS w_start, |
There was a problem hiding this comment.
tumble_Start works, but we prefer using just the TUMBLE table function, which adds two columns window_start and window_end. There are examples in Java. You should add such tests too.
| AS | ||
| SELECT | ||
| ts, | ||
| SUM(value) OVER (ORDER BY ts ROWS BETWEEN 2 PRECEDING AND CURRENT ROW) AS rolling_sum |
There was a problem hiding this comment.
We would support a similar query with RANGE instead of ROWS (and different BETWEEN expressions)
Add tests for lateness propagation.