[FLINK-19896][table-blink] Improve first-n-row fetching in the Rank o… #13921

jerryjzhang · 2020-11-04T07:03:08Z

What is the purpose of the change

Fetching first N rows is a special case of TopN, which currently is supported by the Rank operator. However, the full-fledged TopN implementation is heavyweight in that it involves sort key comparison and raw row buffering. This leads to large state and memory buffer that is not needed for the first-n-row case. This PR introduces a lightweight variant of TopN function to handle it more efficiently.

Brief change log

Add AppendOnlyFirstNFunction as a variant of AppendOnlyTopNFunction
Modify StreamExecRank to instantiate AppendOnlyFirstNFunciton when the first-n-row case is determined

Verifying this change

This change added tests and can be verified as follows:

Added unit tests inAppendOnlyFirstNFunctionTest

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

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

Documentation

Does this pull request introduce a new feature? (no)

flinkbot · 2020-11-04T07:06:08Z

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 5b90fd3 (Sat Aug 28 12:14:32 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-11-04T08:03:36Z

CI report:

5b90fd3 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

jerryjzhang · 2020-11-10T01:45:41Z

@wuchong @godfreyhe would you mind take a look?

JingsongLi · 2021-05-24T10:28:01Z

Hi @tragicjun, can you rebase latest master?

jerryjzhang · 2021-05-27T07:43:26Z

Hi @tragicjun, can you rebase latest master?

Hi @JingsongLi rebase latest master done, please take a look.

…perator

JingsongLi

Thanks for the rebasing. @tragicjun Can you create a ITCase or point out which ITCase you want to solve?

JingsongLi · 2021-05-31T02:38:28Z

...nk/src/main/java/org/apache/flink/table/runtime/operators/rank/AppendOnlyFirstNFunction.java

+ */
+public class AppendOnlyFirstNFunction extends AbstractTopNFunction {
+
+    private static final long serialVersionUID = -889227691088906246L;


serialVersionUID should be 1L at first, From the Flink code style.

serialVersionUID is at the 2nd in all other classes, thus in consistent with them.

JingsongLi · 2021-05-31T02:44:32Z

...link/src/main/java/org/apache/flink/table/planner/plan/nodes/exec/stream/StreamExecRank.java

-                            cacheSize);
+            if (sortFields.length == 1
+                    && inputType.getChildren().get(sortFields[0]).getTypeRoot()
+                            == LogicalTypeRoot.TIMESTAMP_WITHOUT_TIME_ZONE


Just TIMESTAMP_WITHOUT_TIME_ZONE? I don't see any proc-time.

sorry, forgot to push the latest commit.

…perator

jerryjzhang · 2021-05-31T12:33:25Z

Thanks for the rebasing. @tragicjun Can you create a ITCase or point out which ITCase you want to solve?

Thanks for reviewing. I have added an ITCase as RankJsonPlanITCase.testFirstN(). Please let me know if any issues.

JingsongLi · 2021-06-01T06:10:07Z

...link/src/main/java/org/apache/flink/table/planner/plan/nodes/exec/stream/StreamExecRank.java

+            boolean isAppendOnlyFirstN = false;
+            if (sortFields.length == 1) {
+                LogicalType sortKeyType = inputType.getChildren().get(sortFields[0]);
+                if (sortKeyType instanceof LocalZonedTimestampType


TypeCheckUtils.isProcTime

…perator

JingsongLi · 2021-06-02T08:10:57Z

@tragicjun still some compile failure.

…perator

jerryjzhang · 2021-06-02T09:45:31Z

@tragicjun still some compile failure.

sorry, some format violation. Project build passed on my local, please review.

JingsongLi

Thanks @tragicjun for the update.
Looks good to me, merging...

rmetzger added review=description? component=TableSQL/Planner component=TableSQL/Runtime labels Nov 4, 2020

JingsongLi self-requested a review May 24, 2021 10:28

jerryjzhang force-pushed the flink-19896 branch from 3a6d729 to 6b47947 Compare May 27, 2021 07:40

jerryjzhang force-pushed the flink-19896 branch from b246bde to d298115 Compare May 27, 2021 08:36

[FLINK-19896][table-blink] Improve first-n-row fetching in the Rank o…

d298115

…perator

JingsongLi requested changes May 31, 2021

View reviewed changes

jerryjzhang force-pushed the flink-19896 branch from cb02942 to 866cb86 Compare May 31, 2021 12:24

[FLINK-19896][table-blink] Improve first-n-row fetching in the Rank o…

866cb86

…perator

jerryjzhang requested a review from JingsongLi May 31, 2021 12:27

JingsongLi reviewed Jun 1, 2021

View reviewed changes

jerryjzhang force-pushed the flink-19896 branch from 3abf172 to ff41bab Compare June 2, 2021 03:56

jerryjzhang requested a review from JingsongLi June 2, 2021 03:58

[FLINK-19896][table-blink] Improve first-n-row fetching in the Rank o…

ff41bab

…perator

[FLINK-19896][table-blink] Improve first-n-row fetching in the Rank o…

5b90fd3

…perator

JingsongLi approved these changes Jun 3, 2021

View reviewed changes

JingsongLi merged commit 76a08c8 into apache:master Jun 3, 2021

flinkbot removed the component=TableSQL/Planner label Mar 17, 2022

[FLINK-19896][table-blink] Improve first-n-row fetching in the Rank o… #13921

[FLINK-19896][table-blink] Improve first-n-row fetching in the Rank o… #13921

Uh oh!

Conversation

jerryjzhang commented Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

flinkbot commented Nov 4, 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 Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

Uh oh!

jerryjzhang commented Nov 10, 2020

Uh oh!

JingsongLi commented May 24, 2021

Uh oh!

jerryjzhang commented May 27, 2021

Uh oh!

JingsongLi left a comment

Choose a reason for hiding this comment

Uh oh!

JingsongLi May 31, 2021

Choose a reason for hiding this comment

Uh oh!

jerryjzhang May 31, 2021

Choose a reason for hiding this comment

Uh oh!

JingsongLi May 31, 2021

Choose a reason for hiding this comment

Uh oh!

jerryjzhang May 31, 2021

Choose a reason for hiding this comment

Uh oh!

jerryjzhang commented May 31, 2021

Uh oh!

JingsongLi Jun 1, 2021

Choose a reason for hiding this comment

Uh oh!

jerryjzhang Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

JingsongLi commented Jun 2, 2021

Uh oh!

jerryjzhang commented Jun 2, 2021

Uh oh!

JingsongLi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jerryjzhang commented Nov 4, 2020 •

edited

Loading

flinkbot commented Nov 4, 2020 •

edited

Loading

flinkbot commented Nov 4, 2020 •

edited

Loading