Skip to content

Commit 42faf28

Browse files
committed
Merge remote-tracking branch 'origin/main' into feature/delta-native-scan
Main now builds the empty-partition EmptyExec from the projected table schema, partition and constant metadata columns included, so fused operators bind the same way when a bucket is empty. That lives in the shared build_parquet_scan_plan, so an empty or fully pruned Delta partition gets it too. The native RDD keeps main's compute override for scan input metrics.
2 parents de2b013 + 605051a commit 42faf28

22 files changed

Lines changed: 1237 additions & 54 deletions

File tree

‎.github/workflows/pr_build_linux.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -525,6 +525,7 @@ jobs:
525525
org.apache.comet.exec.CometAggregateSuite
526526
org.apache.comet.exec.CometExec3_4PlusSuite
527527
org.apache.comet.exec.CometExecSuite
528+
org.apache.comet.exec.CometTopKSuite
528529
org.apache.comet.exec.CometEmptyRelationExecSuite
529530
org.apache.comet.exec.CometInMemoryCacheSuite
530531
org.apache.comet.exec.CometInMemoryCacheKryoSuite

‎.github/workflows/pr_build_macos.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,7 @@ jobs:
173173
org.apache.comet.exec.CometAggregateSuite
174174
org.apache.comet.exec.CometExec3_4PlusSuite
175175
org.apache.comet.exec.CometExecSuite
176+
org.apache.comet.exec.CometTopKSuite
176177
org.apache.comet.exec.CometEmptyRelationExecSuite
177178
org.apache.comet.exec.CometInMemoryCacheSuite
178179
org.apache.comet.exec.CometInMemoryCacheKryoSuite

‎docs/source/contributor-guide/adding_a_new_operator.md‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,21 @@ in `CometExecRule` rather than using `CometOperatorSerde`, because they don't ne
7070

7171
Examples: `CometBroadcastExchangeExec`, `CometShuffleExchangeExec`
7272

73+
#### Local TopK inside a sink
74+
75+
`CometTakeOrderedAndProjectExec` owns local candidate selection, the optional shuffle, and final
76+
selection with offset and projection. When `spark.comet.exec.topK.fusion.enabled` is enabled for an
77+
eligible native Parquet scan, its conversion inserts `CometLocalTopKExec` before native blocks are
78+
serialized. The local node extends `CometUnaryExec` and serializes a bounded sort above the native
79+
scan, so they execute in one block. It preserves the scan's output and partitioning and advertises
80+
the local sort order.
81+
82+
The inserted node keeps the original Spark TopK for plan bookkeeping, but only the outer TopK owns
83+
Spark's offset and projection. Both transition reversion and aggregate-buffer restoration therefore
84+
remove the inserted local node when restoring Spark execution. Restoring its `originalPlan` would
85+
apply the global TopK twice. A single input partition uses a final native limit and projection;
86+
multiple input partitions still require a final TopK after the shuffle.
87+
7388
### Choosing the Right Operator Type
7489

7590
When adding a new operator, choose based on these criteria:

