andygrove commented on code in PR #2754:
URL:
https://github.com/apache/datafusion-ballista/pull/2754#discussion_r4459273333
##########
python/python/ballista/__init__.py:
##########
@@ -24,6 +24,12 @@
BallistaExecutor,
setup_test_cluster,
)
+from .client import (
Review Comment:
Heads up that this adds `grpcio`, `protobuf>=7.35.1,<8` and a runtime
`pyarrow` for everyone, and this eager import means people only using
`BallistaSessionContext` pick all of it up too (`import ballista` now needs
grpc, `pyarrow.flight` and the generated stubs). The protobuf floor in
particular could clash with other packages in a user's environment.
Would you consider importing the client lazily or putting it behind an extra
like `ballista[python-client]`? If it stays as is, an entry in
`docs/source/upgrading/` about the new dependencies would be great.
##########
ballista/core/src/execution_plans/distributed_explain_analyze.rs:
##########
@@ -306,7 +306,8 @@ pub(crate) fn format_job_plan(
.map(|stages| stages.join("\n\n"))
}
-fn format_metrics_as_record_batch(
+/// Renders `job_metrics` as the single row `EXPLAIN ANALYZE` returns.
+pub fn format_metrics_as_record_batch(
Review Comment:
Since this is becoming public, could we drop the unused `_job_id` and
`_verbose` parameters first? Removing them after release would be a signature
break for library users, and the only callers are in this repo.
--
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]
