BlakeOrth commented on code in PR #18004:
URL: https://github.com/apache/datafusion/pull/18004#discussion_r2421441866
##########
datafusion-cli/src/print_options.rs:
##########
@@ -174,10 +177,78 @@ impl PrintOptions {
query_start_time,
);
+ self.write_output(&mut writer, formatted_exec_details)
+ }
+
+ fn write_output<W: io::Write>(
+ &self,
+ writer: &mut W,
+ formatted_exec_details: String,
+ ) -> Result<()> {
if !self.quiet {
writeln!(writer, "{formatted_exec_details}")?;
+
+ if self.instrumented_registry.mode() !=
InstrumentedObjectStoreMode::Disabled
+ {
+ writeln!(writer, "{OBJECT_STORE_PROFILING_HEADER}")?;
+ for store in self.instrumented_registry.stores() {
+ writeln!(writer, "{store}")?;
+ }
+ }
Review Comment:
Yes, the `PrintOptions` change is unfortunate here. If you have a
recommendation on a better way to handle this I'd be happy to explore a
different solution. The fact that we need to support the commands which all
leverage print options is what pushed me towards this solution versus something
like changing the method signature(s) to pass the registry through. Perhaps
`PrintOptions` has just outgrown its original purpose and could use a
refactor/rethink at some point down the road.
> I wonder if (as a follow on PR) we should also move the timing into this
structure too
I'm not sure I'm understanding you here. Do you mean keeping the records
from the store(s) that will be printed in `PrintOptions`?
--
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]
