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/07/01 11:19:04 UTC
[GitHub] [spark] beliefer opened a new pull request, #37047: [SPARK-39627][SQL] Unify `compileAggregate` and `compileExpression` in `JdbcDialect`
beliefer opened a new pull request, #37047:
URL: https://github.com/apache/spark/pull/37047
### What changes were proposed in this pull request?
Currently, `JdbcDialect` have two API `compileAggregate` and `compileExpression`, we can unify them.
### Why are the changes needed?
Improve ease of use.
### Does this PR introduce _any_ user-facing change?
'No'.
The two API `compileAggregate` call `compileExpression` not changed.
### How was this patch tested?
N/A
--
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 #37047: [SPARK-39627][SQL] DS V2 pushdown should unify the compile API
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37047:
URL: https://github.com/apache/spark/pull/37047#discussion_r915953731
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala:
##########
@@ -253,6 +253,19 @@ abstract class JdbcDialect extends Serializable with Logging {
}
}
+ override def visitAggregateFunction(
+ funcName: String, isDistinct: Boolean, inputs: Array[String]): String = {
+ if (isSupportedFunction(funcName)) {
+ super.visitAggregateFunction(
+ dialectFunctionName(funcName).getOrElse(funcName), isDistinct, inputs)
Review Comment:
This is a bit verbose. Can we make `dialectFunctionName` return string directly with default implementation
```
protected def dialectFunctionName(funcName: String): String = String
```
--
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 pull request #37047: [SPARK-39627][SQL] DS V2 pushdown should unify the compile API
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on PR #37047:
URL: https://github.com/apache/spark/pull/37047#issuecomment-1178659839
thanks, merging to master!
--
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 #37047: [SPARK-39627][SQL] DS V2 pushdown should unify the compile API
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37047:
URL: https://github.com/apache/spark/pull/37047#discussion_r913498986
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java:
##########
@@ -293,6 +321,11 @@ protected String visitGeneralAggregateFunction(
}
}
+ protected String visitGeneralAggregateFunction(
Review Comment:
shall we just have a single `visitAggregateFunction`?
--
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 #37047: [SPARK-39627][SQL] DS V2 pushdown should unify the compile API
Posted by GitBox <gi...@apache.org>.
beliefer commented on PR #37047:
URL: https://github.com/apache/spark/pull/37047#issuecomment-1173440586
ping @huaxingao 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 #37047: [SPARK-39627][SQL] DS V2 pushdown should unify the compile API
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37047:
URL: https://github.com/apache/spark/pull/37047#discussion_r913427733
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java:
##########
@@ -293,6 +321,12 @@ protected String visitGeneralAggregateFunction(
}
}
+ protected String visitGeneralAggregateFunction(
Review Comment:
why should it fail by default? it's not user defined.
--
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 #37047: [SPARK-39627][SQL] DS V2 pushdown should unify the compile API
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37047:
URL: https://github.com/apache/spark/pull/37047#discussion_r913499850
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/DB2Dialect.scala:
##########
@@ -31,34 +33,43 @@ private object DB2Dialect extends JdbcDialect {
url.toLowerCase(Locale.ROOT).startsWith("jdbc:db2")
// See https://www.ibm.com/docs/en/db2/11.5?topic=functions-aggregate
- override def compileAggregate(aggFunction: AggregateFunc): Option[String] = {
- super.compileAggregate(aggFunction).orElse(
- aggFunction match {
- case f: GeneralAggregateFunc if f.name() == "VAR_POP" =>
- assert(f.children().length == 1)
- val distinct = if (f.isDistinct) "DISTINCT " else ""
- Some(s"VARIANCE($distinct${f.children().head})")
- case f: GeneralAggregateFunc if f.name() == "VAR_SAMP" =>
- assert(f.children().length == 1)
- val distinct = if (f.isDistinct) "DISTINCT " else ""
- Some(s"VARIANCE_SAMP($distinct${f.children().head})")
- case f: GeneralAggregateFunc if f.name() == "STDDEV_POP" =>
- assert(f.children().length == 1)
- val distinct = if (f.isDistinct) "DISTINCT " else ""
- Some(s"STDDEV($distinct${f.children().head})")
- case f: GeneralAggregateFunc if f.name() == "STDDEV_SAMP" =>
- assert(f.children().length == 1)
- val distinct = if (f.isDistinct) "DISTINCT " else ""
- Some(s"STDDEV_SAMP($distinct${f.children().head})")
- case f: GeneralAggregateFunc if f.name() == "COVAR_POP" && f.isDistinct == false =>
- assert(f.children().length == 2)
- Some(s"COVARIANCE(${f.children().head}, ${f.children().last})")
- case f: GeneralAggregateFunc if f.name() == "COVAR_SAMP" && f.isDistinct == false =>
- assert(f.children().length == 2)
- Some(s"COVARIANCE_SAMP(${f.children().head}, ${f.children().last})")
- case _ => None
+ private val supportedAggregateFunctions =
+ Set("VAR_POP", "VAR_SAMP", "STDDEV_POP", "STDDEV_SAMP", "COVAR_POP", "COVAR_SAMP")
+ private val supportedFunctions = supportedAggregateFunctions
+
+ override def isSupportedFunction(funcName: String): Boolean =
+ supportedFunctions.contains(funcName)
+
+ class DB2SQLBuilder extends JDBCSQLBuilder {
Review Comment:
shall we update `JDBCSQLBuilder` to respect `supportedFunctions` in `visitAggregateFunction`?
--
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 #37047: [SPARK-39627][SQL] DS V2 pushdown should unify the compile API
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37047:
URL: https://github.com/apache/spark/pull/37047#discussion_r913553451
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala:
##########
@@ -303,26 +316,7 @@ abstract class JdbcDialect extends Serializable with Logging{
* @return Converted value.
*/
@Since("3.3.0")
- def compileAggregate(aggFunction: AggregateFunc): Option[String] = {
Review Comment:
can we deprecate 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] beliefer commented on a diff in pull request #37047: [SPARK-39627][SQL] DS V2 pushdown should unify the compile API
Posted by GitBox <gi...@apache.org>.
beliefer commented on code in PR #37047:
URL: https://github.com/apache/spark/pull/37047#discussion_r916419257
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala:
##########
@@ -253,6 +253,19 @@ abstract class JdbcDialect extends Serializable with Logging {
}
}
+ override def visitAggregateFunction(
+ funcName: String, isDistinct: Boolean, inputs: Array[String]): String = {
+ if (isSupportedFunction(funcName)) {
+ super.visitAggregateFunction(
+ dialectFunctionName(funcName).getOrElse(funcName), isDistinct, inputs)
Review Comment:
OK
--
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 #37047: [SPARK-39627][SQL] DS V2 pushdown should unify the compile API
Posted by GitBox <gi...@apache.org>.
beliefer commented on code in PR #37047:
URL: https://github.com/apache/spark/pull/37047#discussion_r913577883
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala:
##########
@@ -303,26 +316,7 @@ abstract class JdbcDialect extends Serializable with Logging{
* @return Converted value.
*/
@Since("3.3.0")
- def compileAggregate(aggFunction: AggregateFunc): Option[String] = {
Review Comment:
OK
--
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 closed pull request #37047: [SPARK-39627][SQL] DS V2 pushdown should unify the compile API
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #37047: [SPARK-39627][SQL] DS V2 pushdown should unify the compile API
URL: https://github.com/apache/spark/pull/37047
--
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 #37047: [SPARK-39627][SQL] DS V2 pushdown should unify the compile API
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37047:
URL: https://github.com/apache/spark/pull/37047#discussion_r913552918
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala:
##########
@@ -253,6 +253,19 @@ abstract class JdbcDialect extends Serializable with Logging{
}
}
+ override def visitAggregateFunction(
+ funcName: String, isDistinct: Boolean, inputs: Array[String]): String = {
+ if (isSupportedFunction(funcName)) {
+ super.visitAggregateFunction(
+ dialectFunctionName(funcName).getOrElse(funcName), isDistinct, inputs)
+ } else {
+ throw new UnsupportedOperationException(
+ s"${this.getClass.getSimpleName} does not support aggregate function: $funcName");
+ }
+ }
+
+ protected def dialectFunctionName(funcName: String): Option[String] = None
Review Comment:
shall we apply this in `visitSQLFunction`?
--
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 #37047: [SPARK-39627][SQL] DS V2 pushdown should unify the compile API
Posted by GitBox <gi...@apache.org>.
beliefer commented on code in PR #37047:
URL: https://github.com/apache/spark/pull/37047#discussion_r913574677
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala:
##########
@@ -253,6 +253,19 @@ abstract class JdbcDialect extends Serializable with Logging{
}
}
+ override def visitAggregateFunction(
+ funcName: String, isDistinct: Boolean, inputs: Array[String]): String = {
+ if (isSupportedFunction(funcName)) {
+ super.visitAggregateFunction(
+ dialectFunctionName(funcName).getOrElse(funcName), isDistinct, inputs)
+ } else {
+ throw new UnsupportedOperationException(
+ s"${this.getClass.getSimpleName} does not support aggregate function: $funcName");
+ }
+ }
+
+ protected def dialectFunctionName(funcName: String): Option[String] = None
Review Comment:
OK
--
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 #37047: [SPARK-39627][SQL] DS V2 pushdown should unify the compile API
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37047:
URL: https://github.com/apache/spark/pull/37047#discussion_r913427118
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java:
##########
@@ -46,6 +52,28 @@ public String build(Expression expr) {
} else if (expr instanceof Cast) {
Cast cast = (Cast) expr;
return visitCast(build(cast.expression()), cast.dataType());
+ } else if (expr instanceof Min) {
Review Comment:
can we move them to near `else if (expr instanceof GeneralAggregateFunc)`?
--
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 #37047: [SPARK-39627][SQL] DS V2 pushdown should unify the compile API
Posted by GitBox <gi...@apache.org>.
beliefer commented on code in PR #37047:
URL: https://github.com/apache/spark/pull/37047#discussion_r913514346
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/DB2Dialect.scala:
##########
@@ -31,34 +33,43 @@ private object DB2Dialect extends JdbcDialect {
url.toLowerCase(Locale.ROOT).startsWith("jdbc:db2")
// See https://www.ibm.com/docs/en/db2/11.5?topic=functions-aggregate
- override def compileAggregate(aggFunction: AggregateFunc): Option[String] = {
- super.compileAggregate(aggFunction).orElse(
- aggFunction match {
- case f: GeneralAggregateFunc if f.name() == "VAR_POP" =>
- assert(f.children().length == 1)
- val distinct = if (f.isDistinct) "DISTINCT " else ""
- Some(s"VARIANCE($distinct${f.children().head})")
- case f: GeneralAggregateFunc if f.name() == "VAR_SAMP" =>
- assert(f.children().length == 1)
- val distinct = if (f.isDistinct) "DISTINCT " else ""
- Some(s"VARIANCE_SAMP($distinct${f.children().head})")
- case f: GeneralAggregateFunc if f.name() == "STDDEV_POP" =>
- assert(f.children().length == 1)
- val distinct = if (f.isDistinct) "DISTINCT " else ""
- Some(s"STDDEV($distinct${f.children().head})")
- case f: GeneralAggregateFunc if f.name() == "STDDEV_SAMP" =>
- assert(f.children().length == 1)
- val distinct = if (f.isDistinct) "DISTINCT " else ""
- Some(s"STDDEV_SAMP($distinct${f.children().head})")
- case f: GeneralAggregateFunc if f.name() == "COVAR_POP" && f.isDistinct == false =>
- assert(f.children().length == 2)
- Some(s"COVARIANCE(${f.children().head}, ${f.children().last})")
- case f: GeneralAggregateFunc if f.name() == "COVAR_SAMP" && f.isDistinct == false =>
- assert(f.children().length == 2)
- Some(s"COVARIANCE_SAMP(${f.children().head}, ${f.children().last})")
- case _ => None
+ private val supportedAggregateFunctions =
+ Set("VAR_POP", "VAR_SAMP", "STDDEV_POP", "STDDEV_SAMP", "COVAR_POP", "COVAR_SAMP")
+ private val supportedFunctions = supportedAggregateFunctions
+
+ override def isSupportedFunction(funcName: String): Boolean =
+ supportedFunctions.contains(funcName)
+
+ class DB2SQLBuilder extends JDBCSQLBuilder {
Review Comment:
`JDBCSQLBuilder` already check `supportedFunctions`.
--
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 pull request #37047: [SPARK-39627][SQL] DS V2 pushdown should unify the compile API
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on PR #37047:
URL: https://github.com/apache/spark/pull/37047#issuecomment-1178507002
it has conflicts now...
--
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 #37047: [SPARK-39627][SQL] DS V2 pushdown should unify the compile API
Posted by GitBox <gi...@apache.org>.
beliefer commented on PR #37047:
URL: https://github.com/apache/spark/pull/37047#issuecomment-1178537563
> it has conflicts now...
I see.
--
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 #37047: [SPARK-39627][SQL] JDBC V2 pushdown should unify the compile API
Posted by GitBox <gi...@apache.org>.
beliefer commented on PR #37047:
URL: https://github.com/apache/spark/pull/37047#issuecomment-1178661116
@cloud-fan Thank you very much !
--
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