comphead commented on code in PR #7140:
URL: https://github.com/apache/datafusion-comet/pull/7140#discussion_r4354706860
##########
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:
Good catch, it would. The projection alone reports the rows and a nonzero
time. I added a native test, `window_cast_projection_reports_window_time` in
`planner.rs`. It plans a `row_number` window (which gets the cast projection),
runs it, and checks that `elapsed_compute` from `to_native_metric_node` equals
the window time plus the projection time and that `output_rows` is not double
counted. Reverting the `window.rs` change fails it. Fixed in 238a5ed519.
##########
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:
Done in 238a5ed519. The window example in `debugging.md` now describes the
casting projection and `SparkPlan::new_with_additional`, and no longer mentions
`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]
