[SPARK-43611][SQL][PS][CONNCECT] Make `ExtractWindowExpressions` retain the `PLAN_ID_TAG` #42086

zhengruifeng · 2023-07-20T04:52:05Z

What changes were proposed in this pull request?

Make rule ExtractWindowExpressions retain the PLAN_ID_TAG

Why are the changes needed?

In #39925, we introduced a new mechanism to resolve expression with specified plan.

However, sometimes the plan ID might be discarded by some analyzer rules, and then some expressions can not be correctly resolved, this issue is the main blocker of PS on Connect.

Does this PR introduce any user-facing change?

yes, a lot of Pandas APIs enabled

How was this patch tested?

Enable UTs

zhengruifeng · 2023-07-20T04:58:34Z

I have checked with @cloud-fan that we might have to modify the rules one by one.
The good news is that 90% UTs can be resolved by this single one, and I think we only need to touch 3~4 more rules.

@HyukjinKwon @itholic

itholic · 2023-07-20T06:11:25Z

The good news is that 90% UTs can be resolved by this single one, and I think we only need to touch 3~4 more rules.

Great! Could you help creating tickets for remaining 3~4 more rules under SPARK-42497??

zhengruifeng · 2023-07-20T06:22:50Z

The good news is that 90% UTs can be resolved by this single one, and I think we only need to touch 3~4 more rules.

Great! Could you help creating tickets for remaining 3~4 more rules under SPARK-42497??

TBH, I am not very sure about which rules to modify for the remaining UTs, it needs further investigation

itholic · 2023-07-20T06:27:23Z

I got it. Just created single ticket here: SPARK-44492 for addressing undefined remaining tests so we don't miss it.

HyukjinKwon · 2023-07-20T06:46:23Z

I am fine with this as a workaround for now but such implementation depending on tags is sort of flaky. The tags are easily lost when you, e.g., copy the expressions IIRC.

zhengruifeng · 2023-07-20T13:05:28Z

I saw all tests in pyspark-pandas-slow-connect successed

not sure why it suddenly became

let me disable a few UT to see what happens

zhengruifeng · 2023-07-20T15:49:09Z

~~CI link: https://github.com/zhengruifeng/spark/actions/runs/5612996302/job/15208077409~~

cloud-fan · 2023-07-21T13:14:56Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala

Suggested change

other.getTagValue(tag)

.foreach(this.setTagValue(tag, _))

other.getTagValue(tag).foreach(this.setTagValue(tag, _))

cloud-fan · 2023-07-21T13:16:17Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

shall we copy all tags?

sounds good, let me have a try

fix

zhengruifeng · 2023-07-26T12:07:19Z

cc @cloud-fan all tests passed, would you mind taking another look?

zhengruifeng · 2023-07-27T03:01:10Z

thanks! merged to master

itholic · 2023-07-27T03:24:10Z

Thanks!

### What changes were proposed in this pull request? Enable more tests, they were excluded from #42086 due to the flaky CI issues ### Why are the changes needed? for test parity ### Does this PR introduce _any_ user-facing change? no, test-only ### How was this patch tested? enabled tests Closes #42182 from zhengruifeng/spark_43611_followup. Authored-by: Ruifeng Zheng <ruifengz@apache.org> Signed-off-by: Ruifeng Zheng <ruifengz@apache.org>

HyukjinKwon · 2023-07-31T10:40:00Z

Should we merge this to 3.5 too?

zhengruifeng · 2023-07-31T11:10:41Z

@HyukjinKwon not sure, maybe not needed?

…in the `PLAN_ID_TAG` ### What changes were proposed in this pull request? Make rule `ExtractWindowExpressions` retain the `PLAN_ID_TAG ` ### Why are the changes needed? In apache#39925, we introduced a new mechanism to resolve expression with specified plan. However, sometimes the plan ID might be discarded by some analyzer rules, and then some expressions can not be correctly resolved, this issue is the main blocker of PS on Connect. ### Does this PR introduce _any_ user-facing change? yes, a lot of Pandas APIs enabled ### How was this patch tested? Enable UTs Closes apache#42086 from zhengruifeng/ps_connect_analyze_window. Authored-by: Ruifeng Zheng <ruifengz@apache.org> Signed-off-by: Ruifeng Zheng <ruifengz@apache.org>

