Skip to content

chore: surface DataFusion 54 PruningMetrics and Ratio in CometNativeScan metrics#4733

Open
mbutrovich wants to merge 5 commits into
apache:mainfrom
mbutrovich:scan_metrics
Open

chore: surface DataFusion 54 PruningMetrics and Ratio in CometNativeScan metrics#4733
mbutrovich wants to merge 5 commits into
apache:mainfrom
mbutrovich:scan_metrics

Conversation

@mbutrovich

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #.

Rationale for this change

DataFusion 54.0 introduced two composite MetricValue variants (PruningMetrics with pruned/matched/fully_matched counters, and Ratio with part/total). Both intentionally return 0 from as_usize() because the variants bundle multiple counters and aggregation is reserved for MetricsSet. Comet's metric bridge in to_native_metric_node calls value.as_usize() blindly, so every metric of these types silently surfaces as 0 on the Spark side. Row-group pruning, page-index pruning, bloom-filter pruning, file-range pruning, limit pruning, and scan-efficiency stats all read as zero in the Spark UI even when the underlying counters are populated correctly inside DataFusion.

The native Parquet scan also rendered "number of output rows" twice in the UI because CometNativeScanExec.metrics exposed the underlying SQLMetric under both output_rows and numOutputRows, and the description string of the metric appeared once per key. The numOutputRows alias has no consumers outside the class.

What changes are included in this PR?

  • to_native_metric_node now expands PruningMetrics into <name> (pruned count) and s/pruned/matched/<name> (matched count) entries, and Ratio into <name>_part and <name>_total entries. Other MetricValue variants still flow through as_usize() unchanged.
  • CometMetricNode.nativeScanMetrics declares the previously-missing Scala-side metrics (files_ranges_*, limit_*, page_index_pages_*, predicate_cache_*, num_predicate_creation_errors, scan_efficiency_ratio_total). The Scala layer remains the source of truth for what's surfaced; unknown native names are silently dropped.
  • CometNativeScanExec drops the numOutputRows alias, and the still-deprecated filterKeys call on the driver-metric subset is replaced with filter.
  • Fix a typo ("for for") and align the time_elapsed_scanning_total description with the DataFusion docstring.

How are these changes tested?

CometNativeReaderSuite's row-group pruning regression test reads the now-bridged row_groups_pruned_statistics and row_groups_matched_statistics directly, asserts their sum equals the row-group count reported by ParquetFileReader, and asserts the pruned count is greater than zero. Before this PR both metrics read 0; after, they report 3 pruned and 5 matched across the 8 row groups in the test file.

"scan_efficiency_ratio_total" ->
SQLMetrics.createSizeMetric(
sc,
"Total file size, denominator of scan_efficiency_ratio = bytes_scanned / total_file_size"))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe we can rephrase it? unlike to other metrics this one sounds not very clear

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is an awkward one anyway. It's a made up metric from DataFusion (it's a ratio) so the best I could come up with was explaining how to compute the math again.

@comphead comphead left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @mbutrovich

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.

2 participants