adriangb opened a new pull request, #25884:
URL: https://github.com/apache/datafusion/pull/25884
## Which issue does this PR close?
- Closes #25883.
## Rationale for this change
`datafusion-physical-plan` enables the `serde_json/preserve_order` feature.
Cargo feature unification turns this feature on for the whole application that
embeds DataFusion. Then every `serde_json::Map` in that application is backed
by `IndexMap` instead of `BTreeMap`. This changes the key order of JSON that
the application writes (REST responses, signed JSON, snapshot tests), and the
application did not ask for this change.
DataFusion only needs a fixed key order for the `EXPLAIN FORMAT pgjson`
output (`"Node Type"` first, `"Plans"` last, as in PostgreSQL). A library must
not change a global serde behavior for this.
There is also a small latent bug: `datafusion-expr` enabled `preserve_order`
only as a dev-dependency. So in real builds, the key order of the logical-plan
pgjson output depended on whether some other crate in the dependency graph
enabled the feature.
## What changes are included in this PR?
- Physical plan pgjson (`datafusion/physical-plan/src/display.rs`): the
nodes are now `#[derive(Serialize)]` structs. The field order is the output
order. `Extras` is an ordered list of pairs that is serialized as a map.
- Logical plan pgjson (`datafusion/expr/src/logical_plan/display.rs`): there
are many node shapes, so a small private `pg_fields!` macro builds an ordered
field list. It replaces the `json!({...})` calls almost one-for-one. A short
manual `Serialize` impl writes the fields in order.
- Remove `features = ["preserve_order"]` from `datafusion-physical-plan` and
from the `datafusion-expr` dev-dependencies. `datafusion-cli` and
`datafusion-sqllogictest` keep the feature, because they are binaries or test
crates.
- Add `serde` (with `derive`) as a workspace dependency. It was already in
the dependency tree through `serde_json`.
`cargo tree -e features -i serde_json -p datafusion` now shows no
`preserve_order`.
## What is the testing strategy for this PR?
The existing tests cover this change, and the output is byte-identical:
- `pgjson_snapshot_of_sample_plan` in `datafusion-physical-plan` and
`test_display_pg_json` in `datafusion-expr`. These tests now build without
`preserve_order`, so they really check the key order. With sorted keys,
`"Details"` would come before `"Node Type"` and the snapshot would fail.
- `explain.slt` and `explain_analyze.slt` pass.
## Are there any user-facing changes?
- Applications that depend on DataFusion no longer get
`serde_json/preserve_order` from DataFusion.
- The `EXPLAIN FORMAT pgjson` output does not change, with one small
exception. With `ShowMetrics::Full`, `"Actual Rows"` now always comes before
`"Actual Total Time"`. Before, the order followed the order in which the
operator registered its metrics.
- Note: the third-party `substrait` crate also enables
`serde_json/preserve_order`. Users of `datafusion-substrait` still get this
feature from that crate.
No public API changes.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
