[FLINK-14552][table] Enable partition statistics in blink planner #10315

JingsongLi · 2019-11-25T15:36:32Z

What is the purpose of the change

Now after partition pruning, we will lost stats.
Actually, we have partition stats in catalog.
We need update statistics after partition pruning in PushPartitionIntoTableSourceScanRule.

Brief change log

Add merge method to TableStats.
Add table identifier to TableSourceTable
Update stats in PushPartitionIntoTableSourceScanRule.

Verifying this change

CatalogStatisticsTest.testGetPartitionStatsFromCatalog

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, Yarn/Mesos, ZooKeeper: no
The S3 file system connector: no

Documentation

Does this pull request introduce a new feature? (yes)
If yes, how is the feature documented? (JavaDocs

flinkbot · 2019-11-25T15:39:32Z

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 05b3061 (Mon Nov 25 15:39:32 UTC 2019)

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 · 2019-11-25T16:01:44Z

CI report:

05b3061 : FAILURE Build
707fc20 : FAILURE Build
748ccd8 : SUCCESS Build
5edccba : SUCCESS Build
72ed888 : SUCCESS Build
0249886 : FAILURE Build
84e826c : SUCCESS Build
54bd514 : CANCELED Build
71eed85 : CANCELED Build
71acb88 : FAILURE Build
ea45df4 : CANCELED Build

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build

godfreyhe

thanks for the pr @JingsongLi , i left some comments

flink-table/flink-table-common/src/main/java/org/apache/flink/table/plan/stats/ColumnStats.java

flink-table/flink-table-common/src/main/java/org/apache/flink/table/plan/stats/TableStats.java

...org/apache/flink/table/planner/plan/rules/logical/PushPartitionIntoTableSourceScanRule.scala

godfreyhe · 2019-12-02T08:48:24Z

...org/apache/flink/table/planner/plan/rules/logical/PushPartitionIntoTableSourceScanRule.scala

-      // Remove tableStats after predicates pushed down
-      FlinkStatistic.builder().statistic(statistic).tableStats(null).build()
+    val newStatistic = {
+      val tableStats = catalogOption match {


if no partitions are pruned and the original TableStats is not UNKNOWN, we could use the original TableStats and avoid fetch all partitions' statistics

First get stats from regular table stats may be not correct.

In the next step, I want to introduce listPartitionsByFilter, so we can not know the msg: if no partitions are pruned.

...anner-blink/src/main/scala/org/apache/flink/table/planner/plan/schema/TableSourceTable.scala

godfreyhe · 2019-12-02T08:57:07Z

...lanner-blink/src/test/java/org/apache/flink/table/planner/catalog/CatalogStatisticsTest.java

 	}

+	@Test
+	public void testGetPartitionStatsFromCatalog() throws Exception {


please add some tests about UNKNOWN stats

I can add others except unknown fields, because CatalogColumnStatisticsDataBase not support unknown now.

https://issues.apache.org/jira/browse/FLINK-14663, and @zjuwangg is fixing it

I think this blocker is test blocker, we can wait it until 1.10 release.

BTW, the PR is: #10394

JingsongLi · 2019-12-02T13:13:42Z

Thanks @godfreyhe for your review, updated.

flink-table/flink-table-common/src/main/java/org/apache/flink/table/plan/stats/ColumnStats.java

flink-table/flink-table-common/src/main/java/org/apache/flink/table/plan/stats/TableStats.java

godfreyhe · 2019-12-03T07:58:39Z

...lanner-blink/src/test/java/org/apache/flink/table/planner/catalog/CatalogStatisticsTest.java

 	}

+	@Test
+	public void testGetPartitionStatsFromCatalog() throws Exception {


https://issues.apache.org/jira/browse/FLINK-14663, and @zjuwangg is fixing it

godfreyhe

LGTM, +1 to merge

flink-table/flink-table-common/src/main/java/org/apache/flink/table/plan/stats/TableStats.java

KurtYoung · 2019-12-05T05:43:55Z

...anner-blink/src/main/scala/org/apache/flink/table/planner/plan/schema/TableSourceTable.scala

    val isStreamingMode: Boolean,
-    val catalogTable: CatalogTable)
+    val catalogTable: CatalogTable,
+    val tableIdentifier: Option[ObjectIdentifier])


I think names already played this role

Use names is quite hack:

if (names.size() == 3) { // it has ObjectIdentifier return ObjectIdentifier.of(names.get(0), names.get(1), names.get(2)); } else { // there is no ObjectIdentifier. }

So I think in future, we should use ObjectIdentifier always instead of names.

Then i don't think it will be an Option

Now, only one way should go to here without ObjectIdentifier.
It is temporary table with table api(TableSourceQueryOperation). But I can try to fix it.

KurtYoung · 2019-12-05T05:48:15Z

...org/apache/flink/table/planner/plan/rules/logical/PushPartitionIntoTableSourceScanRule.scala

+    val newStatistic = {
+      val tableStats = catalogOption match {
+        case Some(catalog) =>
+          def mergePartitionStats(): TableStats = {


why creating an inner function?

Scala don't have code to break the loop.

KurtYoung · 2019-12-05T05:51:02Z

...org/apache/flink/table/planner/plan/rules/logical/PushPartitionIntoTableSourceScanRule.scala

  override def onMatch(call: RelOptRuleCall): Unit = {
    val filter: Filter = call.rel(0)
    val scan: LogicalTableScan = call.rel(1)
+    val context = call.getPlanner.getContext.unwrap(classOf[FlinkContext])


we need some tests for this rule

We already have PushPartitionIntoTableSourceScanRuleTest.

but it doesn't cover your stats handle logics

I think CatalogStatisticsTest is more professional to test stats.

So who will cover the correctness about the stats after partition pruning?

CatalogStatisticsTest will trigger partition pruning.

flink-table/flink-table-common/src/main/java/org/apache/flink/table/plan/stats/ColumnStats.java

KurtYoung · 2019-12-06T01:57:10Z

Please rebase this

JingsongLi · 2019-12-06T04:29:09Z

passed in https://travis-ci.org/JingsongLi/flink/builds/621410239

rmetzger added the review=description? label Nov 25, 2019

rmetzger added the component=TableSQL/Planner label Nov 25, 2019

JingsongLi force-pushed the partstat branch from 748ccd8 to 5edccba Compare December 2, 2019 03:09

godfreyhe reviewed Dec 2, 2019

View reviewed changes

godfreyhe reviewed Dec 3, 2019

View reviewed changes

JingsongLi force-pushed the partstat branch from 84e826c to 54bd514 Compare December 5, 2019 01:36

godfreyhe approved these changes Dec 5, 2019

View reviewed changes

KurtYoung requested changes Dec 5, 2019

View reviewed changes

KurtYoung reviewed Dec 5, 2019

View reviewed changes

flink-table/flink-table-common/src/main/java/org/apache/flink/table/plan/stats/ColumnStats.java Show resolved Hide resolved

[FLINK-14552][table] Enable partition statistics in blink planner

ea45df4

JingsongLi force-pushed the partstat branch from 71acb88 to ea45df4 Compare December 6, 2019 02:06

KurtYoung merged commit 0d6b439 into apache:master Dec 6, 2019

JingsongLi deleted the partstat branch December 7, 2019 06:14

[FLINK-14552][table] Enable partition statistics in blink planner #10315

[FLINK-14552][table] Enable partition statistics in blink planner #10315

Uh oh!

Conversation

JingsongLi commented Nov 25, 2019

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 25, 2019

Automated Checks

Review Progress

Uh oh!

flinkbot commented Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

Uh oh!

godfreyhe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JingsongLi commented Dec 2, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

godfreyhe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

KurtYoung commented Dec 6, 2019

flinkbot commented Nov 25, 2019 •

edited

Loading