Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(frontend): make expr in explain output concise #802

Merged
merged 8 commits into from
Mar 10, 2022

Conversation

xiangjinwu
Copy link
Contributor

@xiangjinwu xiangjinwu commented Mar 9, 2022

What's changed and what's your intention?

Implement Debug manually for ExprImpl so that plan nodes can explain with a more concise format. It is intended to work with std::fmt::Formatter::debug_list. When verbosity is preferred, it is still available via the alternate flag {:#?}.

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

Comment on lines +98 to +110
let agg_name = format!("{:?}", agg_call.agg_kind());
let mut builder = f.debug_tuple(&agg_name);
agg_call.inputs().iter().for_each(|child| {
builder.field(child);
});
builder.finish()
Copy link
Contributor

Choose a reason for hiding this comment

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

impl fmt::Debug for AggCall?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intention is to keep the default verbose format available via {:#?}, so I am not overriding individual variants (Literal, InputRef, FunctionCall, AggCall).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid this intention may not be easy to find out by new comers. Why not impl a ToExplain for all ast and keep the behaviors of Debug consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Introducing a new trait does not work because the debug_struct / debug_list helpers on std::fmt::Formatter always uses Debug output. What about adding some doc comments on ExprImpl? If a developer is not satisfied with the overloaded concise format, they are expected to know Debug has been manually implemented.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, it's fine for now.

@xiangjinwu xiangjinwu changed the title feat(frontend): (WIP) make expr in explain output concise feat(frontend): make expr in explain output concise Mar 9, 2022
@xiangjinwu xiangjinwu marked this pull request as ready for review March 9, 2022 10:25
@codecov
Copy link

codecov bot commented Mar 9, 2022

Codecov Report

Merging #802 (bd75f68) into main (8ce69a3) will increase coverage by 0.00%.
The diff coverage is 76.47%.

Impacted file tree graph

@@            Coverage Diff            @@
##               main     #802   +/-   ##
=========================================
  Coverage     72.22%   72.23%           
  Complexity     2766     2766           
=========================================
  Files           923      923           
  Lines         53652    53683   +31     
  Branches       1787     1787           
=========================================
+ Hits          38752    38776   +24     
- Misses        14010    14017    +7     
  Partials        890      890           
Flag Coverage Δ
java 61.22% <ø> (ø)
rust 76.81% <76.47%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
rust/frontend/src/expr/input_ref.rs 77.77% <ø> (-4.05%) ⬇️
rust/frontend/src/expr/mod.rs 76.08% <75.00%> (-2.49%) ⬇️
rust/common/src/catalog/schema.rs 94.11% <100.00%> (ø)
rust/frontend/src/expr/literal.rs 66.66% <100.00%> (+6.66%) ⬆️
rust/meta/src/hummock/compaction.rs 81.65% <0.00%> (-0.60%) ⬇️
rust/common/src/types/ordered_float.rs 25.82% <0.00%> (+0.33%) ⬆️
rust/common/src/types/mod.rs 66.66% <0.00%> (+0.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ce69a3...bd75f68. Read the comment docs.

@xiangjinwu xiangjinwu enabled auto-merge (squash) March 10, 2022 09:42
@xiangjinwu xiangjinwu merged commit ce1a4ec into main Mar 10, 2022
@xiangjinwu xiangjinwu deleted the rust-frontend-explain-fmt branch March 10, 2022 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants