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

Spark 3.5: Fix metrics reporting in distributed planning #8602

Merged

Conversation

aokolnychyi
Copy link
Contributor

This PR fixes metrics reporting in distributed planning:

  • SparkDistributedDataScan picks up the base reporter configured for the table.
  • SparkDistributedDataScan correctly computes the number of skipped data and delete files.

@@ -53,6 +53,10 @@ public BaseTable(TableOperations ops, String name, MetricsReporter reporter) {
this.reporter = reporter;
}

MetricsReporter reporter() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The simplest approach is to just expose the reporter with package-private access.

We could have an interface called HasMetricsReporter, similar to HasTableOperations. It will be a bit more generic but it is not required for this PR. Let me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, good to know!

@aokolnychyi
Copy link
Contributor Author

@nastra @rdblue, could you review this one?

@aokolnychyi aokolnychyi added this to the Iceberg 1.4.0 milestone Sep 21, 2023
Copy link
Contributor

@nastra nastra left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @aokolnychyi

@aokolnychyi aokolnychyi merged commit 1354fb2 into apache:master Sep 21, 2023
47 checks passed
@aokolnychyi
Copy link
Contributor Author

Thanks, @nastra!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants