You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/05/29 04:11:35 UTC
[GitHub] [spark] beliefer opened a new pull request, #36714: [SPARK-39320][SQL] Support aggregate function `MEDIAN`
beliefer opened a new pull request, #36714:
URL: https://github.com/apache/spark/pull/36714
### What changes were proposed in this pull request?
Many mainstream database supports aggregate function `MEDIAN`.
**Syntax：**
Aggregate function
`MEDIAN( <expr> )`
Window function
`MEDIAN( <expr> ) OVER ( [ PARTITION BY <expr2> ] )`
**Arguments：**
expr: The expression must evaluate to a numeric data type (INTEGER, FLOAT, DECIMAL, or equivalent).
**Examples**：
```
select k, median(v) from aggr group by k order by k;
+---+-----------+
| K | MEDIAN(V) |
|---+-----------|
| 1 | 20.00000 |
| 2 | 22.50000 |
| 3 | NULL |
+---+-----------+
```
### Why are the changes needed?
The mainstream database supports `MEDIAN` show below:
Snowflake
### Does this PR introduce _any_ user-facing change?
'No'.
New feature.
### How was this patch tested?
New 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a diff in pull request #36714: [SPARK-39320][SQL] Support aggregate function `MEDIAN`
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #36714:
URL: https://github.com/apache/spark/pull/36714#discussion_r888289663
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/percentiles.scala:
##########
@@ -359,6 +359,32 @@ case class Percentile(
)
}
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = "_FUNC_(col) - Returns the median of numeric or ansi interval column `col`.",
+ examples = """
+ Examples:
+ > SELECT _FUNC_(col) FROM VALUES (0), (10) AS tab(col);
+ 5.0
+ > SELECT _FUNC_(col) FROM VALUES (INTERVAL '0' MONTH), (INTERVAL '10' MONTH) AS tab(col);
+ 5.0
+ """,
+ group = "agg_funcs",
+ since = "3.4.0")
+// scalastyle:on line.size.limit
+case class Median(child: Expression)
+ extends AggregateFunction
+ with RuntimeReplaceableAggregate
+ with ImplicitCastInputTypes
+ with UnaryLike[Expression] {
Review Comment:
Should be aligned to `extends`, see https://github.com/databricks/scala-style-guide#spacing-and-indentation
```suggestion
with RuntimeReplaceableAggregate
with ImplicitCastInputTypes
with UnaryLike[Expression] {
```
```suggestion
with RuntimeReplaceableAggregate
with ImplicitCastInputTypes
with UnaryLike[Expression] {
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/percentiles.scala:
##########
@@ -359,6 +359,32 @@ case class Percentile(
)
}
+// scalastyle:off line.size.limit
Review Comment:
Is this really needed?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/percentiles.scala:
##########
@@ -359,6 +359,32 @@ case class Percentile(
)
}
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = "_FUNC_(col) - Returns the median of numeric or ansi interval column `col`.",
+ examples = """
+ Examples:
+ > SELECT _FUNC_(col) FROM VALUES (0), (10) AS tab(col);
+ 5.0
+ > SELECT _FUNC_(col) FROM VALUES (INTERVAL '0' MONTH), (INTERVAL '10' MONTH) AS tab(col);
+ 5.0
+ """,
+ group = "agg_funcs",
+ since = "3.4.0")
+// scalastyle:on line.size.limit
+case class Median(child: Expression)
+ extends AggregateFunction
+ with RuntimeReplaceableAggregate
+ with ImplicitCastInputTypes
+ with UnaryLike[Expression] {
Review Comment:
Should be aligned to `extends`, see https://github.com/databricks/scala-style-guide#spacing-and-indentation
```suggestion
with RuntimeReplaceableAggregate
with ImplicitCastInputTypes
with UnaryLike[Expression] {
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on a diff in pull request #36714: [SPARK-39320][SQL] Support aggregate function `MEDIAN`
Posted by GitBox <gi...@apache.org>.
beliefer commented on code in PR #36714:
URL: https://github.com/apache/spark/pull/36714#discussion_r887439535
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/percentiles.scala:
##########
@@ -359,6 +359,32 @@ case class Percentile(
)
}
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = "_FUNC_(col) - Returns the median of numeric or ansi interval column `col`.",
+ examples = """
+ Examples:
+ > SELECT _FUNC_(col) FROM VALUES (0), (10) AS tab(col);
+ 5.0
+ > SELECT _FUNC_(col) FROM VALUES (INTERVAL '0' MONTH), (INTERVAL '10' MONTH) AS tab(col);
+ 5.0
+ """,
+ group = "agg_funcs",
+ since = "3.4.0")
+// scalastyle:on line.size.limit
+case class Median(child: Expression)
+ extends AggregateFunction
+ with RuntimeReplaceableAggregate
+ with ImplicitCastInputTypes
+ with UnaryLike[Expression] {
+ private lazy val percentile = new Percentile(child, Literal(0.5, DoubleType))
+ override def replacement: Expression = percentile
+ override def nodeName: String = "median"
+ override def inputTypes: Seq[AbstractDataType] = percentile.inputTypes.take(1)
Review Comment:
It seems we could supports ansi interval too.
`TypeCollection(NumericType, YearMonthIntervalType, DayTimeIntervalType)`
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk closed pull request #36714: [SPARK-39320][SQL] Support aggregate function `MEDIAN`
Posted by GitBox <gi...@apache.org>.
MaxGekk closed pull request #36714: [SPARK-39320][SQL] Support aggregate function `MEDIAN`
URL: https://github.com/apache/spark/pull/36714
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #36714: [SPARK-39320][SQL] Support aggregate function `MEDIAN`
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36714:
URL: https://github.com/apache/spark/pull/36714#discussion_r886740530
##########
sql/core/src/test/resources/sql-tests/inputs/percentiles.sql:
##########
@@ -0,0 +1,208 @@
+-- Test data.
+CREATE OR REPLACE TEMPORARY VIEW aggr AS SELECT * FROM VALUES
+(0, 0), (0, 10), (0, 20), (0, 30), (0, 40), (1, 10), (1, 20), (2, 10), (2, 20), (2, 25), (2, 30), (3, 60), (4, null)
+AS aggr(k, v);
+CREATE OR REPLACE TEMPORARY VIEW basic_pays AS SELECT * FROM VALUES
Review Comment:
please add a blank line between SQL statements.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #36714: [SPARK-39320][SQL] Support aggregate function `MEDIAN`
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36714:
URL: https://github.com/apache/spark/pull/36714#discussion_r886739715
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/percentiles.scala:
##########
@@ -359,6 +359,32 @@ case class Percentile(
)
}
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = "_FUNC_(col) - Returns the median of numeric or ansi interval column `col`.",
+ examples = """
+ Examples:
+ > SELECT _FUNC_(col) FROM VALUES (0), (10) AS tab(col);
+ 5.0
+ > SELECT _FUNC_(col) FROM VALUES (INTERVAL '0' MONTH), (INTERVAL '10' MONTH) AS tab(col);
+ 5.0
+ """,
+ group = "agg_funcs",
+ since = "3.4.0")
+// scalastyle:on line.size.limit
+case class Median(child: Expression)
+ extends AggregateFunction
+ with RuntimeReplaceableAggregate
+ with ImplicitCastInputTypes
+ with UnaryLike[Expression] {
+ private lazy val percentile = new Percentile(child, Literal(0.5, DoubleType))
+ override def replacement: Expression = percentile
+ override def nodeName: String = "median"
+ override def inputTypes: Seq[AbstractDataType] = percentile.inputTypes.take(1)
Review Comment:
isn't it just `Seq(DoubleType)`?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #36714: [SPARK-39320][SQL] Support aggregate function `MEDIAN`
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36714:
URL: https://github.com/apache/spark/pull/36714#discussion_r885172773
##########
sql/core/src/test/resources/sql-tests/inputs/group-by.sql:
##########
@@ -273,3 +273,16 @@ SELECT
FROM aggr
GROUP BY k
ORDER BY k;
+
+-- SPARK-39320: Add the MEDIAN() function
Review Comment:
and we can test both window and group-by there.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on pull request #36714: [SPARK-39320][SQL] Support aggregate function `MEDIAN`
Posted by GitBox <gi...@apache.org>.
beliefer commented on PR #36714:
URL: https://github.com/apache/spark/pull/36714#issuecomment-1146566218
@MaxGekk @cloud-fan Thank you for review.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a diff in pull request #36714: [SPARK-39320][SQL] Support aggregate function `MEDIAN`
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #36714:
URL: https://github.com/apache/spark/pull/36714#discussion_r888289663
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/percentiles.scala:
##########
@@ -359,6 +359,32 @@ case class Percentile(
)
}
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = "_FUNC_(col) - Returns the median of numeric or ansi interval column `col`.",
+ examples = """
+ Examples:
+ > SELECT _FUNC_(col) FROM VALUES (0), (10) AS tab(col);
+ 5.0
+ > SELECT _FUNC_(col) FROM VALUES (INTERVAL '0' MONTH), (INTERVAL '10' MONTH) AS tab(col);
+ 5.0
+ """,
+ group = "agg_funcs",
+ since = "3.4.0")
+// scalastyle:on line.size.limit
+case class Median(child: Expression)
+ extends AggregateFunction
+ with RuntimeReplaceableAggregate
+ with ImplicitCastInputTypes
+ with UnaryLike[Expression] {
Review Comment:
Should be aligned to `extends`, see https://github.com/databricks/scala-style-guide#spacing-and-indentation
```suggestion
with RuntimeReplaceableAggregate
with ImplicitCastInputTypes
with UnaryLike[Expression] {
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on a diff in pull request #36714: [SPARK-39320][SQL] Support aggregate function `MEDIAN`
Posted by GitBox <gi...@apache.org>.
beliefer commented on code in PR #36714:
URL: https://github.com/apache/spark/pull/36714#discussion_r888554738
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/percentiles.scala:
##########
@@ -359,6 +359,32 @@ case class Percentile(
)
}
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = "_FUNC_(col) - Returns the median of numeric or ansi interval column `col`.",
+ examples = """
+ Examples:
+ > SELECT _FUNC_(col) FROM VALUES (0), (10) AS tab(col);
+ 5.0
+ > SELECT _FUNC_(col) FROM VALUES (INTERVAL '0' MONTH), (INTERVAL '10' MONTH) AS tab(col);
+ 5.0
+ """,
+ group = "agg_funcs",
+ since = "3.4.0")
+// scalastyle:on line.size.limit
+case class Median(child: Expression)
+ extends AggregateFunction
+ with RuntimeReplaceableAggregate
+ with ImplicitCastInputTypes
+ with UnaryLike[Expression] {
Review Comment:
I got it.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #36714: [SPARK-39320][SQL] Support aggregate function `MEDIAN`
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36714:
URL: https://github.com/apache/spark/pull/36714#discussion_r885172461
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/percentiles.scala:
##########
@@ -359,6 +359,32 @@ case class Percentile(
)
}
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = "_FUNC_(col) - Returns the median of numeric or ansi interval column `col`.",
+ examples = """
+ Examples:
+ > SELECT _FUNC_(col) FROM VALUES (0), (10) AS tab(col);
+ 5.0
+ > SELECT _FUNC_(col) FROM VALUES (INTERVAL '0' MONTH), (INTERVAL '10' MONTH) AS tab(col);
+ 5.0
+ """,
+ group = "agg_funcs",
+ since = "3.4.0")
+// scalastyle:on line.size.limit
+case class Median(child: Expression)
+ extends AggregateFunction
+ with RuntimeReplaceableAggregate
+ with ImplicitCastInputTypes
+ with UnaryLike[Expression] {
+ private lazy val percentile = new Percentile(child, Literal(0.5, DoubleType))
+ override def replacement: Expression = percentile
+ override def nodeName: String = "median"
+ override def inputTypes: Seq[AbstractDataType] = percentile.inputTypes
Review Comment:
are we sure? `Percentile` has 3 inputs but `Median` only has one
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #36714: [SPARK-39320][SQL] Support aggregate function `MEDIAN`
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36714:
URL: https://github.com/apache/spark/pull/36714#discussion_r885172628
##########
sql/core/src/test/resources/sql-tests/inputs/group-by.sql:
##########
@@ -273,3 +273,16 @@ SELECT
FROM aggr
GROUP BY k
ORDER BY k;
+
+-- SPARK-39320: Add the MEDIAN() function
Review Comment:
nit: can we add a new golden file for these statistical functions?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on pull request #36714: [SPARK-39320][SQL] Support aggregate function `MEDIAN`
Posted by GitBox <gi...@apache.org>.
beliefer commented on PR #36714:
URL: https://github.com/apache/spark/pull/36714#issuecomment-1141623396
ping @MaxGekk cc @cloud-fan
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #36714: [SPARK-39320][SQL] Support aggregate function `MEDIAN`
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36714:
URL: https://github.com/apache/spark/pull/36714#discussion_r886740706
##########
sql/core/src/test/resources/sql-tests/inputs/percentiles.sql:
##########
@@ -0,0 +1,208 @@
+-- Test data.
+CREATE OR REPLACE TEMPORARY VIEW aggr AS SELECT * FROM VALUES
+(0, 0), (0, 10), (0, 20), (0, 30), (0, 40), (1, 10), (1, 20), (2, 10), (2, 20), (2, 25), (2, 30), (3, 60), (4, null)
+AS aggr(k, v);
+CREATE OR REPLACE TEMPORARY VIEW basic_pays AS SELECT * FROM VALUES
+('Diane Murphy','Accounting',8435),
+('Mary Patterson','Accounting',9998),
+('Jeff Firrelli','Accounting',8992),
+('William Patterson','Accounting',8870),
+('Gerard Bondur','Accounting',11472),
+('Anthony Bow','Accounting',6627),
+('Leslie Jennings','IT',8113),
+('Leslie Thompson','IT',5186),
+('Julie Firrelli','Sales',9181),
+('Steve Patterson','Sales',9441),
+('Foon Yue Tseng','Sales',6660),
+('George Vanauf','Sales',10563),
+('Loui Bondur','SCM',10449),
+('Gerard Hernandez','SCM',6949),
+('Pamela Castillo','SCM',11303),
+('Larry Bott','SCM',11798),
+('Barry Jones','SCM',10586)
+AS basic_pays(employee_name, department, salary);
+
+SELECT
+ percentile_cont(0.25) WITHIN GROUP (ORDER BY v),
+ percentile_cont(0.25) WITHIN GROUP (ORDER BY v DESC)
+FROM aggr;
+SELECT
Review Comment:
ditto
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org