kosiew commented on code in PR #23096:
URL: https://github.com/apache/datafusion/pull/23096#discussion_r3472370404
##########
benchmarks/queries/h2o/window.sql:
##########
@@ -117,3 +117,34 @@ SELECT id2, largest2_v2 FROM (
ROW_NUMBER() OVER (PARTITION BY id2 ORDER BY v2 DESC) AS order_v2
FROM large WHERE v2 IS NOT NULL
) sub_query WHERE order_v2 <= 2;
+
+-- Window Top-N partition cardinality sweep (id3 % N gives N distinct
partitions).
+-- These exercise PartitionedTopKExec across cardinalities to validate it stays
+-- competitive with the SortExec+Filter baseline as partition count grows.
+-- Window Top-N: 100 partitions
+SELECT pk, largest2_v2 FROM (
+ SELECT id3 % 100 AS pk, v2 AS largest2_v2,
+ ROW_NUMBER() OVER (PARTITION BY id3 % 100 ORDER BY v2 DESC) AS
order_v2
+ FROM large WHERE v2 IS NOT NULL
+) sub_query WHERE order_v2 <= 2;
+
+-- Window Top-N: 1,000 partitions
+SELECT pk, largest2_v2 FROM (
+ SELECT id3 % 1000 AS pk, v2 AS largest2_v2,
+ ROW_NUMBER() OVER (PARTITION BY id3 % 1000 ORDER BY v2 DESC) AS
order_v2
+ FROM large WHERE v2 IS NOT NULL
+) sub_query WHERE order_v2 <= 2;
+
+-- Window Top-N: 10,000 partitions
+SELECT pk, largest2_v2 FROM (
+ SELECT id3 % 10000 AS pk, v2 AS largest2_v2,
+ ROW_NUMBER() OVER (PARTITION BY id3 % 10000 ORDER BY v2 DESC) AS
order_v2
+ FROM large WHERE v2 IS NOT NULL
+) sub_query WHERE order_v2 <= 2;
+
+-- Window Top-N: 100,000 partitions
+SELECT pk, largest2_v2 FROM (
+ SELECT id3 % 100000 AS pk, v2 AS largest2_v2,
+ ROW_NUMBER() OVER (PARTITION BY id3 % 100000 ORDER BY v2 DESC) AS
order_v2
+ FROM large WHERE v2 IS NOT NULL
+) sub_query WHERE order_v2 <= 2;
Review Comment:
Small formatting nit: could you add a trailing newline at EOF? No behavioral
impact, just keeps the file consistent.
##########
datafusion/physical-plan/src/topk/mod.rs:
##########
@@ -1845,4 +2060,157 @@ mod tests {
Ok(())
}
+
+ /// Builds a `(pk Int32, val Int32)` schema and a `PartitionedTopK`
+ /// partitioned by `pk` with order `val ASC`. Helper for the
+ /// `PartitionedTopK` tests below.
+ fn build_partitioned_topk(k: usize) -> Result<(Arc<Schema>,
PartitionedTopK)> {
Review Comment:
Nice addition to the test coverage. One thing that might be worth adding is
a regression covering `ORDER BY val DESC` and/or NULL sort values. This path
relies on Arrow row encoding for sort direction and null ordering, so having
one extra SQL-level or unit test would help protect the shared encoder rewrite
across the main ordering variants used by window Top-N queries. Not blocking
since this reuses the existing `build_sort_fields`/`RowConverter` path and the
current targeted tests pass.
--
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]
