[CALCITE-4176] Key descriptor can be optional in SESSION table function#2164
[CALCITE-4176] Key descriptor can be optional in SESSION table function#2164amaliujia merged 1 commit intoapache:masterfrom
Conversation
|
@liupc please let me know when this PR is ready for review. |
|
@amaliujia It's ready now! One thing may need to consider, we may also need to rework the implementation for the session window table to allow optional key column descriptor, but I prefer to open another jira/PR for it, what do you think? |
|
Thanks, I will take a look this week. yes I think for the enumerable implementation that can be updated in another PR. |
| return throwValidationSignatureErrorOrReturnFalse(callBinding, throwOnFailure); | ||
| } | ||
| if (!checkTimeColumnDescriptorOperand(callBinding, 1)) { | ||
| return throwValidationSignatureErrorOrReturnFalse(callBinding, throwOnFailure); |
There was a problem hiding this comment.
Why need this addition (as this PR is to change SESSION)? is there a test case verify this line of change?
There was a problem hiding this comment.
For TUMBLE/HOP table function, the descriptor must refer to a time column, I just add this check by the way.
currently, there are no tests to verify this change, but If it's ok to do it in this PR, I can add some tests for it.
Or maybe we can put it in another PR? what's your idea?
There was a problem hiding this comment.
I see. Please add tests to this PR (for both TUMBLE and HOP) and we can merge it together.
|
+1 |
amaliujia
added
the
LGTM-will-merge-soon
|
@liupc would you mind squashing your commits into one so I can merge this PR? |
Fix style Fix style Adding tests to check time column for TUMBLE/HOP table function Fix style
|
hi @amaliujia , It's done! |
2 participants
This PR try to make the key descriptor optional in SESSION table function