You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/05/16 04:20:00 UTC
[GitHub] [calcite] amaliujia opened a new pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
amaliujia opened a new pull request #1981:
URL: https://github.com/apache/calcite/pull/1981
…. (Rui Wang)
Details in: https://jira.apache.org/jira/browse/CALCITE-4000
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#issuecomment-658406273
Ah I meant to @chunweilei, not @chunhui-shi (sorry about the confusion).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on a change in pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#discussion_r455220763
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java
##########
@@ -3406,9 +3406,22 @@ abstract Expression implementSafe(RexToLixTranslator translator,
translatedOperands.add(wmColExpr);
translatedOperands.add(intervalExpression);
- return Expressions.call(BuiltInMethod.TUMBLING.method, inputEnumerable,
- EnumUtils.tumblingWindowSelector(inputPhysType, outputPhysType,
- translatedOperands.get(0), translatedOperands.get(1)));
+ // handle the optional offset parameter. Use 0 for the default value when offset
+ // parameter is not set.
+ final Expression offsetExpr = call.getOperands().size() > 2
+ ? translator.translate(call.getOperands().get(2))
+ : Expressions.constant(0, long.class);
+ translatedOperands.add(offsetExpr);
+
+ return Expressions.call(
+ BuiltInMethod.TUMBLING.method,
+ inputEnumerable,
+ EnumUtils.tumblingWindowSelector(
+ inputPhysType,
+ outputPhysType,
+ translatedOperands.get(0),
Review comment:
OK I have updated to remove the list for all three functions.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#issuecomment-657752447
Commits squashed with `.` removed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia edited a comment on pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
amaliujia edited a comment on pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#issuecomment-658405528
Thanks @DonnyZone
cc @chunweilei this PR has a LGTM and it's very close to be merged (thus could be in 1.24.0).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on a change in pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#discussion_r455478838
##########
File path: core/src/test/resources/sql/stream.iq
##########
@@ -64,6 +78,25 @@ SELECT * FROM TABLE(HOP(TABLE ORDERS, DESCRIPTOR(ROWTIME), INTERVAL '5' MINUTE,
!ok
+SELECT * FROM TABLE(HOP(TABLE ORDERS, DESCRIPTOR(ROWTIME), INTERVAL '5' MINUTE, INTERVAL '10' MINUTE, INTERVAL '2' MINUTE));
++---------------------+----+---------+-------+---------------------+---------------------+
+| ROWTIME | ID | PRODUCT | UNITS | window_start | window_end |
++---------------------+----+---------+-------+---------------------+---------------------+
+| 2015-02-15 10:15:00 | 1 | paint | 10 | 2015-02-15 10:07:00 | 2015-02-15 10:17:00 |
+| 2015-02-15 10:15:00 | 1 | paint | 10 | 2015-02-15 10:12:00 | 2015-02-15 10:22:00 |
+| 2015-02-15 10:24:15 | 2 | paper | 5 | 2015-02-15 10:17:00 | 2015-02-15 10:27:00 |
+| 2015-02-15 10:24:15 | 2 | paper | 5 | 2015-02-15 10:22:00 | 2015-02-15 10:32:00 |
+| 2015-02-15 10:24:45 | 3 | brush | 12 | 2015-02-15 10:17:00 | 2015-02-15 10:27:00 |
+| 2015-02-15 10:24:45 | 3 | brush | 12 | 2015-02-15 10:22:00 | 2015-02-15 10:32:00 |
+| 2015-02-15 10:58:00 | 4 | paint | 3 | 2015-02-15 10:52:00 | 2015-02-15 11:02:00 |
+| 2015-02-15 10:58:00 | 4 | paint | 3 | 2015-02-15 10:57:00 | 2015-02-15 11:07:00 |
+| 2015-02-15 11:10:00 | 5 | paint | 3 | 2015-02-15 11:02:00 | 2015-02-15 11:12:00 |
Review comment:
Thanks! Logged https://issues.apache.org/jira/browse/CALCITE-4125
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] DonnyZone commented on pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#issuecomment-657400504
Please also remove "." at the end the commit message
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#issuecomment-654598258
@DonnyZone @danny0405
As people has started to discuss 1.24.0 release, do you think if we could make this change in 1.24.0?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#discussion_r454929221
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumUtils.java
##########
@@ -786,28 +787,29 @@ static Expression tumblingWindowSelector(
final Expression wmColExprToLong = EnumUtils.convert(wmColExpr, long.class);
final Expression shiftExpr = Expressions.constant(1, long.class);
- // Find the fixed window for a timestamp given a window size and return the window start.
- // Fixed windows counts from timestamp 0.
- // wmMillis / intervalMillis * intervalMillis
- expressions.add(
- Expressions.multiply(
- Expressions.divide(
+ // Find the fixed window for a timestamp given a window size and an offset, and return the
+ // window start.
+ // wmMillis - (wmColExprToLong + windowSizeMillis - offsetMillis) % windowSizeMillis
+ Expression windowStartExpr = Expressions.subtract(
Review comment:
`wmMillis ` -> `wmColExprToLong `
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on a change in pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#discussion_r455212526
##########
File path: core/src/test/resources/sql/stream.iq
##########
@@ -64,6 +78,25 @@ SELECT * FROM TABLE(HOP(TABLE ORDERS, DESCRIPTOR(ROWTIME), INTERVAL '5' MINUTE,
!ok
+SELECT * FROM TABLE(HOP(TABLE ORDERS, DESCRIPTOR(ROWTIME), INTERVAL '5' MINUTE, INTERVAL '10' MINUTE, INTERVAL '2' MINUTE));
++---------------------+----+---------+-------+---------------------+---------------------+
+| ROWTIME | ID | PRODUCT | UNITS | window_start | window_end |
++---------------------+----+---------+-------+---------------------+---------------------+
+| 2015-02-15 10:15:00 | 1 | paint | 10 | 2015-02-15 10:07:00 | 2015-02-15 10:17:00 |
+| 2015-02-15 10:15:00 | 1 | paint | 10 | 2015-02-15 10:12:00 | 2015-02-15 10:22:00 |
+| 2015-02-15 10:24:15 | 2 | paper | 5 | 2015-02-15 10:17:00 | 2015-02-15 10:27:00 |
+| 2015-02-15 10:24:15 | 2 | paper | 5 | 2015-02-15 10:22:00 | 2015-02-15 10:32:00 |
+| 2015-02-15 10:24:45 | 3 | brush | 12 | 2015-02-15 10:17:00 | 2015-02-15 10:27:00 |
+| 2015-02-15 10:24:45 | 3 | brush | 12 | 2015-02-15 10:22:00 | 2015-02-15 10:32:00 |
+| 2015-02-15 10:58:00 | 4 | paint | 3 | 2015-02-15 10:52:00 | 2015-02-15 11:02:00 |
+| 2015-02-15 10:58:00 | 4 | paint | 3 | 2015-02-15 10:57:00 | 2015-02-15 11:07:00 |
+| 2015-02-15 11:10:00 | 5 | paint | 3 | 2015-02-15 11:02:00 | 2015-02-15 11:12:00 |
Review comment:
I see. I would prefer to leave that work for another PR because Join still has some areas that need a clarification.
Join windowed streams will need more thoughts. For example, can a TUMBLE stream join HOP stream? It might make sense if there is a use case that intends to do such join, but such case might be rare. So we could end up with validating and rejecting some Join cases. And if we plan to add more constraints, then multi-way join will also become a case that need special handling because join reordering might lead to an illegal join plan (depends on the rules of allowed windowed joins though).
I feel like it would be better to promote stream to the first class citizen in Calcite then in rule matching we should consider different joins: table join table, steam join table and stream join stream, and the stream join stream will need to consider different optimization factors.
Anyway, what I want to say is Join is a big area that can be discussed and implemented separately.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on a change in pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#discussion_r455212526
##########
File path: core/src/test/resources/sql/stream.iq
##########
@@ -64,6 +78,25 @@ SELECT * FROM TABLE(HOP(TABLE ORDERS, DESCRIPTOR(ROWTIME), INTERVAL '5' MINUTE,
!ok
+SELECT * FROM TABLE(HOP(TABLE ORDERS, DESCRIPTOR(ROWTIME), INTERVAL '5' MINUTE, INTERVAL '10' MINUTE, INTERVAL '2' MINUTE));
++---------------------+----+---------+-------+---------------------+---------------------+
+| ROWTIME | ID | PRODUCT | UNITS | window_start | window_end |
++---------------------+----+---------+-------+---------------------+---------------------+
+| 2015-02-15 10:15:00 | 1 | paint | 10 | 2015-02-15 10:07:00 | 2015-02-15 10:17:00 |
+| 2015-02-15 10:15:00 | 1 | paint | 10 | 2015-02-15 10:12:00 | 2015-02-15 10:22:00 |
+| 2015-02-15 10:24:15 | 2 | paper | 5 | 2015-02-15 10:17:00 | 2015-02-15 10:27:00 |
+| 2015-02-15 10:24:15 | 2 | paper | 5 | 2015-02-15 10:22:00 | 2015-02-15 10:32:00 |
+| 2015-02-15 10:24:45 | 3 | brush | 12 | 2015-02-15 10:17:00 | 2015-02-15 10:27:00 |
+| 2015-02-15 10:24:45 | 3 | brush | 12 | 2015-02-15 10:22:00 | 2015-02-15 10:32:00 |
+| 2015-02-15 10:58:00 | 4 | paint | 3 | 2015-02-15 10:52:00 | 2015-02-15 11:02:00 |
+| 2015-02-15 10:58:00 | 4 | paint | 3 | 2015-02-15 10:57:00 | 2015-02-15 11:07:00 |
+| 2015-02-15 11:10:00 | 5 | paint | 3 | 2015-02-15 11:02:00 | 2015-02-15 11:12:00 |
Review comment:
I see. I would prefer to leave that work for another PR because Join still has some areas that need a clarification.
Join windowed streamed will need more thoughts. For example, can a TUMBLE stream join HOP stream? It might make sense if there is a use case that intends to do such join, but such case might be rare. So we could end up with validating some Join cases. That can be put into a single PR to discuss and finalize.
##########
File path: core/src/test/resources/sql/stream.iq
##########
@@ -64,6 +78,25 @@ SELECT * FROM TABLE(HOP(TABLE ORDERS, DESCRIPTOR(ROWTIME), INTERVAL '5' MINUTE,
!ok
+SELECT * FROM TABLE(HOP(TABLE ORDERS, DESCRIPTOR(ROWTIME), INTERVAL '5' MINUTE, INTERVAL '10' MINUTE, INTERVAL '2' MINUTE));
++---------------------+----+---------+-------+---------------------+---------------------+
+| ROWTIME | ID | PRODUCT | UNITS | window_start | window_end |
++---------------------+----+---------+-------+---------------------+---------------------+
+| 2015-02-15 10:15:00 | 1 | paint | 10 | 2015-02-15 10:07:00 | 2015-02-15 10:17:00 |
+| 2015-02-15 10:15:00 | 1 | paint | 10 | 2015-02-15 10:12:00 | 2015-02-15 10:22:00 |
+| 2015-02-15 10:24:15 | 2 | paper | 5 | 2015-02-15 10:17:00 | 2015-02-15 10:27:00 |
+| 2015-02-15 10:24:15 | 2 | paper | 5 | 2015-02-15 10:22:00 | 2015-02-15 10:32:00 |
+| 2015-02-15 10:24:45 | 3 | brush | 12 | 2015-02-15 10:17:00 | 2015-02-15 10:27:00 |
+| 2015-02-15 10:24:45 | 3 | brush | 12 | 2015-02-15 10:22:00 | 2015-02-15 10:32:00 |
+| 2015-02-15 10:58:00 | 4 | paint | 3 | 2015-02-15 10:52:00 | 2015-02-15 11:02:00 |
+| 2015-02-15 10:58:00 | 4 | paint | 3 | 2015-02-15 10:57:00 | 2015-02-15 11:07:00 |
+| 2015-02-15 11:10:00 | 5 | paint | 3 | 2015-02-15 11:02:00 | 2015-02-15 11:12:00 |
Review comment:
I see. I would prefer to leave that work for another PR because Join still has some areas that need a clarification.
Join windowed streams will need more thoughts. For example, can a TUMBLE stream join HOP stream? It might make sense if there is a use case that intends to do such join, but such case might be rare. So we could end up with validating some Join cases. That can be put into a single PR to discuss and finalize.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] DonnyZone commented on pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#issuecomment-657930529
LGTM!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#discussion_r455463422
##########
File path: core/src/test/resources/sql/stream.iq
##########
@@ -64,6 +78,25 @@ SELECT * FROM TABLE(HOP(TABLE ORDERS, DESCRIPTOR(ROWTIME), INTERVAL '5' MINUTE,
!ok
+SELECT * FROM TABLE(HOP(TABLE ORDERS, DESCRIPTOR(ROWTIME), INTERVAL '5' MINUTE, INTERVAL '10' MINUTE, INTERVAL '2' MINUTE));
++---------------------+----+---------+-------+---------------------+---------------------+
+| ROWTIME | ID | PRODUCT | UNITS | window_start | window_end |
++---------------------+----+---------+-------+---------------------+---------------------+
+| 2015-02-15 10:15:00 | 1 | paint | 10 | 2015-02-15 10:07:00 | 2015-02-15 10:17:00 |
+| 2015-02-15 10:15:00 | 1 | paint | 10 | 2015-02-15 10:12:00 | 2015-02-15 10:22:00 |
+| 2015-02-15 10:24:15 | 2 | paper | 5 | 2015-02-15 10:17:00 | 2015-02-15 10:27:00 |
+| 2015-02-15 10:24:15 | 2 | paper | 5 | 2015-02-15 10:22:00 | 2015-02-15 10:32:00 |
+| 2015-02-15 10:24:45 | 3 | brush | 12 | 2015-02-15 10:17:00 | 2015-02-15 10:27:00 |
+| 2015-02-15 10:24:45 | 3 | brush | 12 | 2015-02-15 10:22:00 | 2015-02-15 10:32:00 |
+| 2015-02-15 10:58:00 | 4 | paint | 3 | 2015-02-15 10:52:00 | 2015-02-15 11:02:00 |
+| 2015-02-15 10:58:00 | 4 | paint | 3 | 2015-02-15 10:57:00 | 2015-02-15 11:07:00 |
+| 2015-02-15 11:10:00 | 5 | paint | 3 | 2015-02-15 11:02:00 | 2015-02-15 11:12:00 |
Review comment:
Fine, then please log an issue there ~
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
danny0405 commented on pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#issuecomment-658630745
> Could someone take a final review today or tomorrow? I am planning to have an RC around the 14 of July.
I will do it tomorrow morning ~
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#issuecomment-649831503
Rebased on most-recent master.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on a change in pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#discussion_r453886468
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumUtils.java
##########
@@ -786,28 +787,29 @@ static Expression tumblingWindowSelector(
final Expression wmColExprToLong = EnumUtils.convert(wmColExpr, long.class);
final Expression shiftExpr = Expressions.constant(1, long.class);
- // Find the fixed window for a timestamp given a window size and return the window start.
- // Fixed windows counts from timestamp 0.
- // wmMillis / intervalMillis * intervalMillis
- expressions.add(
- Expressions.multiply(
- Expressions.divide(
+ // Find the fixed window for a timestamp given a window size and an offset and return the
Review comment:
Done.
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java
##########
@@ -3426,13 +3440,24 @@ abstract Expression implementSafe(RexToLixTranslator translator,
translatedOperands.add(slidingInterval);
translatedOperands.add(windowSize);
- return Expressions.call(BuiltInMethod.HOPPING.method,
+
+ // handle the optional offset parameter.
+ if (call.getOperands().size() > 3) {
+ translatedOperands.add(translator.translate(call.getOperands().get(3)));
+ } else {
+ // use 0 for the default value when offset parameter is missing.
Review comment:
+1
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on a change in pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#discussion_r455212526
##########
File path: core/src/test/resources/sql/stream.iq
##########
@@ -64,6 +78,25 @@ SELECT * FROM TABLE(HOP(TABLE ORDERS, DESCRIPTOR(ROWTIME), INTERVAL '5' MINUTE,
!ok
+SELECT * FROM TABLE(HOP(TABLE ORDERS, DESCRIPTOR(ROWTIME), INTERVAL '5' MINUTE, INTERVAL '10' MINUTE, INTERVAL '2' MINUTE));
++---------------------+----+---------+-------+---------------------+---------------------+
+| ROWTIME | ID | PRODUCT | UNITS | window_start | window_end |
++---------------------+----+---------+-------+---------------------+---------------------+
+| 2015-02-15 10:15:00 | 1 | paint | 10 | 2015-02-15 10:07:00 | 2015-02-15 10:17:00 |
+| 2015-02-15 10:15:00 | 1 | paint | 10 | 2015-02-15 10:12:00 | 2015-02-15 10:22:00 |
+| 2015-02-15 10:24:15 | 2 | paper | 5 | 2015-02-15 10:17:00 | 2015-02-15 10:27:00 |
+| 2015-02-15 10:24:15 | 2 | paper | 5 | 2015-02-15 10:22:00 | 2015-02-15 10:32:00 |
+| 2015-02-15 10:24:45 | 3 | brush | 12 | 2015-02-15 10:17:00 | 2015-02-15 10:27:00 |
+| 2015-02-15 10:24:45 | 3 | brush | 12 | 2015-02-15 10:22:00 | 2015-02-15 10:32:00 |
+| 2015-02-15 10:58:00 | 4 | paint | 3 | 2015-02-15 10:52:00 | 2015-02-15 11:02:00 |
+| 2015-02-15 10:58:00 | 4 | paint | 3 | 2015-02-15 10:57:00 | 2015-02-15 11:07:00 |
+| 2015-02-15 11:10:00 | 5 | paint | 3 | 2015-02-15 11:02:00 | 2015-02-15 11:12:00 |
Review comment:
I see. I would prefer to leave that work for another PR because Join still has some areas that need a clarification.
Join windowed streams will need more thoughts. For example, can a TUMBLE stream join HOP stream? It might make sense if there is a use case that intends to do such join, but such case might be rare. So we could end up with validating some Join cases. And if we plan to add more constraints, then multi-way join will also become a case that need special handling because join reordering might lead to an illegal join plan (depends on the rules of allowed windowed joins though).
I feel like it would be better to promote stream to the first class citizen in Calcite then in rule matching we should consider different joins: table join table, steam join table and stream join stream, and the stream join stream will need to consider different optimization factors.
Anyway, what I want to say is Join is a big area that can be discussed and implemented separately.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] DonnyZone commented on a change in pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#discussion_r453469608
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java
##########
@@ -3406,9 +3406,23 @@ abstract Expression implementSafe(RexToLixTranslator translator,
translatedOperands.add(wmColExpr);
translatedOperands.add(intervalExpression);
- return Expressions.call(BuiltInMethod.TUMBLING.method, inputEnumerable,
- EnumUtils.tumblingWindowSelector(inputPhysType, outputPhysType,
- translatedOperands.get(0), translatedOperands.get(1)));
+ // handle the optional offset parameter.
+ if (call.getOperands().size() > 2) {
+ translatedOperands.add(translator.translate(call.getOperands().get(2)));
+ } else {
+ // use 0 for the default value when offset parameter is not set.
Review comment:
Can we simpilfy this logic as below? It may be more intuitive.
```
// handle the optional offset parameter, use 0 as default when ...
final Expression xxxExpression = call.getOperands().size() > 2 ? translator.translate(call.getOperands().get(2)) : Expressions.constant(0, long.class);
translatedOperands.add(xxxExpression);
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] DonnyZone commented on a change in pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#discussion_r453473001
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java
##########
@@ -3426,13 +3440,24 @@ abstract Expression implementSafe(RexToLixTranslator translator,
translatedOperands.add(slidingInterval);
translatedOperands.add(windowSize);
- return Expressions.call(BuiltInMethod.HOPPING.method,
+
+ // handle the optional offset parameter.
+ if (call.getOperands().size() > 3) {
+ translatedOperands.add(translator.translate(call.getOperands().get(3)));
+ } else {
+ // use 0 for the default value when offset parameter is missing.
Review comment:
Also here
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] DonnyZone commented on a change in pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#discussion_r453469608
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java
##########
@@ -3406,9 +3406,23 @@ abstract Expression implementSafe(RexToLixTranslator translator,
translatedOperands.add(wmColExpr);
translatedOperands.add(intervalExpression);
- return Expressions.call(BuiltInMethod.TUMBLING.method, inputEnumerable,
- EnumUtils.tumblingWindowSelector(inputPhysType, outputPhysType,
- translatedOperands.get(0), translatedOperands.get(1)));
+ // handle the optional offset parameter.
+ if (call.getOperands().size() > 2) {
+ translatedOperands.add(translator.translate(call.getOperands().get(2)));
+ } else {
+ // use 0 for the default value when offset parameter is not set.
Review comment:
Can we simpilfy this logic as below? It may be more intuitive.
```
// handle the optional offset parameter, use 0 as default when ...
final Expression xxxExpression = call.getOperands().size() > 2
? translator.translate(call.getOperands().get(2))
: Expressions.constant(0, long.class);
translatedOperands.add(xxxExpression);
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#discussion_r454930502
##########
File path: core/src/test/resources/sql/stream.iq
##########
@@ -64,6 +78,25 @@ SELECT * FROM TABLE(HOP(TABLE ORDERS, DESCRIPTOR(ROWTIME), INTERVAL '5' MINUTE,
!ok
+SELECT * FROM TABLE(HOP(TABLE ORDERS, DESCRIPTOR(ROWTIME), INTERVAL '5' MINUTE, INTERVAL '10' MINUTE, INTERVAL '2' MINUTE));
++---------------------+----+---------+-------+---------------------+---------------------+
+| ROWTIME | ID | PRODUCT | UNITS | window_start | window_end |
++---------------------+----+---------+-------+---------------------+---------------------+
+| 2015-02-15 10:15:00 | 1 | paint | 10 | 2015-02-15 10:07:00 | 2015-02-15 10:17:00 |
+| 2015-02-15 10:15:00 | 1 | paint | 10 | 2015-02-15 10:12:00 | 2015-02-15 10:22:00 |
+| 2015-02-15 10:24:15 | 2 | paper | 5 | 2015-02-15 10:17:00 | 2015-02-15 10:27:00 |
+| 2015-02-15 10:24:15 | 2 | paper | 5 | 2015-02-15 10:22:00 | 2015-02-15 10:32:00 |
+| 2015-02-15 10:24:45 | 3 | brush | 12 | 2015-02-15 10:17:00 | 2015-02-15 10:27:00 |
+| 2015-02-15 10:24:45 | 3 | brush | 12 | 2015-02-15 10:22:00 | 2015-02-15 10:32:00 |
+| 2015-02-15 10:58:00 | 4 | paint | 3 | 2015-02-15 10:52:00 | 2015-02-15 11:02:00 |
+| 2015-02-15 10:58:00 | 4 | paint | 3 | 2015-02-15 10:57:00 | 2015-02-15 11:07:00 |
+| 2015-02-15 11:10:00 | 5 | paint | 3 | 2015-02-15 11:02:00 | 2015-02-15 11:12:00 |
Review comment:
Can we try to add cases for join two window stream ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on a change in pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#discussion_r455221086
##########
File path: site/_docs/reference.md
##########
@@ -1890,7 +1890,7 @@ on a timestamp column. Windows assigned could have overlapping so hopping someti
| Operator syntax | Description
|:-------------------- |:-----------
-| HOP(table, DESCRIPTOR(datetime), slide, size) | Indicates a hopping window for *datetime*, covering rows within the interval of *size*, shifting every *slide*.
+| HOP(table, DESCRIPTOR(datetime), slide, size [, time ]) | Indicates a hopping window for *datetime*, covering rows within the interval of *size*, shifting every *slide* and optionally aligned at time.
Review comment:
Agreed. Done!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#issuecomment-658940552
Squashed commits to re-trigger tests.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on a change in pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#discussion_r455212526
##########
File path: core/src/test/resources/sql/stream.iq
##########
@@ -64,6 +78,25 @@ SELECT * FROM TABLE(HOP(TABLE ORDERS, DESCRIPTOR(ROWTIME), INTERVAL '5' MINUTE,
!ok
+SELECT * FROM TABLE(HOP(TABLE ORDERS, DESCRIPTOR(ROWTIME), INTERVAL '5' MINUTE, INTERVAL '10' MINUTE, INTERVAL '2' MINUTE));
++---------------------+----+---------+-------+---------------------+---------------------+
+| ROWTIME | ID | PRODUCT | UNITS | window_start | window_end |
++---------------------+----+---------+-------+---------------------+---------------------+
+| 2015-02-15 10:15:00 | 1 | paint | 10 | 2015-02-15 10:07:00 | 2015-02-15 10:17:00 |
+| 2015-02-15 10:15:00 | 1 | paint | 10 | 2015-02-15 10:12:00 | 2015-02-15 10:22:00 |
+| 2015-02-15 10:24:15 | 2 | paper | 5 | 2015-02-15 10:17:00 | 2015-02-15 10:27:00 |
+| 2015-02-15 10:24:15 | 2 | paper | 5 | 2015-02-15 10:22:00 | 2015-02-15 10:32:00 |
+| 2015-02-15 10:24:45 | 3 | brush | 12 | 2015-02-15 10:17:00 | 2015-02-15 10:27:00 |
+| 2015-02-15 10:24:45 | 3 | brush | 12 | 2015-02-15 10:22:00 | 2015-02-15 10:32:00 |
+| 2015-02-15 10:58:00 | 4 | paint | 3 | 2015-02-15 10:52:00 | 2015-02-15 11:02:00 |
+| 2015-02-15 10:58:00 | 4 | paint | 3 | 2015-02-15 10:57:00 | 2015-02-15 11:07:00 |
+| 2015-02-15 11:10:00 | 5 | paint | 3 | 2015-02-15 11:02:00 | 2015-02-15 11:12:00 |
Review comment:
I see. I would prefer to leave that work for another PR because Join still has some areas needs a clarification.
Join windowed streamed will need more thoughts. For example, can a TUMBLE stream join HOP stream? It might make sense if there is a use case that intends to do such join, but such case might be rare. So we could end up with validating some Join cases. That can be put into a single PR to discuss and finalize.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on a change in pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#discussion_r455206406
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java
##########
@@ -3406,9 +3406,22 @@ abstract Expression implementSafe(RexToLixTranslator translator,
translatedOperands.add(wmColExpr);
translatedOperands.add(intervalExpression);
- return Expressions.call(BuiltInMethod.TUMBLING.method, inputEnumerable,
- EnumUtils.tumblingWindowSelector(inputPhysType, outputPhysType,
- translatedOperands.get(0), translatedOperands.get(1)));
+ // handle the optional offset parameter. Use 0 for the default value when offset
+ // parameter is not set.
+ final Expression offsetExpr = call.getOperands().size() > 2
+ ? translator.translate(call.getOperands().get(2))
+ : Expressions.constant(0, long.class);
+ translatedOperands.add(offsetExpr);
+
+ return Expressions.call(
+ BuiltInMethod.TUMBLING.method,
+ inputEnumerable,
+ EnumUtils.tumblingWindowSelector(
+ inputPhysType,
+ outputPhysType,
+ translatedOperands.get(0),
Review comment:
Ah I think `translatedOperands` might be easy to read. For example, it can be mapped to function signature. When I write such code, I tend to map parameters to offsets. WatermarkCol expression is the first parameter, then others. Maybe not everyone thinks in this way.
I guess you are thinking why not directly use expressions?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#discussion_r454924400
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java
##########
@@ -3406,9 +3406,22 @@ abstract Expression implementSafe(RexToLixTranslator translator,
translatedOperands.add(wmColExpr);
translatedOperands.add(intervalExpression);
- return Expressions.call(BuiltInMethod.TUMBLING.method, inputEnumerable,
- EnumUtils.tumblingWindowSelector(inputPhysType, outputPhysType,
- translatedOperands.get(0), translatedOperands.get(1)));
+ // handle the optional offset parameter. Use 0 for the default value when offset
+ // parameter is not set.
+ final Expression offsetExpr = call.getOperands().size() > 2
+ ? translator.translate(call.getOperands().get(2))
+ : Expressions.constant(0, long.class);
+ translatedOperands.add(offsetExpr);
+
+ return Expressions.call(
+ BuiltInMethod.TUMBLING.method,
+ inputEnumerable,
+ EnumUtils.tumblingWindowSelector(
+ inputPhysType,
+ outputPhysType,
+ translatedOperands.get(0),
Review comment:
Is there any special reason we must use a list here to keep the translatedOperands ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#issuecomment-658405528
Thanks @DonnyZone
cc @chunhui-shi this PR has a LGTM and it's very close to be merged (thus could be in 1.24.0).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on a change in pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#discussion_r455212526
##########
File path: core/src/test/resources/sql/stream.iq
##########
@@ -64,6 +78,25 @@ SELECT * FROM TABLE(HOP(TABLE ORDERS, DESCRIPTOR(ROWTIME), INTERVAL '5' MINUTE,
!ok
+SELECT * FROM TABLE(HOP(TABLE ORDERS, DESCRIPTOR(ROWTIME), INTERVAL '5' MINUTE, INTERVAL '10' MINUTE, INTERVAL '2' MINUTE));
++---------------------+----+---------+-------+---------------------+---------------------+
+| ROWTIME | ID | PRODUCT | UNITS | window_start | window_end |
++---------------------+----+---------+-------+---------------------+---------------------+
+| 2015-02-15 10:15:00 | 1 | paint | 10 | 2015-02-15 10:07:00 | 2015-02-15 10:17:00 |
+| 2015-02-15 10:15:00 | 1 | paint | 10 | 2015-02-15 10:12:00 | 2015-02-15 10:22:00 |
+| 2015-02-15 10:24:15 | 2 | paper | 5 | 2015-02-15 10:17:00 | 2015-02-15 10:27:00 |
+| 2015-02-15 10:24:15 | 2 | paper | 5 | 2015-02-15 10:22:00 | 2015-02-15 10:32:00 |
+| 2015-02-15 10:24:45 | 3 | brush | 12 | 2015-02-15 10:17:00 | 2015-02-15 10:27:00 |
+| 2015-02-15 10:24:45 | 3 | brush | 12 | 2015-02-15 10:22:00 | 2015-02-15 10:32:00 |
+| 2015-02-15 10:58:00 | 4 | paint | 3 | 2015-02-15 10:52:00 | 2015-02-15 11:02:00 |
+| 2015-02-15 10:58:00 | 4 | paint | 3 | 2015-02-15 10:57:00 | 2015-02-15 11:07:00 |
+| 2015-02-15 11:10:00 | 5 | paint | 3 | 2015-02-15 11:02:00 | 2015-02-15 11:12:00 |
Review comment:
I see. I would prefer to leave that work for another PR because Join still has some areas that need a clarification.
Join windowed streams will need more thoughts. For example, can a TUMBLE stream join HOP stream? It might make sense if there is a use case that intends to do such join, but such case might be rare. So we could end up with validating and rejecting some Join cases. And if we plan to add more constraints, then multi-way join will also become a case that need special handling because join reordering might lead to an illegal join plan (depends on the rules of allowed windowed joins though).
I feel like it would be better to promote stream to the first class citizen in Calcite then in rule matching we should consider different joins: table join table, steam join table and stream join stream, and then join involves streams might need to consider different optimization factors. e.g. estimated biggest window size? number events per second? etc.
Anyway, what I want to say is Join is a big area that can be discussed and implemented separately.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] DonnyZone merged pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
DonnyZone merged pull request #1981:
URL: https://github.com/apache/calcite/pull/1981
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] DonnyZone commented on pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#issuecomment-655240992
@amaliujia Sorry for the late reply. Yes, I think we can merge this feature in 1.24.0.
I will take a look next week.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
danny0405 commented on pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#issuecomment-658521589
> Hi @danny0405, do you have time to take a final review? This feature is going to be merged into 1.24.0.
I would, but maybe this weekend.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#issuecomment-638588987
A friendly ping~
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on a change in pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#discussion_r455206406
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java
##########
@@ -3406,9 +3406,22 @@ abstract Expression implementSafe(RexToLixTranslator translator,
translatedOperands.add(wmColExpr);
translatedOperands.add(intervalExpression);
- return Expressions.call(BuiltInMethod.TUMBLING.method, inputEnumerable,
- EnumUtils.tumblingWindowSelector(inputPhysType, outputPhysType,
- translatedOperands.get(0), translatedOperands.get(1)));
+ // handle the optional offset parameter. Use 0 for the default value when offset
+ // parameter is not set.
+ final Expression offsetExpr = call.getOperands().size() > 2
+ ? translator.translate(call.getOperands().get(2))
+ : Expressions.constant(0, long.class);
+ translatedOperands.add(offsetExpr);
+
+ return Expressions.call(
+ BuiltInMethod.TUMBLING.method,
+ inputEnumerable,
+ EnumUtils.tumblingWindowSelector(
+ inputPhysType,
+ outputPhysType,
+ translatedOperands.get(0),
Review comment:
Ah I think `translatedOperands` might be easy to read. For example, it can be mapped to function signature. When I write such code, I tend to map parameters to offsets. WatermarkCol expression is the first parameter, then others. Maybe not everyone thinks in this way.
I guess you are thinking why not directly use expressions? Either is fine to me.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] DonnyZone commented on a change in pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#discussion_r453473647
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumUtils.java
##########
@@ -786,28 +787,29 @@ static Expression tumblingWindowSelector(
final Expression wmColExprToLong = EnumUtils.convert(wmColExpr, long.class);
final Expression shiftExpr = Expressions.constant(1, long.class);
- // Find the fixed window for a timestamp given a window size and return the window start.
- // Fixed windows counts from timestamp 0.
- // wmMillis / intervalMillis * intervalMillis
- expressions.add(
- Expressions.multiply(
- Expressions.divide(
+ // Find the fixed window for a timestamp given a window size and an offset and return the
Review comment:
"...a window size and an offset and return..." -> "a window size and an offset, and return"
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#issuecomment-629585828
R: @DonnyZone @danny0405
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] DonnyZone commented on pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#issuecomment-657935797
Hi @danny0405, do you have time to take a final review? This feature is going to be merged into 1.24.0.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#discussion_r454928540
##########
File path: site/_docs/reference.md
##########
@@ -1890,7 +1890,7 @@ on a timestamp column. Windows assigned could have overlapping so hopping someti
| Operator syntax | Description
|:-------------------- |:-----------
-| HOP(table, DESCRIPTOR(datetime), slide, size) | Indicates a hopping window for *datetime*, covering rows within the interval of *size*, shifting every *slide*.
+| HOP(table, DESCRIPTOR(datetime), slide, size [, time ]) | Indicates a hopping window for *datetime*, covering rows within the interval of *size*, shifting every *slide* and optionally aligned at time.
Review comment:
I prefer to name the time to offset, which is more straight-forward.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on a change in pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#discussion_r453886553
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java
##########
@@ -3406,9 +3406,23 @@ abstract Expression implementSafe(RexToLixTranslator translator,
translatedOperands.add(wmColExpr);
translatedOperands.add(intervalExpression);
- return Expressions.call(BuiltInMethod.TUMBLING.method, inputEnumerable,
- EnumUtils.tumblingWindowSelector(inputPhysType, outputPhysType,
- translatedOperands.get(0), translatedOperands.get(1)));
+ // handle the optional offset parameter.
+ if (call.getOperands().size() > 2) {
+ translatedOperands.add(translator.translate(call.getOperands().get(2)));
+ } else {
+ // use 0 for the default value when offset parameter is not set.
Review comment:
Done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] chunweilei commented on pull request #1981: [CALCITE-4000] Support OFFSET parameter in TUMBLE/HOP table functions…
Posted by GitBox <gi...@apache.org>.
chunweilei commented on pull request #1981:
URL: https://github.com/apache/calcite/pull/1981#issuecomment-658564188
Could someone take a final review today or tomorrow? I am planning to have an RC around the 14 of July.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org