[FLINK-19605][table-runtime-blink] Implement cumulative windowing for window aggregate operator #13650

wuchong · 2020-10-15T12:02:02Z

What is the purpose of the change

Implements the cumulative window assigner and supports cumulative window aggregate in WindowOperator.

Brief change log

Add CumulativeWindowAssigner
Add cumulative window builder method in WindowOperatorBuilder

Verifying this change

Add CumulativeWindowAssignerTest to test CumulativeWindowAssigner.
Add harness tests for cumulative window aggregate in WindowOperatorTest.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
The serializers: (yes / no / don't know)
The runtime per-record code paths (performance sensitive): (yes / no / don't know)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (yes / no / don't know)
The S3 file system connector: (yes / no / don't know)

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

… window aggregate operator

wuchong · 2020-10-15T12:02:15Z

cc @danny0405

flinkbot · 2020-10-15T12:05:00Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 2cd041a (Fri May 28 07:11:56 UTC 2021)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

Details

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 commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

flinkbot · 2020-10-15T12:19:28Z

CI report:

2cd041a Azure: FAILURE

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build
@flinkbot run azure re-run the last Azure build

danny0405 · 2020-10-19T07:48:13Z

...java/org/apache/flink/table/runtime/operators/window/assigners/CumulativeWindowAssigner.java

+		this.offset = offset;
+		this.isEventTime = isEventTime;
+		this.paneSize = step;
+	}


If paneSize always equals step, keep only step is enough, because we always have limitation size must be an integral multiple of step.

danny0405 · 2020-10-19T07:49:53Z

...java/org/apache/flink/table/runtime/operators/window/assigners/CumulativeWindowAssigner.java

+		}
+
+		this.size = size;
+		this.step = step;


Do we support size argument that is not 1 day ? Say 2 days or 3 days ?

We should name it the maxSize because the window size is actually increasing.

Yes. We can support 2 days max size.

danny0405 · 2020-10-19T07:56:32Z

...-blink/src/test/java/org/apache/flink/table/runtime/operators/window/WindowOperatorTest.java

+		assertor.assertOutputEqualsSorted("Output was not correct.", expectedOutput, testHarness.getOutput());
+
+		testHarness.processWatermark(new Watermark(5999));
+		expectedOutput.addAll(doubleRecord(isTableAggregate, insertRecord("key2", 1L, 1L, 3000L, 6000L, 5999L)));


We may also need to add a test case for the late arrive data.

wuchong · 2020-10-20T04:16:31Z

@danny0405 please have a look again.

danny0405 · 2020-10-20T04:33:31Z

...java/org/apache/flink/table/runtime/operators/window/assigners/CumulativeWindowAssigner.java

+	 * @return The time policy.
+	 */
+	public static CumulativeWindowAssigner of(Duration size, Duration step) {
+		return new CumulativeWindowAssigner(size.toMillis(), step.toMillis(), 0, true);


size -> maxSize

wuchong · 2020-10-21T02:38:51Z

The failed Azure is the unstable e2e tests. This change shouldn't affect e2e part.

Merging...

HuangZhenQiu · 2021-01-21T21:12:48Z

@danny0405 @wuchong
As I remember, there was a FLIP talking about the cumulative window. But I can't find it right now. Would you please point me to the design doc? We have some similar requirements internally, so want to align more on the functionality with you guys.

wuchong · 2021-01-22T02:21:56Z

@HuangZhenQiu , here is the link https://cwiki.apache.org/confluence/display/FLINK/FLIP-145%3A+Support+SQL+windowing+table-valued+function

[FLINK-19605][table-runtime-blink] Implement cumulative windowing for…

6f7b817

… window aggregate operator

rmetzger added review=description? component=TableSQL/Runtime labels Oct 15, 2020

danny0405 reviewed Oct 19, 2020

View reviewed changes

address review comment

26d7b65

danny0405 reviewed Oct 20, 2020

View reviewed changes

size -> maxSize

2cd041a

danny0405 approved these changes Oct 21, 2020

View reviewed changes

wuchong merged commit 9423cf1 into apache:master Oct 21, 2020

wuchong deleted the cumulative-agg-runtime branch October 21, 2020 02:40

[FLINK-19605][table-runtime-blink] Implement cumulative windowing for window aggregate operator #13650

[FLINK-19605][table-runtime-blink] Implement cumulative windowing for window aggregate operator #13650

Uh oh!

Conversation

wuchong commented Oct 15, 2020

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Uh oh!

wuchong commented Oct 15, 2020

Uh oh!

flinkbot commented Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automated Checks

Review Progress

Uh oh!

flinkbot commented Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

Uh oh!

danny0405 Oct 19, 2020

Choose a reason for hiding this comment

Uh oh!

danny0405 Oct 19, 2020

Choose a reason for hiding this comment

Uh oh!

danny0405 Oct 19, 2020

Choose a reason for hiding this comment

Uh oh!

wuchong Oct 19, 2020

Choose a reason for hiding this comment

Uh oh!

danny0405 Oct 19, 2020

Choose a reason for hiding this comment

Uh oh!

wuchong commented Oct 20, 2020

Uh oh!

danny0405 Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

wuchong commented Oct 21, 2020

Uh oh!

HuangZhenQiu commented Jan 21, 2021

Uh oh!

wuchong commented Jan 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

flinkbot commented Oct 15, 2020 •

edited

Loading

flinkbot commented Oct 15, 2020 •

edited

Loading