You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "YutaLin (via GitHub)" <gi...@apache.org> on 2026/03/21 20:50:20 UTC
[PR] feat: Support Spark expression: percentile_cont [datafusion-comet]
YutaLin opened a new pull request, #3757:
URL: https://github.com/apache/datafusion-comet/pull/3757
## Which issue does this PR close?
Comet does not currently support the Spark percentile_cont function, causing queries using this function to fall back to Spark's JVM execution instead of running natively on DataFusion.
PercentileCont calculates a percentile value based on a continuous distribution of numeric or ANSI interval columns at a given percentage. It implements the SQL PERCENTILE_CONT function which uses linear interpolation between values when the exact percentile position falls between two data points. This expression is a runtime-replaceable aggregate that delegates to the internal Percentile implementation.
Supporting this expression would allow more Spark workloads to benefit from Comet's native acceleration.
Not include array percentile and weighted percentile now.
Closes #3190
## What changes are included in this PR?
Add PercentileCont message for expr.proto
Add `CometPercentile` with validations
Register Percentile class in QueryPlanSerde
Handle PercentileCont protobuf in `planner.rs`
Custom `percentile.rs` with Binary state because Datafusion percentile_cont stores all values as List<Float64> and shuffle with that will cause `Cannot cast list to non-list data types` error
## How are these changes tested?
Add sql test include numerical and interval
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark expression: percentile_cont [datafusion-comet]
Posted by "coderfender (via GitHub)" <gi...@apache.org>.
coderfender commented on code in PR #3757:
URL: https://github.com/apache/datafusion-comet/pull/3757#discussion_r2970600883
##########
spark/src/main/scala/org/apache/comet/serde/aggregates.scala:
##########
@@ -671,6 +672,62 @@ object CometBloomFilterAggregate extends CometAggregateExpressionSerde[BloomFilt
}
}
+object CometPercentile extends CometAggregateExpressionSerde[Percentile] {
+ override def convert(
+ aggExpr: AggregateExpression,
+ expr: Percentile,
+ inputs: Seq[Attribute],
+ binding: Boolean,
+ conf: SQLConf): Option[ExprOuterClass.AggExpr] = {
+
+ // Only support when frequency is Literal(1L) - i.e., percentile_cont behavior
+ expr.frequencyExpression match {
+ case Literal(1L, LongType) =>
+ case _ =>
+ withInfo(aggExpr, "weighted percentile not supported")
+ return None
+ }
+
+ // Only support scalar percentile, not array of percentiles
+ if (expr.percentageExpression.dataType.isInstanceOf[ArrayType]) {
+ withInfo(aggExpr, "array of percentiles not supported")
+ return None
+ }
+
+ // Support numeric types and interval types
+ expr.child.dataType match {
+ case _: NumericType =>
+ case _: DecimalType =>
+ case _: YearMonthIntervalType =>
+ case _: DayTimeIntervalType =>
+ case _ =>
+ withInfo(aggExpr, s"unsupported input type: ${expr.child.dataType}")
+ return None
+ }
+
+ val childExpr = exprToProto(expr.child, inputs, binding)
+ val percentileExpr = exprToProto(expr.percentageExpression, inputs, binding)
+ val dataType = serializeDataType(expr.dataType)
+
Review Comment:
nit : Any reason this if condition wouldn't always be true at all times ?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark expression: percentile_cont [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #3757:
URL: https://github.com/apache/datafusion-comet/pull/3757#issuecomment-4178421165
@YutaLin could you run `cargo fmt --all` to fix lint failures
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark expression: percentile_cont [datafusion-comet]
Posted by "YutaLin (via GitHub)" <gi...@apache.org>.
YutaLin commented on code in PR #3757:
URL: https://github.com/apache/datafusion-comet/pull/3757#discussion_r2978085522
##########
spark/src/main/scala/org/apache/comet/serde/aggregates.scala:
##########
@@ -671,6 +672,62 @@ object CometBloomFilterAggregate extends CometAggregateExpressionSerde[BloomFilt
}
}
+object CometPercentile extends CometAggregateExpressionSerde[Percentile] {
+ override def convert(
+ aggExpr: AggregateExpression,
+ expr: Percentile,
+ inputs: Seq[Attribute],
+ binding: Boolean,
+ conf: SQLConf): Option[ExprOuterClass.AggExpr] = {
+
+ // Only support when frequency is Literal(1L) - i.e., percentile_cont behavior
+ expr.frequencyExpression match {
+ case Literal(1L, LongType) =>
+ case _ =>
+ withInfo(aggExpr, "weighted percentile not supported")
+ return None
+ }
+
+ // Only support scalar percentile, not array of percentiles
+ if (expr.percentageExpression.dataType.isInstanceOf[ArrayType]) {
+ withInfo(aggExpr, "array of percentiles not supported")
+ return None
+ }
+
+ // Support numeric types and interval types
+ expr.child.dataType match {
+ case _: NumericType =>
+ case _: DecimalType =>
+ case _: YearMonthIntervalType =>
+ case _: DayTimeIntervalType =>
+ case _ =>
+ withInfo(aggExpr, s"unsupported input type: ${expr.child.dataType}")
+ return None
+ }
+
+ val childExpr = exprToProto(expr.child, inputs, binding)
+ val percentileExpr = exprToProto(expr.percentageExpression, inputs, binding)
+ val dataType = serializeDataType(expr.dataType)
+
Review Comment:
Hi @coderfender,
The child expression might contain unsupported sub-expressions, also reference attributes that can't be resolved.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark expression: percentile_cont [datafusion-comet]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #3757: feat: Support Spark expression: percentile_cont
URL: https://github.com/apache/datafusion-comet/pull/3757
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark expression: percentile_cont [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #3757:
URL: https://github.com/apache/datafusion-comet/pull/3757#discussion_r2971648120
##########
spark/src/main/scala/org/apache/comet/serde/aggregates.scala:
##########
@@ -671,6 +672,62 @@ object CometBloomFilterAggregate extends CometAggregateExpressionSerde[BloomFilt
}
}
+object CometPercentile extends CometAggregateExpressionSerde[Percentile] {
+ override def convert(
+ aggExpr: AggregateExpression,
+ expr: Percentile,
+ inputs: Seq[Attribute],
+ binding: Boolean,
+ conf: SQLConf): Option[ExprOuterClass.AggExpr] = {
+
+ // Only support when frequency is Literal(1L) - i.e., percentile_cont behavior
+ expr.frequencyExpression match {
+ case Literal(1L, LongType) =>
+ case _ =>
+ withInfo(aggExpr, "weighted percentile not supported")
+ return None
+ }
+
+ // Only support scalar percentile, not array of percentiles
+ if (expr.percentageExpression.dataType.isInstanceOf[ArrayType]) {
+ withInfo(aggExpr, "array of percentiles not supported")
+ return None
+ }
+
+ // Support numeric types and interval types
+ expr.child.dataType match {
+ case _: NumericType =>
+ case _: DecimalType =>
Review Comment:
The match on `DecimalType` is unreachable (because `NumericType` already handles it)
```suggestion
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark expression: percentile_cont [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #3757:
URL: https://github.com/apache/datafusion-comet/pull/3757#discussion_r2971649122
##########
spark/src/test/resources/sql-tests/expressions/aggregate/percentile_cont.sql:
##########
@@ -0,0 +1,77 @@
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements. See the NOTICE file
+-- distributed with this work for additional information
+-- regarding copyright ownership. The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"); you may not use this file except in compliance
+-- with the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied. See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+
+-- Tests for percentile_cont aggregate function
+-- Uses similar test data as Spark's percentiles.sql
+
+statement
+CREATE TABLE test_percentile(k int, v int) USING parquet
Review Comment:
Thanks for adding the SQL tests! It would be great to add tests with negative values, boundary percentiles (0.0 and 1.0), all-null groups, single-value groups, and DOUBLE/FLOAT column types. The negative values test is especially important since the sort order matters for correctness and I think there may currently be bugs in the implementation, but would be good to start with adding tests
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark expression: percentile_cont [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #3757:
URL: https://github.com/apache/datafusion-comet/pull/3757#issuecomment-4384073780
I moved this to draft since feedback has not been addressed yet. @YutaLin please mark as ready for review if/when you pick this up again
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark expression: percentile_cont [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #3757:
URL: https://github.com/apache/datafusion-comet/pull/3757#discussion_r2971646686
##########
spark/src/main/scala/org/apache/comet/serde/aggregates.scala:
##########
@@ -22,9 +22,10 @@ package org.apache.comet.serde
import scala.jdk.CollectionConverters._
import org.apache.spark.sql.catalyst.expressions.Attribute
-import org.apache.spark.sql.catalyst.expressions.aggregate.{AggregateExpression, Average, BitAndAgg, BitOrAgg, BitXorAgg, BloomFilterAggregate, CentralMomentAgg, Corr, Count, Covariance, CovPopulation, CovSample, First, Last, Max, Min, StddevPop, StddevSamp, Sum, VariancePop, VarianceSamp}
+import org.apache.spark.sql.catalyst.expressions.Literal
+import org.apache.spark.sql.catalyst.expressions.aggregate.{AggregateExpression, Average, BitAndAgg, BitOrAgg, BitXorAgg, BloomFilterAggregate, CentralMomentAgg, Corr, Count, Covariance, CovPopulation, CovSample, First, Last, Max, Min, Percentile, StddevPop, StddevSamp, Sum, VariancePop, VarianceSamp}
import org.apache.spark.sql.internal.SQLConf
-import org.apache.spark.sql.types.{ByteType, DataTypes, DecimalType, IntegerType, LongType, ShortType, StringType}
+import org.apache.spark.sql.types.{ArrayType, ByteType, DataTypes, DayTimeIntervalType, DecimalType, IntegerType, LongType, NumericType, , StringType, YearMonthIntervalType}
Review Comment:
```suggestion
import org.apache.spark.sql.types.{ArrayType, ByteType, DataTypes, DayTimeIntervalType, DecimalType, IntegerType, LongType, NumericType, StringType, YearMonthIntervalType}
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark expression: percentile_cont [datafusion-comet]
Posted by "coderfender (via GitHub)" <gi...@apache.org>.
coderfender commented on code in PR #3757:
URL: https://github.com/apache/datafusion-comet/pull/3757#discussion_r2970600033
##########
spark/src/main/scala/org/apache/comet/serde/aggregates.scala:
##########
@@ -671,6 +672,62 @@ object CometBloomFilterAggregate extends CometAggregateExpressionSerde[BloomFilt
}
}
+object CometPercentile extends CometAggregateExpressionSerde[Percentile] {
+ override def convert(
+ aggExpr: AggregateExpression,
+ expr: Percentile,
+ inputs: Seq[Attribute],
+ binding: Boolean,
+ conf: SQLConf): Option[ExprOuterClass.AggExpr] = {
+
+ // Only support when frequency is Literal(1L) - i.e., percentile_cont behavior
+ expr.frequencyExpression match {
+ case Literal(1L, LongType) =>
+ case _ =>
+ withInfo(aggExpr, "weighted percentile not supported")
+ return None
+ }
+
+ // Only support scalar percentile, not array of percentiles
+ if (expr.percentageExpression.dataType.isInstanceOf[ArrayType]) {
+ withInfo(aggExpr, "array of percentiles not supported")
+ return None
+ }
Review Comment:
nit : could be a better idea to test scalar and reject all other inputs to make the code more defensive ?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark expression: percentile_cont [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #3757:
URL: https://github.com/apache/datafusion-comet/pull/3757#issuecomment-4178458553
Some test suggestions for edge cases that could reveal incompatibilities. The main risk is that Comet casts all inputs to f64 before accumulation, while Spark stores original typed values in its HashMap.
**Large integers that lose f64 precision** - Spark keeps these as distinct keys, Comet would merge them into one count after f64 cast:
```sql
SELECT percentile_cont(0.5) WITHIN GROUP (ORDER BY v)
FROM (VALUES (9007199254740992), (9007199254740993)) AS t(v)
```
**NaN handling** - Spark includes NaN in the sort (sorts high via Java's Double.compare). Worth checking Comet matches:
```sql
SELECT percentile_cont(0.5) WITHIN GROUP (ORDER BY v)
FROM (VALUES (1.0), (2.0), (CAST('NaN' AS DOUBLE))) AS t(v)
```
**Negative zero**:
```sql
SELECT percentile_cont(0.5) WITHIN GROUP (ORDER BY v)
FROM (VALUES (CAST(-0.0 AS DOUBLE)), (CAST(0.0 AS DOUBLE)), (1.0)) AS t(v)
```
**Infinity**:
```sql
SELECT percentile_cont(0.5) WITHIN GROUP (ORDER BY v)
FROM (VALUES (1.0), (CAST('Infinity' AS DOUBLE)), (CAST('-Infinity' AS DOUBLE))) AS t(v)
```
If any of these fail, they could be added as `ignore` tests with a linked issue, and `getSupportLevel` could be updated to note the incompatibility.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark expression: percentile_cont [datafusion-comet]
Posted by "coderfender (via GitHub)" <gi...@apache.org>.
coderfender commented on PR #3757:
URL: https://github.com/apache/datafusion-comet/pull/3757#issuecomment-4250104303
Thank you for raising this PR @YutaLin . Could you please run `make format` to clear out rust lint issues in the CI as well ? I generally set my `pre-commit.sh` file to Lint, check clippy and build the code . Hope that helps
```
set -euo pipefail
make format
cd native && cargo clippy --all-targets -- -D warnings
make clean
PROFILES="-Pspark-4.0" make release
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark expression: percentile_cont [datafusion-comet]
Posted by "YutaLin (via GitHub)" <gi...@apache.org>.
YutaLin commented on code in PR #3757:
URL: https://github.com/apache/datafusion-comet/pull/3757#discussion_r2978066285
##########
spark/src/test/resources/sql-tests/expressions/aggregate/percentile_cont.sql:
##########
@@ -0,0 +1,77 @@
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements. See the NOTICE file
+-- distributed with this work for additional information
+-- regarding copyright ownership. The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"); you may not use this file except in compliance
+-- with the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied. See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+
+-- Tests for percentile_cont aggregate function
+-- Uses similar test data as Spark's percentiles.sql
+
+statement
+CREATE TABLE test_percentile(k int, v int) USING parquet
Review Comment:
Hi @andygrove
thanks for review, I've added Negative values, Boundary percentiles 0.0 and 1.0, All-null groups and single-value groups, DOUBLE column type, FLOAT column type
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark expression: percentile_cont [datafusion-comet]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3757:
URL: https://github.com/apache/datafusion-comet/pull/3757#issuecomment-4884548032
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org