New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FLINK-24186][table-planner] Allow multiple rowtime attributes for collect() and print() #17217
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 9dabc5a (Thu Sep 09 11:57:07 UTC 2021) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we revert the documentation changes from #17183 when merging this?
@MartijnVisser the documented query is still correct. I was planning to merge this only to master, so we could revert you change only there. |
@@ -120,13 +121,16 @@ public StreamExecSink( | |||
rowtimeFieldIndices.add(i); | |||
} | |||
} | |||
|
|||
final boolean isCollectSink = tableSinkSpec.getTableSink() instanceof CollectDynamicSink; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be "cleaner" if an isCollectSink()
method (or simplyisCollect()
) is introduced to the iface?
Or alternatively, if the method is implemented in a utility class in that package, so that changing the class to public
just for the purpose of instanceof
call is avoided?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be "cleaner" if an isCollectSink() method (or simplyisCollect()) is introduced to the iface?
DynamicTableSink
is a public API, so we'd be throwing a method into it for internal purposes. Such a method also breaks the abstraction, because you're asking the interface for whether it is a specific implementation.
Or alternatively, if the method is implemented in a utility class in that package, so that changing the class to public just for the purpose of instanceof call is avoided?
Then you'd just have to make that utility public instead, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DynamicTableSink
is a public API, so we'd be throwing a method into it for internal purposes. Such a method also breaks the abstraction, because you're asking the interface for whether it is a specific implementation.
True, thx!
Then you'd just have to make that utility public instead, no?
Yep, not insisting, just to me seems a bit cleaner than exposing the class as public just to enable the instanceof
call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@flinkbot run azure |
@flinkbot run azure |
…llect() and print()
@flinkbot run azure |
…llect() and print() This closes apache#17217.
What is the purpose of the change
Reduces friction by not throwing an exception for multi-rowtime queries for
collect()
andprint()
.Verifying this change
This change added tests and can be verified as follows:
TableITCase
.Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation