Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GLUTEN-3559][VL] Fixed unit test failing after change of Spark-38674 #4143

Merged
merged 16 commits into from
Dec 29, 2023

Conversation

vibhaska
Copy link
Contributor

What changes were proposed in this pull request?

This PR fixes the following suites:
GlutenDynamicPartitionPruningV1SuiteAEOff
GlutenDynamicPartitionPruningV1SuiteAEOff
GlutenDynamicPartitionPruningV1SuiteAEOn
GlutenDynamicPartitionPruningV1SuiteAEOnDisableScan
GlutenDynamicPartitionPruningV1SuiteAEOffDisableScan
GlutenDynamicPartitionPruningV2SuiteAEOff
GlutenDynamicPartitionPruningV2SuiteAEOn
GlutenDynamicPartitionPruningV2SuiteAEOnDisableScan
GlutenDynamicPartitionPruningV2SuiteAEOffDisableScan

(Fixes: #3559)

How was this patch tested?

Ran these UTs to confirm the fix

Copy link

#3559

Copy link

Run Gluten Clickhouse CI

1 similar comment
Copy link

Run Gluten Clickhouse CI

Copy link

Run Gluten Clickhouse CI

Copy link

Run Gluten Clickhouse CI

Copy link

Run Gluten Clickhouse CI

@vibhaska
Copy link
Contributor Author

@JkSelf Kindly review.
Some failing checks seems unrelated.

Copy link

Run Gluten Clickhouse CI

Copy link

Run Gluten Clickhouse CI

Copy link

Run Gluten Clickhouse CI

Copy link

Run Gluten Clickhouse CI

Copy link

Run Gluten Clickhouse CI

Copy link

Run Gluten Clickhouse CI

Copy link

Run Gluten Clickhouse CI

@ayushi-agarwal
Copy link
Contributor

@zhli1142015 @ulysses-you Could you please help in reviewing this?

@@ -550,17 +606,26 @@ class GlutenDynamicPartitionPruningV1SuiteAEOff
scanOption.get
}

def getDriverMetrics(plan: SparkPlan, key: String): Option[SQLMetric] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks this function is duplicated several times. If any reason we need it as we always pass scan for plan.

Copy link
Contributor Author

@vibhaska vibhaska Dec 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhli1142015 the corresponding metrics need to be sourced from driverMetrics (as against metrics, used earlier)

Copy link
Contributor

@JkSelf JkSelf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vibhaska Thanks for your great work. Just two small questions. Thanks.

Copy link

Run Gluten Clickhouse CI

Copy link

Run Gluten Clickhouse CI

Copy link

Run Gluten Clickhouse CI

Copy link

Run Gluten Clickhouse CI

Copy link
Contributor

@JkSelf JkSelf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Can you help to update the issue list here?

@JkSelf JkSelf merged commit 15bf8bc into apache:main Dec 29, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[VL] Track all the failed unit test in Spark 3.4.
4 participants