Skip to content

fix(proto): preserve AnalyzeExec metric types across serialization - #24669

Open
haohuaijin wants to merge 5 commits into
apache:mainfrom
haohuaijin:fix-analyze-metric-types-proto
Open

fix(proto): preserve AnalyzeExec metric types across serialization#24669
haohuaijin wants to merge 5 commits into
apache:mainfrom
haohuaijin:fix-analyze-metric-types-proto

Conversation

@haohuaijin

@haohuaijin haohuaijin commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

AnalyzeExec::metric_types was lost during protobuf round-trips, causing non-default selections such as summary-only metrics to reset to [Summary, Dev].

What changes are included in this PR?

  • Serialize and deserialize AnalyzeExec::metric_types.
  • Preserve compatibility with older protobuf messages.
  • Distinguish an absent field from an explicitly empty metric type list.
  • Add regression tests for summary, dev, empty, and legacy states.

Are these changes tested?

Yes.

  • Relevant datafusion-proto integration tests
  • Clippy for the affected crates
  • Formatting and diff checks

Are there any user-facing changes?

Yes. EXPLAIN ANALYZE metric type selections now survive physical-plan protobuf serialization. There are no public Rust API changes.

@github-actions github-actions Bot added proto Related to proto crate physical-plan Changes to the physical-plan crate labels Aug 25, 2026
@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown

Thank you for opening this pull request!

Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).

Details
     Cloning apache/main
    Building datafusion-physical-plan v55.0.0 (current)
       Built [  42.398s] (current)
     Parsing datafusion-physical-plan v55.0.0 (current)
      Parsed [   0.162s] (current)
    Building datafusion-physical-plan v55.0.0 (baseline)
       Built [  41.344s] (baseline)
     Parsing datafusion-physical-plan v55.0.0 (baseline)
      Parsed [   0.161s] (baseline)
    Checking datafusion-physical-plan v55.0.0 -> v55.0.0 (no change; assume patch)
     Checked [   1.025s] 223 checks: 223 pass, 31 skip
     Summary no semver update required
    Finished [  87.014s] datafusion-physical-plan
    Building datafusion-proto v55.0.0 (current)
       Built [  60.273s] (current)
     Parsing datafusion-proto v55.0.0 (current)
      Parsed [   0.020s] (current)
    Building datafusion-proto v55.0.0 (baseline)
       Built [  61.002s] (baseline)
     Parsing datafusion-proto v55.0.0 (baseline)
      Parsed [   0.021s] (baseline)
    Checking datafusion-proto v55.0.0 -> v55.0.0 (no change; assume patch)
     Checked [   0.160s] 223 checks: 223 pass, 31 skip
     Summary no semver update required
    Finished [ 122.895s] datafusion-proto
    Building datafusion-proto-models v55.0.0 (current)
       Built [  27.443s] (current)
     Parsing datafusion-proto-models v55.0.0 (current)
      Parsed [   0.140s] (current)
    Building datafusion-proto-models v55.0.0 (baseline)
       Built [  27.084s] (baseline)
     Parsing datafusion-proto-models v55.0.0 (baseline)
      Parsed [   0.142s] (baseline)
    Checking datafusion-proto-models v55.0.0 -> v55.0.0 (no change; assume patch)
     Checked [   2.568s] 223 checks: 222 pass, 1 fail, 0 warn, 31 skip

--- failure constructible_struct_adds_field: struct exhaustively constructible through public API adds field ---

Description:
A pub struct that could be exhaustively constructed with a literal using only public API has a new pub field, breaking existing exhaustive literals.
        ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/constructible_struct_adds_field.ron

Failed in:
  field AnalyzeExecNode.has_metric_types in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:2142
  field AnalyzeExecNode.metric_types in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:2145
  field AnalyzeExecNode.has_metric_types in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:2142
  field AnalyzeExecNode.metric_types in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:2145

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  58.897s] datafusion-proto-models

@github-actions github-actions Bot added the auto detected api change Auto detected API change label Aug 25, 2026
@codecov-commenter

codecov-commenter commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.52%. Comparing base (c56a7ab) to head (c66a9e3).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24669      +/-   ##
==========================================
+ Coverage   81.47%   81.52%   +0.04%     
==========================================
  Files        1122     1122              
  Lines      404140   404199      +59     
  Branches   404140   404199      +59     
==========================================
+ Hits       329284   329521     +237     
+ Misses      55546    55305     -241     
- Partials    19310    19373      +63     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kosiew

kosiew commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

@haohuaijin
Can you resolve the CI error?

@haohuaijin

haohuaijin commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

@haohuaijin Can you resolve the CI error?

it should not related to my change, let me rerun the ci

@haohuaijin

Copy link
Copy Markdown
Contributor Author

@haohuaijin Can you resolve the CI error?

Hi @kosiew, ci is green now.

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

Labels

auto detected api change Auto detected API change physical-plan Changes to the physical-plan crate proto Related to proto crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants