You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "haohuaijin (via GitHub)" <gi...@apache.org> on 2026/08/25 14:01:07 UTC
[PR] fix(proto): preserve AnalyzeExec metric types across serialization [datafusion]
haohuaijin opened a new pull request, #24669:
URL: https://github.com/apache/datafusion/pull/24669
## Which issue does this PR close?
- Related to #23494.
## 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): preserve AnalyzeExec metric types across serialization [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on PR #24669:
URL: https://github.com/apache/datafusion/pull/24669#issuecomment-5451981362
> @haohuaijin Can you resolve the CI error?
Hi @kosiew, ci is green now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): preserve AnalyzeExec metric types across serialization [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #24669:
URL: https://github.com/apache/datafusion/pull/24669#discussion_r3886127505
##########
datafusion/proto/tests/cases/plans/misc.rs:
##########
@@ -94,6 +94,180 @@ fn roundtrip_analyze() -> Result<()> {
Ok(())
}
+#[test]
+fn roundtrip_analyze_metric_types() -> Result<()> {
+ use protobuf::MetricType as ProtoMetricType;
+
+ let codec = DefaultPhysicalExtensionCodec {};
+ let ctx = SessionContext::new();
+
+ for (metric_types, expected) in [
+ (
+ Some(vec![MetricType::Summary]),
+ vec![ProtoMetricType::Summary as i32],
+ ),
+ (
+ Some(vec![MetricType::Dev]),
+ vec![ProtoMetricType::Dev as i32],
+ ),
+ (Some(vec![]), vec![]),
+ (
+ None,
+ vec![ProtoMetricType::Summary as i32, ProtoMetricType::Dev as i32],
+ ),
+ ] {
+ let legacy = metric_types.is_none();
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("plan_type", DataType::Utf8, false),
+ Field::new("plan", DataType::Utf8, false),
+ ]));
+ let input = Arc::new(PlaceholderRowExec::new(Arc::clone(&schema)));
+ let builder = AnalyzeExec::builder(false, false, input, schema);
+ let analyze = Arc::new(match metric_types {
+ Some(metric_types) => builder.with_metric_types(metric_types).build(),
+ None => builder.build(),
+ });
+ let mut node = PhysicalPlanNode::try_from_physical_plan(analyze, &codec)?;
+
+ let Some(protobuf::physical_plan_node::PhysicalPlanType::Analyze(analyze)) =
+ node.physical_plan_type.as_mut()
+ else {
+ unreachable!("expected AnalyzeExecNode")
+ };
+ if legacy {
+ analyze.has_metric_types = false;
+ analyze.metric_types.clear();
+ }
+
+ let node = PhysicalPlanNode::decode(node.encode_to_vec().as_slice())
+ .map_err(|e| DataFusionError::External(Box::new(e)))?;
+ #[cfg(feature = "json")]
+ let node: PhysicalPlanNode =
+ serde_json::from_str(&serde_json::to_string(&node).unwrap()).unwrap();
+ let roundtripped = node.try_into_physical_plan(&ctx.task_ctx(), &codec)?;
+ let mut node = PhysicalPlanNode::try_from_physical_plan(roundtripped, &codec)?;
+ {
+ let Some(protobuf::physical_plan_node::PhysicalPlanType::Analyze(analyze)) =
+ node.physical_plan_type.as_ref()
+ else {
+ unreachable!("expected AnalyzeExecNode")
+ };
+ assert!(analyze.has_metric_types);
+ assert_eq!(analyze.metric_types, expected);
+ }
+ if expected == [ProtoMetricType::Summary as i32] {
Review Comment:
Could we move the invalid-schema and unknown-enum assertions into a dedicated malformed `AnalyzeExecNode` decode test? Right now they only run when the successful round-trip case happens to select `Summary`, which makes the independent decode-error contract a little harder to see. This is just a test organization suggestion, and the existing coverage looks equivalent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): preserve AnalyzeExec metric types across serialization [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24669:
URL: https://github.com/apache/datafusion/pull/24669#issuecomment-5411584527
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/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>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion-physical-plan v55.0.0 (current)
Built [ 39.104s] (current)
Parsing datafusion-physical-plan v55.0.0 (current)
Parsed [ 0.160s] (current)
Building datafusion-physical-plan v55.0.0 (baseline)
Built [ 38.122s] (baseline)
Parsing datafusion-physical-plan v55.0.0 (baseline)
Parsed [ 0.160s] (baseline)
Checking datafusion-physical-plan v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 1.021s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 80.277s] datafusion-physical-plan
Building datafusion-proto v55.0.0 (current)
Built [ 54.444s] (current)
Parsing datafusion-proto v55.0.0 (current)
Parsed [ 0.020s] (current)
Building datafusion-proto v55.0.0 (baseline)
Built [ 54.968s] (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.153s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 111.272s] datafusion-proto
Building datafusion-proto-models v55.0.0 (current)
Built [ 25.006s] (current)
Parsing datafusion-proto-models v55.0.0 (current)
Parsed [ 0.139s] (current)
Building datafusion-proto-models v55.0.0 (baseline)
Built [ 25.279s] (baseline)
Parsing datafusion-proto-models v55.0.0 (baseline)
Parsed [ 0.140s] (baseline)
Checking datafusion-proto-models v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 2.523s] 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:2124
field AnalyzeExecNode.metric_types in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:2127
field AnalyzeExecNode.has_metric_types in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:2124
field AnalyzeExecNode.metric_types in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:2127
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 54.471s] datafusion-proto-models
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): preserve AnalyzeExec metric types across serialization [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24669:
URL: https://github.com/apache/datafusion/pull/24669#issuecomment-5411893493
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24669?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `46.96970%` with `35 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.43%. Comparing base ([`63f5b55`](https://app.codecov.io/gh/apache/datafusion/commit/63f5b55fb071707d2d78e18805df1ab365e1fd93?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`506326a`](https://app.codecov.io/gh/apache/datafusion/commit/506326a74edc2b8a357d0f5a9a1ea430df40747c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/24669?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/proto-models/src/generated/pbjson.rs](https://app.codecov.io/gh/apache/datafusion/pull/24669?src=pr&el=tree&filepath=datafusion%2Fproto-models%2Fsrc%2Fgenerated%2Fpbjson.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9wcm90by1tb2RlbHMvc3JjL2dlbmVyYXRlZC9wYmpzb24ucnM=) | 0.00% | [30 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24669?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/physical-plan/src/analyze.rs](https://app.codecov.io/gh/apache/datafusion/pull/24669?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Fanalyze.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9hbmFseXplLnJz) | 86.11% | [2 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24669?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #24669 +/- ##
==========================================
- Coverage 81.44% 81.43% -0.01%
==========================================
Files 1118 1118
Lines 399550 399609 +59
Branches 399550 399609 +59
==========================================
+ Hits 325398 325417 +19
- Misses 55154 55190 +36
- Partials 18998 19002 +4
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24669?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): preserve AnalyzeExec metric types across serialization [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #24669:
URL: https://github.com/apache/datafusion/pull/24669#issuecomment-5450926518
@haohuaijin
Can you resolve the CI error?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): preserve AnalyzeExec metric types across serialization [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on PR #24669:
URL: https://github.com/apache/datafusion/pull/24669#issuecomment-5462774479
Thanks @kosiew for reviews, i apply the suggestion in [aa8f71a](https://github.com/apache/datafusion/pull/24669/commits/aa8f71af152a9dbe81f279a1edb7e46829d7f040).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): preserve AnalyzeExec metric types across serialization [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on PR #24669:
URL: https://github.com/apache/datafusion/pull/24669#issuecomment-5451057216
> @haohuaijin Can you resolve the CI error?
it should not related to my change, it my rerun the ci to check
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): preserve AnalyzeExec metric types across serialization [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan merged PR #24669:
URL: https://github.com/apache/datafusion/pull/24669
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org