[FLINK-8650] Tests for WINDOW clause and documentation update #6226
Conversation
| @@ -139,7 +143,8 @@ select: | |||
| [ WHERE booleanExpression ] | |||
| [ GROUP BY { groupItem [, groupItem ]* } ] | |||
| [ HAVING booleanExpression ] | |||
|
|
|||
| [ WINDOW windowName AS windowSpec [, windowName AS windowSpec ]* ] | |||
There was a problem hiding this comment.
Done in a way Calcite does it
| @@ -115,6 +115,10 @@ The following BNF-grammar describes the superset of supported SQL features in ba | |||
|
|
|||
| {% highlight sql %} | |||
|
|
|||
| insert: | |||
| INSERT INTO tableReference | |||
There was a problem hiding this comment.
Move up to be sync with the similar Calcite's doc
There was a problem hiding this comment.
Thanks for the PR @snuyanzin.
I've left a few comments. Let me know what you think.
Best, Fabian
docs/dev/table/sql.md
Outdated
| [ ORDER BY orderItem [, orderItem ]* ] | ||
| [ PARTITION BY expression [, expression ]* ] | ||
| [ | ||
| RANGE numericOrIntervalExpression { PRECEDING | FOLLOWING } |
There was a problem hiding this comment.
Flink does not support FOLLOWING yet
There was a problem hiding this comment.
Yes agree I've removed it
docs/dev/table/sql.md
Outdated
| [ PARTITION BY expression [, expression ]* ] | ||
| [ | ||
| RANGE numericOrIntervalExpression { PRECEDING | FOLLOWING } | ||
| | ROWS numericExpression { PRECEDING | FOLLOWING } |
There was a problem hiding this comment.
Flink does not support FOLLOWING yet
| windowName | ||
| | windowSpec | ||
|
|
||
| windowSpec: |
There was a problem hiding this comment.
Can you also add a WINDOW example to the Over Window aggregation section in https://ci.apache.org/projects/flink/flink-docs-release-1.5/dev/table/sql.html#aggregations ?
| @@ -44,7 +44,26 @@ class OverWindowTest extends TableTestBase { | |||
| "sum(DISTINCT c) OVER (PARTITION BY b ORDER BY proctime ROWS BETWEEN 2 preceding AND " + | |||
| "CURRENT ROW) as sum2 " + | |||
| "from MyTable" | |||
|
|
|||
| val sql2 = "SELECT " + | |||
There was a problem hiding this comment.
I think we can reduce the number of tests is a bit.
We are basically testing Calcite's parser / validator multiple times with very similar queries. For example, Calcite does not distinguish between proctime and rowtime. One query with a WINDOW clause per test case should be sufficient.
There was a problem hiding this comment.
Agree. I've reduced number of queries per test case (one without WINDOW and one with). Please let me know if it looks good or not
|
@fhueske thank you for your review |
|
Thanks for the update @snuyanzin. Looks good. Will merge it. |
What is the purpose of the change
This PR adds test and documentation coverage of WINDOW clause
Brief change log
Verifying this change
This change added tests and can be verified as follows:
via running of org.apache.flink.table.api.stream.sql.OverWindowTest
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (no)Documentation