‎docs/source/user-guide/latest/expressions.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -298,7 +298,7 @@ The type-name conversion functions (`bigint`, `binary`, `boolean`, `date`, `deci
298298
| `timediff` | ✅ | — | Spark 4.0+ grammar alias that parses to `timestampdiff`; runs through codegen dispatch |
299299
| `timestamp_micros` | ✅ | Codegen dispatch | |
300300
| `timestamp_millis` | ✅ | Codegen dispatch | |
301-
| `timestamp_seconds` | ✅ | Native | |
301+
| `timestamp_seconds` | ✅ | Hybrid | Integer, long, float and double inputs run natively; decimal, byte and short inputs route through the JVM codegen dispatcher |
302302
| `timestampadd` | ✅ | — | Reached through the grammar rather than the function registry; runs through codegen dispatch |
303303
| `timestampdiff` | ✅ | — | Reached through the grammar rather than the function registry; runs through codegen dispatch |
304304
| `to_date` | ✅ | — | Rewrites to `Cast` (or `Cast(GetTimestamp)` with a format) before Comet sees the plan |

‎docs/source/user-guide/latest/tuning/operators.md‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,24 @@ SET spark.comet.datafusion.execution.skip_partial_aggregation_probe_ratio_thresh
9999
These settings only tune eligible plans. Unsupported accumulators and modes remain
100100
disabled even when configuration overrides are enabled.
101101

102+
## Local TopK Fusion
103+
104+
Set `spark.comet.exec.topK.fusion.enabled=true` to run an eligible local TopK in the same native
105+
execution as its Parquet scan. This experimental optimization is disabled by default. It currently
106+
supports a direct native Parquet scan ordered by one signed integer column (`TINYINT`, `SMALLINT`,
107+
`INT`, or `BIGINT`). Both sort directions and null orderings are supported. Other inputs use the
108+
existing TopK execution path.
109+
110+
Each scan partition keeps enough candidates for both `LIMIT` and `OFFSET`. With multiple partitions,
111+
Comet shuffles those candidates and performs the final TopK. With one partition, it reuses the local
112+
ordering without building a second heap. The final stage applies the offset and output projection.
113+
114+
Fusion reduces the work of passing scan batches between native execution blocks. It still reads all
115+
input rows and does not enable TopK reader pruning. It can also reduce overlap between scan decoding
116+
and TopK processing, so some workloads may run slower. Compare enabled and disabled runs with your
117+
data layout, payload width, limit, and partition count before enabling it. The
118+
`CometTopKBenchmark` microbenchmark covers these cases with ascending, descending, and random layouts.
119+
102120
## Optimizing Sorting on Floating-Point Values
103121

104122
Comet normalizes NaN payloads and signed zeros in scalar `FLOAT` and `DOUBLE` ordering keys, so `ORDER BY`, window

‎native/core/src/execution/planner.rs‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@ use datafusion_comet_spark_expr::{
9797
EvalMode, SparkArraysZipFunc, SparkBloomFilterVersion, SparkListAgg, SparkPercentile,
9898
SumInteger, ToCsv,
9999
};
100+
use datafusion_datasource::TableSchema;
100101
use iceberg::expr::Bind;
101102

102103
use crate::execution::operators::ExecutionError::GeneralError;
@@ -488,7 +489,19 @@ impl PhysicalPlanner {
488489
// Check if this partition has any files (bucketed scan with bucket pruning may have
489490
// empty partitions; a fully-pruned Delta partition likewise).
490491
if files.is_empty() {
491-
let empty_exec = Arc::new(EmptyExec::new(required_schema));
492+
// Match the nonempty scan's projected table schema, including partition
493+
// and constant metadata columns. Fused operators bind against this schema
494+
// even when the bucket has no rows.
495+
let partition_fields: Vec<FieldRef> = partition_schema
496+
.fields()
497+
.iter()
498+
.map(|f| Arc::new(Field::new(f.name(), f.data_type().clone(), f.is_nullable())))
499+
.collect();
500+
let table_schema = TableSchema::builder(data_schema)
501+
.with_table_partition_cols(partition_fields)
502+
.build();
503+
let output_schema = Arc::new(table_schema.table_schema().project(&projection_vector)?);
504+
let empty_exec = Arc::new(EmptyExec::new(output_schema));
492505
return Ok(Arc::new(SparkPlan::new(plan_id, empty_exec, vec![])));
493506
}
494507

@@ -5161,6 +5174,8 @@ fn needs_fields_coercion(sig: &TypeSignature) -> bool {
51615174

51625175
#[cfg(test)]
51635176
mod tests {
5177+
mod empty_native_scan;
5178+
51645179
use futures::{poll, StreamExt};
51655180
use std::{
51665181
sync::{
Lines changed: 173 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,173 @@
1+
// Licensed to the Apache Software Foundation (ASF) under one
2+
// or more contributor license agreements. See the NOTICE file
3+
// distributed with this work for additional information
4+
// regarding copyright ownership. The ASF licenses this file
5+
// to you under the Apache License, Version 2.0 (the
6+
// "License"); you may not use this file except in compliance
7+
// with the License. You may obtain a copy of the License at
8+
//
9+
// http://www.apache.org/licenses/LICENSE-2.0
10+
//
11+
// Unless required by applicable law or agreed to in writing,
12+
// software distributed under the License is distributed on an
13+
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
// KIND, either express or implied. See the License for the
15+
// specific language governing permissions and limitations
16+
// under the License.
17+
18+
use std::collections::HashMap;
19+
20+
use arrow::datatypes::{DataType, Field, Schema, SchemaRef};
21+
use datafusion_comet_proto::spark_expression::{
22+
data_type::DataTypeId, expr::ExprStruct, literal::Value, DataType as SparkDataType, Expr,
23+
Literal,
24+
};
25+
use datafusion_comet_proto::spark_operator::{
26+
operator::OpStruct, NativeScan, NativeScanCommon, Operator, SparkFilePartition,
27+
SparkPartitionedFile, SparkStructField,
28+
};
29+
30+
use crate::execution::planner::PhysicalPlanner;
31+
32+
fn spark_field(
33+
name: &str,
34+
data_type: DataTypeId,
35+
nullable: bool,
36+
field_id: &str,
37+
) -> SparkStructField {
38+
SparkStructField {
39+
name: name.to_string(),
40+
data_type: Some(SparkDataType {
41+
type_id: data_type as i32,
42+
type_info: None,
43+
}),
44+
nullable,
45+
metadata: HashMap::from([("parquet.field.id".to_string(), field_id.to_string())]),
46+
}
47+
}
48+
49+
fn common(required_data_indices: &[usize], projection: Vec<i64>) -> NativeScanCommon {
50+
let data_schema = vec![
51+
spark_field("id", DataTypeId::Int32, false, "1"),
52+
spark_field("unused", DataTypeId::String, true, "2"),
53+
spark_field("payload", DataTypeId::Int64, true, "3"),
54+
];
55+
NativeScanCommon {
56+
required_schema: required_data_indices
57+
.iter()
58+
.map(|&index| data_schema[index].clone())
59+
.collect(),
60+
data_schema,
61+
// Constant file metadata is appended alongside partition values by the JVM scan.
62+
partition_schema: vec![
63+
spark_field("part", DataTypeId::Int32, false, "4"),
64+
spark_field("_metadata.file_size", DataTypeId::Int64, false, "5"),
65+
],
66+
projection_vector: projection,
67+
session_timezone: "UTC".to_string(),
68+
..Default::default()
69+
}
70+
}
71+
72+
fn literal(data_type: DataTypeId, value: Value) -> Expr {
73+
Expr {
74+
expr_struct: Some(ExprStruct::Literal(Literal {
75+
datatype: Some(SparkDataType {
76+
type_id: data_type as i32,
77+
type_info: None,
78+
}),
79+
value: Some(value),
80+
is_null: false,
81+
})),
82+
..Default::default()
83+
}
84+
}
85+
86+
fn plan_schema(common: &NativeScanCommon, empty: bool) -> SchemaRef {
87+
let files = if empty {
88+
vec![]
89+
} else {
90+
// Planning does not read the file. Only its presence selects the nonempty path.
91+
vec![SparkPartitionedFile {
92+
file_path: "file:///tmp/comet-empty-native-scan-schema.parquet".to_string(),
93+
start: 0,
94+
length: 16,
95+
file_size: 16,
96+
partition_values: vec![
97+
literal(DataTypeId::Int32, Value::IntVal(7)),
98+
literal(DataTypeId::Int64, Value::LongVal(16)),
99+
],
100+
}]
101+
};
102+
let operator = Operator {
103+
op_struct: Some(OpStruct::NativeScan(NativeScan {
104+
common: Some(common.clone()),
105+
file_partition: Some(SparkFilePartition {
106+
partitioned_file: files,
107+
}),
108+
..Default::default()
109+
})),
110+
..Default::default()
111+
};
112+
let (_, _, plan) = PhysicalPlanner::default()
113+
.create_plan(&operator, &mut vec![], 1)
114+
.unwrap();
115+
plan.schema()
116+
}
117+
118+
fn assert_schema_matches_nonempty(common: NativeScanCommon, expected: Vec<Field>) {
119+
let nonempty_schema = plan_schema(&common, false);
120+
let empty_schema = plan_schema(&common, true);
121+
assert_eq!(nonempty_schema.as_ref(), &Schema::new(expected));
122+
assert_eq!(empty_schema, nonempty_schema);
123+
}
124+
125+
fn id_field() -> Field {
126+
Field::new("id", DataType::Int32, false).with_metadata(HashMap::from([(
127+
"parquet.field.id".to_string(),
128+
"1".to_string(),
129+
)]))
130+
}
131+
132+
fn payload_field() -> Field {
133+
Field::new("payload", DataType::Int64, true).with_metadata(HashMap::from([(
134+
"parquet.field.id".to_string(),
135+
"3".to_string(),
136+
)]))
137+
}
138+
139+
#[test]
140+
fn empty_scan_preserves_reordered_and_pruned_data_columns() {
141+
assert_schema_matches_nonempty(
142+
common(&[0, 2], vec![2, 0]),
143+
vec![payload_field(), id_field()],
144+
);
145+
}
146+
147+
#[test]
148+
fn empty_scan_preserves_partition_and_constant_metadata_projection() {
149+
// Match the nonempty path: data-field metadata is preserved, while partition
150+
// and constant metadata fields retain their names, types and nullability only.
151+
assert_schema_matches_nonempty(
152+
common(&[0, 2], vec![3, 2, 4, 0]),
153+
vec![
154+
Field::new("part", DataType::Int32, false),
155+
payload_field(),
156+
Field::new("_metadata.file_size", DataType::Int64, false),
157+
id_field(),
158+
],
159+
);
160+
}
161+
162+
#[test]
163+
fn empty_scan_preserves_partition_only_projection() {
164+
assert_schema_matches_nonempty(
165+
common(&[], vec![3]),
166+
vec![Field::new("part", DataType::Int32, false)],
167+
);
168+
}
169+
170+
#[test]
171+
fn empty_scan_preserves_zero_column_projection() {
172+
assert_schema_matches_nonempty(common(&[], vec![]), vec![]);
173+
}

‎spark/src/main/scala/org/apache/comet/CometConf.scala‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -447,6 +447,17 @@ object CometConf extends ShimCometConf {
447447
.booleanConf
448448
.createWithDefault(false)
449449

450+
val COMET_EXEC_TOPK_FUSION_ENABLED: ConfigEntry[Boolean] =
451+
conf(s"$COMET_EXEC_CONFIG_PREFIX.topK.fusion.enabled")
452+
.category(CATEGORY_EXEC)
453+
.doc(
454+
"Experimental opt-in: run an eligible local TopK in the same native execution as its " +
455+
"Parquet scan. Supports one direct signed integer sort key. This changes the local " +
456+
"execution pipeline and can reduce scan/TopK overlap, so it may be slower for some " +
457+
"workloads.")
458+
.booleanConf
459+
.createWithDefault(false)
460+
450461
val COMET_SCALA_UDF_CODEGEN_ENABLED: ConfigEntry[Boolean] =
451462
conf("spark.comet.exec.scalaUDF.codegen.enabled")
452463
.category(CATEGORY_EXEC)

‎spark/src/main/scala/org/apache/comet/rules/CometExecRule.scala‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,9 @@ case class CometExecRule(session: SparkSession)
261261
other
262262
} else {
263263
other match {
264+
// Only the outer TopK owns Spark's original offset and projection. If a future
265+
// fused input can contain a restored aggregate, remove its inserted local node.
266+
case _: CometLocalTopKExec => children.head
264267
// A native ancestor embeds its old child in nativeOp. Replacing only its SparkPlan
265268
// child would leave the incompatible native partial in that serialized plan.
266269
case comet: CometExec =>

‎spark/src/main/scala/org/apache/comet/rules/RevertNativeForTransitionHeavyStages.scala‎

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ import org.apache.spark.internal.Logging
2323
import org.apache.spark.sql.SparkSession
2424
import org.apache.spark.sql.catalyst.expressions.aggregate.{Final, Partial, PartialMerge}
2525
import org.apache.spark.sql.catalyst.rules.Rule
26-
import org.apache.spark.sql.comet.{CometColumnarToRowExec, CometExec, CometHashAggregateExec, CometNativeColumnarToRowExec, CometSparkToColumnarExec}
26+
import org.apache.spark.sql.comet.{CometColumnarToRowExec, CometExec, CometHashAggregateExec, CometLocalTopKExec, CometNativeColumnarToRowExec, CometSparkToColumnarExec}
2727
import org.apache.spark.sql.execution.{ColumnarToRowExec, ColumnarToRowTransition, RowToColumnarExec, SparkPlan}
2828
import org.apache.spark.sql.execution.adaptive.QueryStageExec
2929
import org.apache.spark.sql.execution.exchange.{BroadcastExchangeLike, ShuffleExchangeLike}
@@ -192,15 +192,19 @@ case class RevertNativeForTransitionHeavyStages(session: SparkSession, wholePlan
192192
case sparkToColumnar: CometSparkToColumnarExec => sparkToColumnar.child
193193
case RowToColumnarExec(child) => child
194194
}
195-
val reverted = transformStageUp(stripped) { case cometExec: CometExec =>
196-
if (cometExec.originalPlan.children.size == cometExec.children.size) {
197-
cometExec.originalPlan.withNewChildren(cometExec.children)
198-
} else {
199-
logWarning(
200-
"Comet plan and original have different child count for " +
201-
s"${cometExec.getClass.getSimpleName}, using originalPlan as-is.")
202-
cometExec.originalPlan
203-
}
195+
val reverted = transformStageUp(stripped) {
196+
// Local candidate selection was inserted by Comet. Only the outer TopK owns
197+
// the original Spark operator's offset and projection.
198+
case local: CometLocalTopKExec => local.child
199+
case cometExec: CometExec =>
200+
if (cometExec.originalPlan.children.size == cometExec.children.size) {
201+
cometExec.originalPlan.withNewChildren(cometExec.children)
202+
} else {
203+
logWarning(
204+
"Comet plan and original have different child count for " +
205+
s"${cometExec.getClass.getSimpleName}, using originalPlan as-is.")
206+
cometExec.originalPlan
207+
}
204208
}
205209
insertTransitions(reverted)
206210
}

0 commit comments

Comments
 (0)