Copilot commented on code in PR #1492:
URL:
https://github.com/apache/datafusion-python/pull/1492#discussion_r3072549071
##########
crates/core/src/dataframe.rs:
##########
@@ -707,7 +707,15 @@ impl PyDataFrame {
/// Print the result, 20 lines by default
#[pyo3(signature = (num=20))]
fn show(&self, py: Python, num: usize) -> PyDataFusionResult<()> {
- let df = self.df.as_ref().clone().limit(0, Some(num))?;
+ let mut df = self.df.as_ref().clone();
+ df = match self.df.logical_plan() {
+ LogicalPlan::Explain(_) | LogicalPlan::Analyze(_) => {
+ // Explain and Analyzer require they are at the top
+ // of the plan, so do not add a limit.
+ df
+ }
+ _ => df.limit(0, Some(num))?,
Review Comment:
`DataFrame::logical_plan()` in this codebase appears to return a reference
(e.g., `self.df.logical_plan().clone()` in `crates/core/src/table.rs`), so
`match self.df.logical_plan()` here is matching on `&LogicalPlan`. As written,
the patterns `LogicalPlan::Explain(_) | LogicalPlan::Analyze(_)` will not match
a reference and is likely to fail to compile. Adjust the match to destructure
the reference (or explicitly dereference/clone the plan before matching).
##########
crates/core/src/dataframe.rs:
##########
@@ -707,7 +707,15 @@ impl PyDataFrame {
/// Print the result, 20 lines by default
#[pyo3(signature = (num=20))]
fn show(&self, py: Python, num: usize) -> PyDataFusionResult<()> {
- let df = self.df.as_ref().clone().limit(0, Some(num))?;
+ let mut df = self.df.as_ref().clone();
+ df = match self.df.logical_plan() {
+ LogicalPlan::Explain(_) | LogicalPlan::Analyze(_) => {
+ // Explain and Analyzer require they are at the top
+ // of the plan, so do not add a limit.
+ df
+ }
+ _ => df.limit(0, Some(num))?,
+ };
print_dataframe(py, df)
}
Review Comment:
For `EXPLAIN` / `ANALYZE` plans, `show(num=...)` no longer applies any row
limiting, so the `num` parameter is effectively ignored for these cases. If you
still want to respect `num` without changing the logical plan root, consider
limiting at the printing/collection layer (e.g., truncating collected batches /
taking first N rows from the stream) rather than adding a `LIMIT` node.
--
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]
