rich7420 commented on code in PR #7140:
URL: https://github.com/apache/datafusion-comet/pull/7140#discussion_r4346498817
##########
spark/src/test/scala/org/apache/comet/exec/CometWindowExecSuite.scala:
##########
@@ -521,6 +521,32 @@ class CometWindowExecSuite extends CometTestBase {
}
}
+ test("Native window operator reports its output rows and time") {
+ withTempDir { dir =>
+ (0 until 1000)
+ .map(i => (i % 10, i))
+ .toDF("k", "v")
+ .write
+ .mode("overwrite")
+ .parquet(dir.toString)
+ withTempView("window_metrics") {
+
spark.read.parquet(dir.toString).createOrReplaceTempView("window_metrics")
+ // `row_number` returns a type Spark does not expect, so the planner
casts it in a
+ // projection above the native window, while `max` needs no cast.
+ val (_, plan) = checkSparkAnswerAndOperator(sql("""
+ |SELECT max(v) OVER (PARTITION BY k ORDER BY v),
+ | row_number() OVER (PARTITION BY v % 3 ORDER BY v)
+ |FROM window_metrics""".stripMargin))
+ val windows = collect(plan) { case w: CometWindowExec => w }
+ assert(windows.length == 2)
+ windows.foreach { w =>
+ assert(w.metrics("output_rows").value == 1000)
+ assert(w.metrics("elapsed_compute").value > 0)
Review Comment:
Would this test still pass if the `window.rs` change were reverted? The cast
projection alone reports 1000 rows and nonzero time. Could a native test verify
that the window's time is added?
##########
native/core/src/execution/planner/window.rs:
##########
@@ -180,17 +180,24 @@ impl OperatorBuilder for WindowBuilder {
proj_exprs.push((expr, field.name().to_string()));
}
if needs_cast {
- Arc::new(ProjectionExec::try_new(proj_exprs, window_agg)?)
+ let projection: Arc<dyn ExecutionPlan> =
Arc::new(ProjectionExec::try_new(
+ proj_exprs,
+ Arc::clone(&window_agg),
+ )?);
+ // The window is an additional plan so that its time is
reported with the
+ // projection's rather than left out.
+ SparkPlan::new_with_additional(
Review Comment:
nit: could we update the window section of `debugging.md` too? It still
mentions `final_plan` and `SparkPlan::new(..., final_plan, ...)`.
--
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]
