[SPARK-27739][SQL] df.persist should save stats from optimized plan

[jzhuge]

What changes were proposed in this pull request?

CacheManager.cacheQuery saves the stats from the optimized plan to cache.

How was this patch tested?

Existing testss.

[dongjoon-hyun]

Hi, @jzhuge . Could you add a test case for this, too?

[jzhuge]

Thanks @dongjoon-hyun for the review. Unfortunately I couldn't find a good way to unit test the accuracy of InMemoryRelation.statsOfPlanToCache and there is no existing unit test for this field. Any suggestion is welcome.

[rdblue]

I'm not sure that we can test this without filter pushdown in stats calculation.

We've added filter pushdown, so the stats methods use PhysicalOperation to detect scans paired with projections and filters. When getting the stats for a scan, those filters are passed so that we can get accurate stats. Without that optimization, the stats would be identical between the analyzed plan and the optimized plan.

[dongjoon-hyun]

ok to test

[SparkQA]

Test build #105439 has finished for PR 24623 at commit bb94d88.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[viirya]

retest this please.

[SparkQA]

Test build #105438 has finished for PR 24623 at commit bb94d88.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #105440 has finished for PR 24623 at commit bb94d88.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[viirya]

When re-caches some entries in the cache in recacheByCondition, it still uses analyzed plan's stats.

spark/sql/core/src/main/scala/org/apache/spark/sql/execution/CacheManager.scala

Lines 188 to 190 in 3e30a98

	val newCache = InMemoryRelation(
	cacheBuilder = cd.cachedRepresentation.cacheBuilder.copy(cachedPlan = plan),
	logicalPlan = cd.plan)

[maropu]

How about renaming logicalPlan->optimizedPlan in InMemoryRelation?;

spark/sql/core/src/main/scala/org/apache/spark/sql/execution/columnar/InMemoryRelation.scala

Line 149 in 70ef906

logicalPlan: LogicalPlan): InMemoryRelation = {

[SparkQA]

Test build #108493 has finished for PR 24623 at commit e0e678b.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[jzhuge]

Like the idea @maropu

[SparkQA]

Test build #108538 has finished for PR 24623 at commit 07f48fb.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[jzhuge]

@maropu @dongjoon-hyun @viirya Could you please take another look at this PR? I believe all comments have been addressed.

[gatorsmile]

We do have the leaf nodes that can provide stats. Not sure why we are unable to test the PR? Anybody can dig it deeper? This does not require any DSV2 change.

[gatorsmile]

cc @cloud-fan @gengliangwang

[gatorsmile]

If the stats of analyzed plan is the same as the optimized plan, please hold this PR until they become different.

[cloud-fan]

They can be different if we do DS v2 operator pushdown in the optimizer, but AFAIK it's not done yet.

[rdblue]

Stats already differ in the v1 path because partition pruning happens in the optimizer for data source tables (PrunedInMemoryFileIndex).

I see no reason not to get this in.

[cloud-fan]

ah that's a good point, I agree with it. @jzhuge can we write a test using file source partition pruning? I'm not comfortable merging a fix without tests. The change itself LGTM.

[SparkQA]

Test build #109069 has finished for PR 24623 at commit f6312c1.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[cloud-fan]

thanks, merging to master!

[jzhuge]

thanks all for the reviews. thanks @cloud-fan for the merge.