zhuqi-lucas commented on code in PR #22751:
URL: https://github.com/apache/datafusion/pull/22751#discussion_r3355609398
##########
datafusion/physical-plan/Cargo.toml:
##########
@@ -76,6 +81,9 @@ datafusion-physical-expr = { workspace = true,
default-features = true }
datafusion-physical-expr-common = { workspace = true }
datafusion-proto-common = { workspace = true, optional = true }
datafusion-proto-models = { workspace = true, optional = true }
+# Optional heap profiler used by the `dhat-heap` feature for memory
Review Comment:
Good point, I agree. The dhat dep is shaky to take on upstream when the
tests it backs are not in CI.
Just force-pushed (`1c0f4d070`):
- removed `dhat-heap` feature from `physical-plan/Cargo.toml`
- removed the `dhat` dependency
- removed the `cfg(feature = "dhat-heap")` global_allocator wiring in
`lib.rs`
- removed the `dhat_tests` module in `multi_group_by/mod.rs`
The PR is now a single-file diff in `multi_group_by/mod.rs` covering:
factory extraction, Time32 / Time64 supported_type alignment, the
`supported_type` ↔ `make_group_column` consistency fuzz, and the `try_new`-time
eager construction that @2010YOUY01 asked for.
For the subsequent PRs in the #22706 split (FixedSizeList, Struct, List,
LargeList, composition tests), I'll measure memory savings via
`GroupColumn::size()` head-to-head against `GroupValuesRows` (the pattern that
the existing `column_path_uses_less_memory_than_rows_for_*` tests in #22706
already use). Those run in normal `cargo test` so they're exercised by CI. The
dhat-based profiling stays on a personal-fork side branch for manual
investigation only.
--
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]
