-
Notifications
You must be signed in to change notification settings - Fork 882
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
Use brief explain stats #7355
base: main
Are you sure you want to change the base?
Use brief explain stats #7355
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7355 +/- ##
==========================================
+ Coverage 80.06% 82.09% +2.03%
==========================================
Files 190 230 +40
Lines 37181 43145 +5964
Branches 9450 10853 +1403
==========================================
+ Hits 29770 35422 +5652
- Misses 2997 3397 +400
+ Partials 4414 4326 -88 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
bf29758
to
8f68c55
Compare
8f68c55
to
dbf3262
Compare
4d48cfb
to
8262b48
Compare
ExplainOpenGroup("Array Cache", "Arrow Array Cache", true, es); | ||
ExplainPropertyInteger("hits", NULL, decompress_cache_stats.hits, es); | ||
ExplainPropertyInteger("misses", NULL, decompress_cache_stats.misses, es); | ||
ExplainPropertyInteger("evictions", NULL, decompress_cache_stats.evictions, es); | ||
ExplainCloseGroup("Array Cache", "Arrow Array Cache", true, es); | ||
|
||
ExplainOpenGroup("Array Decompress", "Arrow Array Decompress", true, es); | ||
ExplainPropertyInteger("count", NULL, decompress_cache_stats.decompressions, es); | ||
if (es->verbose) | ||
ExplainPropertyInteger("calls", | ||
NULL, | ||
decompress_cache_stats.decompress_calls, | ||
es); | ||
ExplainCloseGroup("Array Decompress", "Arrow Array Decompress", true, es); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add tests for other EXPLAIN output format than TEXT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added one for JSON, which should get better coverage here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we also need to test using VERBOSE
on the explain to fully cover the code paths
75c2c10
to
cf8ce07
Compare
Use a more compact and easy to read version for the decompression and arrow array cache stats. Also simplify the code and the tests to remove unnecessary parts.
cf8ce07
to
675f2f4
Compare
if (has_decompress_data) | ||
appendStringInfoString(es->str, ", decompress"); | ||
append_if_positive(es->str, "count", decompress_cache_stats.decompressions); | ||
append_if_positive(es->str, "calls", decompress_cache_stats.decompress_calls); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this calls
be added only when es->verbose==true
like in non-text format?
Use a more compact and easy to read version for the decompression and arrow array cache stats and simplify the code.
Disable-check: force-changelog-file