…wExpressions` & `WidenSetOperationTypes` retain the `PLAN_ID_TAG` ### What changes were proposed in this pull request? Backport for #42086 and #42230 ### Why are the changes needed? for functionality parity ### Does this PR introduce _any_ user-facing change? Enabling couple of pandas APIs ### How was this patch tested? Enabling the existing UTs Closes #42252 from itholic/SPARK-43611-3.5. Lead-authored-by: Ruifeng Zheng <ruifengz@apache.org> Co-authored-by: itholic <haejoon.lee@databricks.com> Signed-off-by: Ruifeng Zheng <ruifengz@apache.org>

github-actions bot added SQL PYTHON PANDAS API ON SPARK labels Jul 20, 2023

zhengruifeng requested review from HyukjinKwon and cloud-fan July 20, 2023 05:03

zhengruifeng mentioned this pull request Jul 20, 2023

[WIP][SPARK-43611][SQL][PS][CONNCECT] Fix unexpected AnalysisException from Spark Connect client #42040

Closed

itholic mentioned this pull request Jul 20, 2023

[DO-NOT-MERGE][PS][TESTS] Enable pandas API on Spark tests related to SPARK-43611 #42041

Closed

HyukjinKwon approved these changes Jul 20, 2023

View reviewed changes

zhengruifeng force-pushed the ps_connect_analyze_window branch 3 times, most recently from 38bc650 to f3efddf Compare July 20, 2023 13:23

github-actions bot added the BUILD label Jul 20, 2023

zhengruifeng force-pushed the ps_connect_analyze_window branch from 666b2c1 to f3efddf Compare July 20, 2023 15:33

github-actions bot removed the BUILD label Jul 20, 2023

zhengruifeng force-pushed the ps_connect_analyze_window branch 2 times, most recently from 2ec3f81 to e22e1d9 Compare July 20, 2023 23:31

cloud-fan reviewed Jul 21, 2023

View reviewed changes

zhengruifeng force-pushed the ps_connect_analyze_window branch from e22e1d9 to ff6d479 Compare July 25, 2023 00:23

zhengruifeng changed the title ~~[SPARK-43611][SQL][PS][CONNCECT] Make ExtractWindowExpressions retain the PLAN_ID_TAG~~ [SPARK-43611][SQL][PS][CONNCECT] Make ExtractWindowExpressions retain the PLAN_ID_TAG Jul 25, 2023

zhengruifeng added 2 commits July 26, 2023 16:03

init

b444772

fix

address comments

fc488da

zhengruifeng force-pushed the ps_connect_analyze_window branch from ff6d479 to fc488da Compare July 26, 2023 08:03

cloud-fan approved these changes Jul 27, 2023

View reviewed changes

zhengruifeng closed this in 185a0a5 Jul 27, 2023

zhengruifeng deleted the ps_connect_analyze_window branch July 27, 2023 03:01

zhengruifeng mentioned this pull request Jul 27, 2023

[SPARK-43611][PS][CONNECT][TESTS][FOLLOWUPS] Enable more tests #42182

Closed

HyukjinKwon mentioned this pull request Jul 31, 2023

[SPARK-44602][SQL][CONNECT][PS] Make WidenSetOperationTypes retain the Plan_ID_TAG #42230

Closed

itholic mentioned this pull request Aug 1, 2023

[SPARK-43611][SPARK-44602][SQL][PS][CONNCECT][3.5] Make ExtractWindowExpressions & WidenSetOperationTypes retain the PLAN_ID_TAG #42252

Closed

	other.getTagValue(tag)
	.foreach(this.setTagValue(tag, _))
	other.getTagValue(tag).foreach(this.setTagValue(tag, _))

[SPARK-43611][SQL][PS][CONNCECT] Make ExtractWindowExpressions retain the PLAN_ID_TAG #42086

[SPARK-43611][SQL][PS][CONNCECT] Make ExtractWindowExpressions retain the PLAN_ID_TAG #42086

Uh oh!

Conversation

zhengruifeng commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

zhengruifeng commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itholic commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhengruifeng commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itholic commented Jul 20, 2023

Uh oh!

HyukjinKwon commented Jul 20, 2023

Uh oh!

zhengruifeng commented Jul 20, 2023

Uh oh!

zhengruifeng commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloud-fan Jul 21, 2023

Choose a reason for hiding this comment

Uh oh!

cloud-fan Jul 21, 2023

Choose a reason for hiding this comment

Uh oh!

zhengruifeng Jul 24, 2023

Choose a reason for hiding this comment

Uh oh!

zhengruifeng commented Jul 26, 2023

Uh oh!

zhengruifeng commented Jul 27, 2023

Uh oh!

itholic commented Jul 27, 2023

Uh oh!

HyukjinKwon commented Jul 31, 2023

Uh oh!

zhengruifeng commented Jul 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[SPARK-43611][SQL][PS][CONNCECT] Make `ExtractWindowExpressions` retain the `PLAN_ID_TAG` #42086

[SPARK-43611][SQL][PS][CONNCECT] Make `ExtractWindowExpressions` retain the `PLAN_ID_TAG` #42086

zhengruifeng commented Jul 20, 2023 •

edited

Loading

zhengruifeng commented Jul 20, 2023 •

edited

Loading

itholic commented Jul 20, 2023 •

edited

Loading

zhengruifeng commented Jul 20, 2023 •

edited

Loading

zhengruifeng commented Jul 20, 2023 •

edited

Loading