robertwb commented on code in PR #27891:
URL: https://github.com/apache/beam/pull/27891#discussion_r1293883637
##########
runners/google-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/DataflowPipelineTranslator.java:
##########
@@ -948,6 +950,18 @@ private <K, V> void groupByKeyHelper(
PropertyNames.IS_MERGING_WINDOW_FN,
!windowingStrategy.getWindowFn().isNonMerging());
}
+
+ private boolean containsCountTrigger(Trigger trigger) {
Review Comment:
I realize that triggers are mostly considered a closed set these days, but
if we ever go back to implement things like data-driven triggers those would
need to be excluded as well, so I'd be more comfortable with an allow list of
what can be lifted than a denylist of what can't. (A generic/default
implantation of recursing into sub-triggers is probably fine.)
##########
runners/google-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/DataflowPipelineTranslator.java:
##########
@@ -936,8 +937,9 @@ private <K, V> void groupByKeyHelper(
&& windowingStrategy.getWindowFn().assignsToOneWindow();
if (isStreaming) {
allowCombinerLifting &= transform.fewKeys();
- // TODO: Allow combiner lifting on the non-default trigger, as
appropriate.
- allowCombinerLifting &= (windowingStrategy.getTrigger()
instanceof DefaultTrigger);
+ // We don't currently have a good way to safely lift combiners
in the face of count triggers, as we will
+ // lose track of how many elements have been combined.
+ allowCombinerLifting &=
!containsCountTrigger(windowingStrategy.getTrigger());
Review Comment:
Any concerns about backwards compatibility 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
