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/02/12 02:23:42 UTC
[GitHub] [spark] beliefer opened a new pull request #35494: [SPARK-37960][SQL][FOLLOWUP] Reactor framework so as JDBC dialect could compile expression by self way
beliefer opened a new pull request #35494:
URL: https://github.com/apache/spark/pull/35494
### What changes were proposed in this pull request?
https://github.com/apache/spark/pull/35248 provides a new framework to represent catalyst expressions in DS V2 APIs.
Because the framework translate all catalyst expressions to a unified SQL string and cannot keep compatibility between different JDBC database, the framework works not good.
This PR reactor the framework so as JDBC dialect could compile expression by self way.
First, The framework translate catalyst expressions to DS V2 expression.
Second, The JDBC dialect could compile DS V2 expression to different SQL syntax.
### Why are the changes needed?
Make the framework be more common use.
### Does this PR introduce _any_ user-facing change?
'No'.
The feature is not released.
### How was this patch tested?
Exists 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] beliefer commented on pull request #35494: [SPARK-37960][SQL][FOLLOWUP] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on pull request #35494:
URL: https://github.com/apache/spark/pull/35494#issuecomment-1037039127
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] HyukjinKwon commented on a change in pull request #35494: [SPARK-37960][SQL][FOLLOWUP] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r805224638
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralSQLExpression.java
##########
@@ -22,20 +22,34 @@
import org.apache.spark.annotation.Evolving;
/**
- * The general SQL string corresponding to expression.
+ * The general V2 expression corresponding to V1 expression.
*
* @since 3.3.0
*/
@Evolving
public class GeneralSQLExpression implements Expression, Serializable {
+ private String name;
+ private Expression[] children;
Review comment:
The indentation should be 2-spaced
--
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r811617740
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java
##########
@@ -0,0 +1,186 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.expressions;
+
+import java.io.Serializable;
+
+import org.apache.spark.annotation.Evolving;
+
+/**
+ * The general representation of SQL scalar expressions, which contains the upper-cased
+ * expression name and all the children expressions.
+ * <p>
+ * The currently supported expressions:
+ * <table border="1">
+ * <tr>
+ * <th>Expression name</th>
+ * <th>SQL scalar expression</th>
+ * <th>Since version</th>
+ * </tr>
+ * <tr>
+ * <td>IS_NULL</td>
+ * <td><pre>expr IS NULL</pre></td>
+ * <td>3.3.0</td>
+ * </tr>
+ * <tr>
+ * <td>IS_NOT_NULL</td>
+ * <td><pre>expr IS NOT NULL</pre></td>
+ * <td>3.3.0</td>
+ * </tr>
+ * <tr>
+ * <td>=</td>
+ * <td><pre>expr1 = expr2</pre></td>
+ * <td>3.3.0</td>
+ * </tr>
+ * <tr>
+ * <td>!=</td>
+ * <td><pre>expr1 != expr2</pre></td>
+ * <td>3.3.0</td>
+ * </tr>
+ * <tr>
+ * <td><=></td>
+ * <td><pre>expr1 <=> expr2</pre></td>
+ * <td>3.3.0</td>
+ * </tr>
+ * <tr>
+ * <td><</td>
+ * <td><pre>expr1 < expr2</pre></td>
+ * <td>3.3.0</td>
+ * </tr>
+ * <tr>
+ * <td><=</td>
+ * <td><pre>expr1 <= expr2</pre></td>
+ * <td>3.3.0</td>
+ * </tr>
+ * <tr>
+ * <td>></td>
+ * <td><pre>expr1 > expr2</pre></td>
+ * <td>3.3.0</td>
+ * </tr>
+ * <tr>
+ * <td>>=</td>
+ * <td><pre>expr1 >= expr2</pre></td>
+ * <td>3.3.0</td>
+ * </tr>
+ * <tr>
+ * <td>+</td>
+ * <td><pre>expr1 + expr2</pre></td>
Review comment:
Now, this comes to the tricky part. If ANSI mode is off, `expr1 + expr2` has a different behavior in Spark compared to other databases, because it won't fail for overflow.
One simple fix is: We don't translate math operations to v2 expression if ANSI mode is off. Or we can have a new expression name for ANSI-off add.
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r814554462
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala
##########
@@ -0,0 +1,93 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.expressions.{Add, Attribute, BinaryArithmetic, BinaryOperator, BitwiseAnd, BitwiseOr, BitwiseXor, CaseWhen, Divide, EqualTo, Expression, IntegralDivide, IsNotNull, IsNull, Literal, Multiply, Not, Pmod, Remainder, Subtract}
+import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference, GeneralScalarExpression, LiteralValue}
+
+/**
+ * The builder to generate V2 expressions from catalyst expressions.
+ */
+class V2ExpressionBuilder(e: Expression) {
+
+ def build(): Option[V2Expression] = generateExpression(e)
+
+ private def buildEnabled(b: BinaryArithmetic) = b match {
+ case add: Add if add.failOnError => true
Review comment:
Good idea.
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r816406684
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java
##########
@@ -0,0 +1,133 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.util;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.spark.sql.connector.expressions.Expression;
+import org.apache.spark.sql.connector.expressions.FieldReference;
+import org.apache.spark.sql.connector.expressions.GeneralScalarExpression;
+import org.apache.spark.sql.connector.expressions.LiteralValue;
+
+/**
+ * The builder to generate SQL from V2 expressions.
+ */
+public class V2ExpressionSQLBuilder {
+ public String build(Expression expr) {
+ if (expr instanceof LiteralValue) {
+ return visitLiteral((LiteralValue) expr);
+ } else if (expr instanceof FieldReference) {
+ return visitFieldReference((FieldReference) expr);
+ } else if (expr instanceof GeneralScalarExpression) {
+ GeneralScalarExpression e = (GeneralScalarExpression) expr;
+ String name = e.name();
+ switch (name) {
+ case "IS_NULL":
+ return visitIsNull(build(e.children()[0]));
+ case "IS_NOT_NULL":
+ return visitIsNotNull(build(e.children()[0]));
+ case "=":
+ case "!=":
+ case "<=>":
+ case "<":
+ case "<=":
+ case ">":
+ case ">=":
+ case "+":
+ case "-":
+ case "*":
+ case "/":
+ case "%":
+ case "&&":
+ case "||":
+ case "AND":
+ case "OR":
+ case "&":
+ case "|":
+ case "^":
+ return visitBinaryOperation(name, build(e.children()[0]), build(e.children()[1]));
+ case "NOT":
+ return visitNot(build(e.children()[0]));
+ case "CASE_WHEN":
+ List<String> children = new ArrayList<>();
+ for (Expression child : e.children()) {
+ children.add(build(child));
+ }
+ return visitCaseWhen(children.toArray(new String[e.children().length]));
+ // TODO supports other expressions
Review comment:
Let's update `canTranslate` to exclude `IntegralDivide`.
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r814530759
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java
##########
@@ -0,0 +1,86 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.expressions;
+
+import java.io.Serializable;
+import java.util.Arrays;
+
+import org.apache.spark.annotation.Evolving;
+import org.apache.spark.sql.connector.util.V2ExpressionSQLBuilder;
+
+// scalastyle:off line.size.limit
+/**
+ * The general representation of SQL scalar expressions, which contains the upper-cased
+ * expression name and all the children expressions.
+ * <p>
+ * The currently supported SQL aggregate functions:
Review comment:
```suggestion
* The currently supported SQL scalar expressions:
```
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r818647090
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java
##########
@@ -0,0 +1,201 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.expressions;
+
+import java.io.Serializable;
+import java.util.Arrays;
+
+import org.apache.spark.annotation.Evolving;
+import org.apache.spark.sql.connector.util.V2ExpressionSQLBuilder;
+
+// scalastyle:off line.size.limit
+/**
+ * The general representation of SQL scalar expressions, which contains the upper-cased
+ * expression name and all the children expressions.
+ * <p>
+ * The currently supported SQL scalar expressions:
+ * <ol>
+ * <li>Name: IS_NULL
+ * <ul>
+ * <li>SQL semantic: `expr IS NULL`</li>
Review comment:
I think we need to use `<code>expr IS NULL</code>`
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r816406684
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java
##########
@@ -0,0 +1,133 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.util;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.spark.sql.connector.expressions.Expression;
+import org.apache.spark.sql.connector.expressions.FieldReference;
+import org.apache.spark.sql.connector.expressions.GeneralScalarExpression;
+import org.apache.spark.sql.connector.expressions.LiteralValue;
+
+/**
+ * The builder to generate SQL from V2 expressions.
+ */
+public class V2ExpressionSQLBuilder {
+ public String build(Expression expr) {
+ if (expr instanceof LiteralValue) {
+ return visitLiteral((LiteralValue) expr);
+ } else if (expr instanceof FieldReference) {
+ return visitFieldReference((FieldReference) expr);
+ } else if (expr instanceof GeneralScalarExpression) {
+ GeneralScalarExpression e = (GeneralScalarExpression) expr;
+ String name = e.name();
+ switch (name) {
+ case "IS_NULL":
+ return visitIsNull(build(e.children()[0]));
+ case "IS_NOT_NULL":
+ return visitIsNotNull(build(e.children()[0]));
+ case "=":
+ case "!=":
+ case "<=>":
+ case "<":
+ case "<=":
+ case ">":
+ case ">=":
+ case "+":
+ case "-":
+ case "*":
+ case "/":
+ case "%":
+ case "&&":
+ case "||":
+ case "AND":
+ case "OR":
+ case "&":
+ case "|":
+ case "^":
+ return visitBinaryOperation(name, build(e.children()[0]), build(e.children()[1]));
+ case "NOT":
+ return visitNot(build(e.children()[0]));
+ case "CASE_WHEN":
+ List<String> children = new ArrayList<>();
+ for (Expression child : e.children()) {
+ children.add(build(child));
+ }
+ return visitCaseWhen(children.toArray(new String[e.children().length]));
+ // TODO supports other expressions
Review comment:
I update `canTranslate` to exclude `IntegralDivide`.
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r816387816
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java
##########
@@ -0,0 +1,213 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.expressions;
+
+import java.io.Serializable;
+import java.util.Arrays;
+
+import org.apache.spark.annotation.Evolving;
+import org.apache.spark.sql.connector.util.V2ExpressionSQLBuilder;
+
+// scalastyle:off line.size.limit
+/**
+ * The general representation of SQL scalar expressions, which contains the upper-cased
+ * expression name and all the children expressions.
+ * <p>
+ * The currently supported SQL scalar expressions:
+ * <ol>
+ * <li>Name: IS_NULL
+ * <ul>
+ * <li>SQL semantic: `expr IS NULL`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: IS_NOT_NULL
+ * <ul>
+ * <li>SQL semantic: `expr IS NOT NULL`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: =
+ * <ul>
+ * <li>SQL semantic: `expr1 = expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: !=
+ * <ul>
+ * <li>SQL semantic: `expr1 != expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <>
+ * <ul>
+ * <li>SQL semantic: `expr1 <> expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <=>
+ * <ul>
+ * <li>SQL semantic: `expr1 <=> expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <
+ * <ul>
+ * <li>SQL semantic: `expr1 < expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <=
+ * <ul>
+ * <li>SQL semantic: `expr1 <= expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: >
+ * <ul>
+ * <li>SQL semantic: `expr1 > expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: >=
+ * <ul>
+ * <li>SQL semantic: `expr1 >= expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: +
+ * <ul>
+ * <li>SQL semantic: `expr1 + expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: -
+ * <ul>
+ * <li>SQL semantic: `expr1 - expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: *
+ * <ul>
+ * <li>SQL semantic: `expr1 * expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: /
+ * <ul>
+ * <li>SQL semantic: `expr1 / expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: %
+ * <ul>
+ * <li>SQL semantic: `expr1 % expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: pmod
Review comment:
It seems they are not.
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r818357421
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/jdbc/H2Dialect.scala
##########
@@ -22,12 +22,33 @@ import java.util.Locale
import org.apache.spark.sql.AnalysisException
import org.apache.spark.sql.catalyst.analysis.{NoSuchNamespaceException, NoSuchTableException, TableAlreadyExistsException}
+import org.apache.spark.sql.connector.expressions.{Expression, FieldReference}
import org.apache.spark.sql.connector.expressions.aggregate.{AggregateFunc, GeneralAggregateFunc}
+import org.apache.spark.sql.connector.util.V2ExpressionSQLBuilder
private object H2Dialect extends JdbcDialect {
override def canHandle(url: String): Boolean =
url.toLowerCase(Locale.ROOT).startsWith("jdbc:h2")
+ class H2SQLBuilder extends V2ExpressionSQLBuilder {
+ override def visitFieldReference(fieldRef: FieldReference): String = {
+ if (fieldRef.fieldNames().length != 1) {
+ throw new IllegalArgumentException(
+ "FieldReference with field name has multiple or zero parts unsupported: " + fieldRef);
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] HyukjinKwon commented on a change in pull request #35494: [SPARK-37960][SQL][FOLLOWUP] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r805224276
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala
##########
@@ -194,6 +194,66 @@ abstract class JdbcDialect extends Serializable with Logging{
case _ => value
}
+ protected final val BINARY_COMPARISON = Set("=", "!=", "<=>", "<", "<=", ">", ">=",
+ "+", "-", "*", "/", "%", "&&", "||", "AND", "OR", "&", "|", "^")
+
+ /**
+ * Converts V2 expression to String representing a SQL expression.
+ * @param expr The V2 expression to be converted.
+ * @return Converted value.
+ */
+ @Since("3.3.0")
+ def compileExpression(expr: Expression): Option[String] = expr match {
+ case l @ LiteralValue(_, _) => Some(l.toString)
Review comment:
should avoid `l` https://github.com/databricks/scala-style-guide#variable-naming-convention as a variable name.
--
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] HyukjinKwon commented on a change in pull request #35494: [SPARK-37960][SQL][FOLLOWUP] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r805224161
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralSQLExpression.java
##########
@@ -22,20 +22,34 @@
import org.apache.spark.annotation.Evolving;
/**
- * The general SQL string corresponding to expression.
+ * The general V2 expression corresponding to V1 expression.
*
* @since 3.3.0
*/
@Evolving
public class GeneralSQLExpression implements Expression, Serializable {
Review comment:
Can we rename it something like `SQLStringHolder`, `SQLStringRepresentation` and `SQLString`?
--
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 #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #35494:
URL: https://github.com/apache/spark/pull/35494#issuecomment-1044616306
Instead of adding one v2 expression class for each catalyst expression, I agree it's much simpler to have a general v2 expression class that is a tree structure.
However, we must clearly document the name for each supported v2 expression, similar to `GeneralAggregateFunc`.
For the naming, maybe `GeneralScalarExpression` to differentiate from `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] cloud-fan commented on a change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r811146470
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java
##########
@@ -22,20 +22,46 @@
import org.apache.spark.annotation.Evolving;
/**
- * The general SQL string corresponding to expression.
+ * The general V2 expression corresponding to V1 expression.
+ * <p>
+ * The currently supported expression:
+ * <ol>
+ * <li><pre>CASE WHEN expr1 THEN expr2 [WHEN expr3 THEN expr4]* [ELSE expr5] END</pre>
+ * Since 3.3.0</li>
+ * </ol>
*
* @since 3.3.0
*/
@Evolving
-public class GeneralSQLExpression implements Expression, Serializable {
- private String sql;
+public class GeneralScalarExpression implements Expression, Serializable {
+ private String name;
+ private Expression[] children;
+ private String sql;
Review comment:
why do we need this field?
--
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r811620966
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java
##########
@@ -0,0 +1,124 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.util;
+
+import org.apache.spark.sql.connector.expressions.Expression;
+import org.apache.spark.sql.connector.expressions.FieldReference;
+import org.apache.spark.sql.connector.expressions.GeneralScalarExpression;
+import org.apache.spark.sql.connector.expressions.LiteralValue;
+import org.apache.spark.sql.errors.QueryExecutionErrors;
+
+/**
+ * The builder to generate SQL from V2 expressions.
+ */
+public class V2ExpressionSQLBuilder {
+ public String build(Expression expr) throws Throwable {
+ if (expr instanceof LiteralValue) {
+ return visitLiteral((LiteralValue) expr);
+ } else if (expr instanceof FieldReference) {
+ return visitFieldReference((FieldReference) expr);
+ } else if (expr instanceof GeneralScalarExpression) {
+ GeneralScalarExpression e = (GeneralScalarExpression) expr;
+ String name = e.name();
+ switch (name) {
+ case "IS_NULL":
+ return visitIsNull(build(e.children()[0]));
+ case "IS_NOT_NULL":
+ return visitIsNotNull(build(e.children()[0]));
+ case "=":
+ case "!=":
+ case "<=>":
+ case "<":
+ case "<=":
+ case ">":
+ case ">=":
+ case "+":
+ case "-":
+ case "*":
+ case "/":
+ case "%":
+ case "&&":
+ case "||":
+ case "AND":
+ case "OR":
+ case "&":
+ case "|":
+ case "^":
+ return visitBinaryOperation(name, build(e.children()[0]), build(e.children()[1]));
+ case "NOT":
+ return visitNot(build(e.children()[0]));
+ case "CASE_WHEN":
+ return visitCaseWhen(e);
Review comment:
shall we pass the children sql string? `visitCaseWhen(e.children.map(build)) // please use the proper jave 8 lambda syntax`
--
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r811620097
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java
##########
@@ -0,0 +1,124 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.util;
+
+import org.apache.spark.sql.connector.expressions.Expression;
+import org.apache.spark.sql.connector.expressions.FieldReference;
+import org.apache.spark.sql.connector.expressions.GeneralScalarExpression;
+import org.apache.spark.sql.connector.expressions.LiteralValue;
+import org.apache.spark.sql.errors.QueryExecutionErrors;
+
+/**
+ * The builder to generate SQL from V2 expressions.
+ */
+public class V2ExpressionSQLBuilder {
+ public String build(Expression expr) throws Throwable {
+ if (expr instanceof LiteralValue) {
+ return visitLiteral((LiteralValue) expr);
+ } else if (expr instanceof FieldReference) {
+ return visitFieldReference((FieldReference) expr);
+ } else if (expr instanceof GeneralScalarExpression) {
+ GeneralScalarExpression e = (GeneralScalarExpression) expr;
Review comment:
2 spaces indentation
--
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r813524686
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java
##########
@@ -20,22 +20,42 @@
import java.io.Serializable;
import org.apache.spark.annotation.Evolving;
+import org.apache.spark.sql.connector.util.V2ExpressionSQLBuilder;
/**
- * The general SQL string corresponding to expression.
+ * The general representation of SQL scalar expressions, which contains the upper-cased
+ * expression name and all the children expressions.
*
Review comment:
It seems JDK11 and JKD17 maven build exceeds 5 hours. I just check the issue.
I will restore the classdoc later.
--
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r807440130
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceStrategy.scala
##########
@@ -41,7 +41,7 @@ import org.apache.spark.sql.catalyst.streaming.StreamingRelationV2
import org.apache.spark.sql.catalyst.util.ExpressionSQLBuilder
import org.apache.spark.sql.connector.catalog.SupportsRead
import org.apache.spark.sql.connector.catalog.TableCapability._
-import org.apache.spark.sql.connector.expressions.{Expression => ExpressionV2, FieldReference, GeneralSQLExpression, NullOrdering, SortDirection, SortOrder => SortOrderV2, SortValue}
+import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference, NullOrdering, SortDirection, SortOrder => V2SortOrder, SortValue}
Review comment:
Because other place use the pattern V2*, just unify the style.
--
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r813520243
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java
##########
@@ -20,22 +20,42 @@
import java.io.Serializable;
import org.apache.spark.annotation.Evolving;
+import org.apache.spark.sql.connector.util.V2ExpressionSQLBuilder;
/**
- * The general SQL string corresponding to expression.
+ * The general representation of SQL scalar expressions, which contains the upper-cased
+ * expression name and all the children expressions.
*
* @since 3.3.0
*/
@Evolving
-public class GeneralSQLExpression implements Expression, Serializable {
- private String sql;
+public class GeneralScalarExpression implements Expression, Serializable {
+ private String name;
+ private Expression[] children;
+ private String sql;
- public GeneralSQLExpression(String sql) {
- this.sql = sql;
- }
+ public GeneralScalarExpression(String name, Expression[] children) {
+ this.name = name;
+ this.children = children;
+ }
- public String sql() { return sql; }
+ public String name() { return name; }
+ public Expression[] children() { return children; }
- @Override
- public String toString() { return sql; }
+ @Override
+ public String toString() {
+ if (sql == null) {
Review comment:
is it a cache? seems not needed as `toString` is not called frequently.
--
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r811620006
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java
##########
@@ -0,0 +1,124 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.util;
+
+import org.apache.spark.sql.connector.expressions.Expression;
+import org.apache.spark.sql.connector.expressions.FieldReference;
+import org.apache.spark.sql.connector.expressions.GeneralScalarExpression;
+import org.apache.spark.sql.connector.expressions.LiteralValue;
+import org.apache.spark.sql.errors.QueryExecutionErrors;
+
+/**
+ * The builder to generate SQL from V2 expressions.
+ */
+public class V2ExpressionSQLBuilder {
+ public String build(Expression expr) throws Throwable {
+ if (expr instanceof LiteralValue) {
+ return visitLiteral((LiteralValue) expr);
+ } else if (expr instanceof FieldReference) {
+ return visitFieldReference((FieldReference) expr);
+ } else if (expr instanceof GeneralScalarExpression) {
+ GeneralScalarExpression e = (GeneralScalarExpression) expr;
+ String name = e.name();
+ switch (name) {
+ case "IS_NULL":
+ return visitIsNull(build(e.children()[0]));
+ case "IS_NOT_NULL":
+ return visitIsNotNull(build(e.children()[0]));
+ case "=":
+ case "!=":
+ case "<=>":
+ case "<":
+ case "<=":
+ case ">":
+ case ">=":
+ case "+":
+ case "-":
+ case "*":
+ case "/":
+ case "%":
+ case "&&":
+ case "||":
+ case "AND":
+ case "OR":
+ case "&":
+ case "|":
+ case "^":
+ return visitBinaryOperation(name, build(e.children()[0]), build(e.children()[1]));
+ case "NOT":
+ return visitNot(build(e.children()[0]));
+ case "CASE_WHEN":
+ return visitCaseWhen(e);
+ // TODO supports other expressions
+ default:
+ return visitUnexpectedExpr(expr);
+ }
+ } else {
+ return visitUnexpectedExpr(expr);
Review comment:
```suggestion
return visitUnexpectedExpr(expr);
```
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r815248265
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala
##########
@@ -0,0 +1,93 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.expressions.{Add, Attribute, BinaryArithmetic, BinaryOperator, BitwiseAnd, BitwiseOr, BitwiseXor, CaseWhen, Divide, EqualTo, Expression, IntegralDivide, IsNotNull, IsNull, Literal, Multiply, Not, Pmod, Remainder, Subtract}
+import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference, GeneralScalarExpression, LiteralValue}
+
+/**
+ * The builder to generate V2 expressions from catalyst expressions.
+ */
+class V2ExpressionBuilder(e: Expression) {
+
+ def build(): Option[V2Expression] = generateExpression(e)
+
+ private def buildEnabled(b: BinaryArithmetic) = b match {
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r818295428
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java
##########
@@ -0,0 +1,201 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.expressions;
+
+import java.io.Serializable;
+import java.util.Arrays;
+
+import org.apache.spark.annotation.Evolving;
+import org.apache.spark.sql.connector.util.V2ExpressionSQLBuilder;
+
+// scalastyle:off line.size.limit
+/**
+ * The general representation of SQL scalar expressions, which contains the upper-cased
+ * expression name and all the children expressions.
+ * <p>
+ * The currently supported SQL scalar expressions:
+ * <ol>
+ * <li>Name: IS_NULL
+ * <ul>
+ * <li>SQL semantic: `expr IS NULL`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: IS_NOT_NULL
+ * <ul>
+ * <li>SQL semantic: `expr IS NOT NULL`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: =
+ * <ul>
+ * <li>SQL semantic: `expr1 = expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: !=
+ * <ul>
+ * <li>SQL semantic: `expr1 != expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <>
+ * <ul>
+ * <li>SQL semantic: `expr1 <> expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <=>
+ * <ul>
+ * <li>SQL semantic: `expr1 <=> expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <
+ * <ul>
+ * <li>SQL semantic: `expr1 < expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <=
+ * <ul>
+ * <li>SQL semantic: `expr1 <= expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: >
+ * <ul>
+ * <li>SQL semantic: `expr1 > expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: >=
+ * <ul>
+ * <li>SQL semantic: `expr1 >= expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: +
+ * <ul>
+ * <li>SQL semantic: `expr1 + expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: -
Review comment:
I used `UNARY_ARITHMETIC(-)` as the name of `UnaryMinus`.
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r818337308
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java
##########
@@ -0,0 +1,201 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.expressions;
+
+import java.io.Serializable;
+import java.util.Arrays;
+
+import org.apache.spark.annotation.Evolving;
+import org.apache.spark.sql.connector.util.V2ExpressionSQLBuilder;
+
+// scalastyle:off line.size.limit
+/**
+ * The general representation of SQL scalar expressions, which contains the upper-cased
+ * expression name and all the children expressions.
+ * <p>
+ * The currently supported SQL scalar expressions:
+ * <ol>
+ * <li>Name: IS_NULL
+ * <ul>
+ * <li>SQL semantic: `expr IS NULL`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: IS_NOT_NULL
+ * <ul>
+ * <li>SQL semantic: `expr IS NOT NULL`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: =
+ * <ul>
+ * <li>SQL semantic: `expr1 = expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: !=
+ * <ul>
+ * <li>SQL semantic: `expr1 != expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <>
+ * <ul>
+ * <li>SQL semantic: `expr1 <> expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <=>
+ * <ul>
+ * <li>SQL semantic: `expr1 <=> expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <
+ * <ul>
+ * <li>SQL semantic: `expr1 < expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <=
+ * <ul>
+ * <li>SQL semantic: `expr1 <= expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: >
+ * <ul>
+ * <li>SQL semantic: `expr1 > expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: >=
+ * <ul>
+ * <li>SQL semantic: `expr1 >= expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: +
+ * <ul>
+ * <li>SQL semantic: `expr1 + expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: -
Review comment:
I think we can still use the name `-`, and users can look at the number of children to know which `-` it is.
```
SQL semantic: `expr1 - expr2` or `-expr1`
```
--
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 #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #35494:
URL: https://github.com/apache/spark/pull/35494
--
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] HyukjinKwon commented on pull request #35494: [SPARK-37960][SQL][FOLLOWUP] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #35494:
URL: https://github.com/apache/spark/pull/35494#issuecomment-1037657225
@beliefer this PR adds a new API. Can we file a new JIRA?
--
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r810430517
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceStrategy.scala
##########
@@ -41,7 +41,7 @@ import org.apache.spark.sql.catalyst.streaming.StreamingRelationV2
import org.apache.spark.sql.catalyst.util.ExpressionSQLBuilder
import org.apache.spark.sql.connector.catalog.SupportsRead
import org.apache.spark.sql.connector.catalog.TableCapability._
-import org.apache.spark.sql.connector.expressions.{Expression => ExpressionV2, FieldReference, GeneralSQLExpression, NullOrdering, SortDirection, SortOrder => SortOrderV2, SortValue}
+import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference, NullOrdering, SortDirection, SortOrder => V2SortOrder, SortValue}
Review comment:
Ref:
https://github.com/apache/spark/blob/439975590cf4f21c2a548a2ac6231eb234e1a2f3/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala#L46
https://github.com/apache/spark/blob/439975590cf4f21c2a548a2ac6231eb234e1a2f3/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/V2ExpressionUtils.scala#L23
Only `DataSourceStrategy` uses the name `ExpressionV2`.
--
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] HyukjinKwon commented on pull request #35494: [SPARK-37960][SQL][FOLLOWUP] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #35494:
URL: https://github.com/apache/spark/pull/35494#issuecomment-1037657225
@beliefer this PR adds a new API. Can we file a new JIRA?
--
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] xkrogen commented on a change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
xkrogen commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r807233625
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceStrategy.scala
##########
@@ -41,7 +41,7 @@ import org.apache.spark.sql.catalyst.streaming.StreamingRelationV2
import org.apache.spark.sql.catalyst.util.ExpressionSQLBuilder
import org.apache.spark.sql.connector.catalog.SupportsRead
import org.apache.spark.sql.connector.catalog.TableCapability._
-import org.apache.spark.sql.connector.expressions.{Expression => ExpressionV2, FieldReference, GeneralSQLExpression, NullOrdering, SortDirection, SortOrder => SortOrderV2, SortValue}
+import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference, NullOrdering, SortDirection, SortOrder => V2SortOrder, SortValue}
Review comment:
Why do we rename `ExpressionV2` to `V2Expression` (same for `SortOrder`)? IMO the old format is much easier to read and aligns more closely with other naming conventions like "DataSource V2"
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r814546368
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala
##########
@@ -0,0 +1,93 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.expressions.{Add, Attribute, BinaryArithmetic, BinaryOperator, BitwiseAnd, BitwiseOr, BitwiseXor, CaseWhen, Divide, EqualTo, Expression, IntegralDivide, IsNotNull, IsNull, Literal, Multiply, Not, Pmod, Remainder, Subtract}
+import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference, GeneralScalarExpression, LiteralValue}
+
+/**
+ * The builder to generate V2 expressions from catalyst expressions.
+ */
+class V2ExpressionBuilder(e: Expression) {
+
+ def build(): Option[V2Expression] = generateExpression(e)
+
+ private def buildEnabled(b: BinaryArithmetic) = b match {
+ case add: Add if add.failOnError => true
Review comment:
```suggestion
case add: Add => add.failOnError
```
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r814549510
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala
##########
@@ -194,6 +195,21 @@ abstract class JdbcDialect extends Serializable with Logging{
case _ => value
}
+ /**
+ * Converts V2 expression to String representing a SQL expression.
+ * @param expr The V2 expression to be converted.
+ * @return Converted value.
+ */
+ @Since("3.3.0")
+ def compileExpression(expr: Expression): Option[String] = {
+ val V2ExpressionSQLBuilder = new V2ExpressionSQLBuilder()
+ try {
+ Some(V2ExpressionSQLBuilder.build(expr))
Review comment:
Shall we have a JDBC v2 specific `V2ExpressionSQLBuilder` implementation? One requirement is that JDBC v2 should not support nested columns, so `visitFieldReference` should only work for single-part name.
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r816387816
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java
##########
@@ -0,0 +1,213 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.expressions;
+
+import java.io.Serializable;
+import java.util.Arrays;
+
+import org.apache.spark.annotation.Evolving;
+import org.apache.spark.sql.connector.util.V2ExpressionSQLBuilder;
+
+// scalastyle:off line.size.limit
+/**
+ * The general representation of SQL scalar expressions, which contains the upper-cased
+ * expression name and all the children expressions.
+ * <p>
+ * The currently supported SQL scalar expressions:
+ * <ol>
+ * <li>Name: IS_NULL
+ * <ul>
+ * <li>SQL semantic: `expr IS NULL`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: IS_NOT_NULL
+ * <ul>
+ * <li>SQL semantic: `expr IS NOT NULL`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: =
+ * <ul>
+ * <li>SQL semantic: `expr1 = expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: !=
+ * <ul>
+ * <li>SQL semantic: `expr1 != expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <>
+ * <ul>
+ * <li>SQL semantic: `expr1 <> expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <=>
+ * <ul>
+ * <li>SQL semantic: `expr1 <=> expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <
+ * <ul>
+ * <li>SQL semantic: `expr1 < expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <=
+ * <ul>
+ * <li>SQL semantic: `expr1 <= expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: >
+ * <ul>
+ * <li>SQL semantic: `expr1 > expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: >=
+ * <ul>
+ * <li>SQL semantic: `expr1 >= expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: +
+ * <ul>
+ * <li>SQL semantic: `expr1 + expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: -
+ * <ul>
+ * <li>SQL semantic: `expr1 - expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: *
+ * <ul>
+ * <li>SQL semantic: `expr1 * expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: /
+ * <ul>
+ * <li>SQL semantic: `expr1 / expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: %
+ * <ul>
+ * <li>SQL semantic: `expr1 % expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: pmod
Review comment:
pmod seems is not.
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r816418985
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala
##########
@@ -0,0 +1,93 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.expressions.{Add, Attribute, BinaryArithmetic, BinaryOperator, BitwiseAnd, BitwiseOr, BitwiseXor, CaseWhen, Divide, EqualTo, Expression, IntegralDivide, IsNotNull, IsNull, Literal, Multiply, Not, Pmod, Remainder, Subtract}
+import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference, GeneralScalarExpression, LiteralValue}
+
+/**
+ * The builder to generate V2 expressions from catalyst expressions.
+ */
+class V2ExpressionBuilder(e: Expression) {
+
+ def build(): Option[V2Expression] = generateExpression(e)
+
+ private def canTranslate(b: BinaryOperator) = b match {
+ case add: Add => add.failOnError
+ case sub: Subtract => sub.failOnError
+ case mul: Multiply => mul.failOnError
+ case div: Divide => div.failOnError
+ case intDiv: IntegralDivide => intDiv.failOnError
+ case r: Remainder => r.failOnError
+ case p: Pmod => p.failOnError
+ case _: BitwiseAnd => true
+ case _: BitwiseOr => true
+ case _: BitwiseXor => true
+ case _ => false
+ }
+
+ private def generateExpression(expr: Expression): Option[V2Expression] = expr match {
+ case Literal(value, dataType) => Some(LiteralValue(value, dataType))
+ case attr: Attribute => Some(FieldReference.column(quoteIfNeeded(attr.name)))
+ case IsNull(col) => generateExpression(col)
+ .map(c => new GeneralScalarExpression("IS_NULL", Array[V2Expression](c)))
+ case IsNotNull(col) => generateExpression(col)
+ .map(c => new GeneralScalarExpression("IS_NOT_NULL", Array[V2Expression](c)))
+ case b: BinaryOperator
+ if !b.isInstanceOf[BinaryArithmetic] || canTranslate(b) =>
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r815995258
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java
##########
@@ -0,0 +1,213 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.expressions;
+
+import java.io.Serializable;
+import java.util.Arrays;
+
+import org.apache.spark.annotation.Evolving;
+import org.apache.spark.sql.connector.util.V2ExpressionSQLBuilder;
+
+// scalastyle:off line.size.limit
+/**
+ * The general representation of SQL scalar expressions, which contains the upper-cased
+ * expression name and all the children expressions.
+ * <p>
+ * The currently supported SQL scalar expressions:
+ * <ol>
+ * <li>Name: IS_NULL
+ * <ul>
+ * <li>SQL semantic: `expr IS NULL`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: IS_NOT_NULL
+ * <ul>
+ * <li>SQL semantic: `expr IS NOT NULL`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: =
+ * <ul>
+ * <li>SQL semantic: `expr1 = expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: !=
+ * <ul>
+ * <li>SQL semantic: `expr1 != expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <>
+ * <ul>
+ * <li>SQL semantic: `expr1 <> expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <=>
+ * <ul>
+ * <li>SQL semantic: `expr1 <=> expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <
+ * <ul>
+ * <li>SQL semantic: `expr1 < expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <=
+ * <ul>
+ * <li>SQL semantic: `expr1 <= expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: >
+ * <ul>
+ * <li>SQL semantic: `expr1 > expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: >=
+ * <ul>
+ * <li>SQL semantic: `expr1 >= expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: +
+ * <ul>
+ * <li>SQL semantic: `expr1 + expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: -
+ * <ul>
+ * <li>SQL semantic: `expr1 - expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: *
+ * <ul>
+ * <li>SQL semantic: `expr1 * expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: /
+ * <ul>
+ * <li>SQL semantic: `expr1 / expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: %
+ * <ul>
+ * <li>SQL semantic: `expr1 % expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: pmod
Review comment:
can we do some research and verify if `%` and `pmod` are standard SQL?
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r818350141
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.expressions.{Add, Attribute, BinaryArithmetic, BinaryOperator, BitwiseNot, CaseWhen, Divide, EqualTo, Expression, IsNotNull, IsNull, Literal, Multiply, Not, Remainder, Subtract, UnaryMinus}
+import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference, GeneralScalarExpression, LiteralValue}
+
+/**
+ * The builder to generate V2 expressions from catalyst expressions.
+ */
+class V2ExpressionBuilder(e: Expression) {
+
+ def build(): Option[V2Expression] = generateExpression(e)
+
+ private def canTranslate(b: BinaryOperator) = b match {
+ case add: Add => add.failOnError
+ case sub: Subtract => sub.failOnError
+ case mul: Multiply => mul.failOnError
+ case div: Divide => div.failOnError
+ case r: Remainder => r.failOnError
+ case o: BinaryOperator => !o.isInstanceOf[BinaryArithmetic]
Review comment:
Does `BinaryArithmetic` decided by failOnError ?
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r818647659
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java
##########
@@ -0,0 +1,201 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.expressions;
+
+import java.io.Serializable;
+import java.util.Arrays;
+
+import org.apache.spark.annotation.Evolving;
+import org.apache.spark.sql.connector.util.V2ExpressionSQLBuilder;
+
+// scalastyle:off line.size.limit
+/**
+ * The general representation of SQL scalar expressions, which contains the upper-cased
+ * expression name and all the children expressions.
+ * <p>
+ * The currently supported SQL scalar expressions:
+ * <ol>
+ * <li>Name: IS_NULL
Review comment:
To make it pretty, we can wrap the name with <coode>: `<code>IS_NULL</code>`
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r816740040
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java
##########
@@ -0,0 +1,165 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.util;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.spark.sql.connector.expressions.Expression;
+import org.apache.spark.sql.connector.expressions.FieldReference;
+import org.apache.spark.sql.connector.expressions.GeneralScalarExpression;
+import org.apache.spark.sql.connector.expressions.LiteralValue;
+
+/**
+ * The builder to generate SQL from V2 expressions.
+ */
+public class V2ExpressionSQLBuilder {
+ public String build(Expression expr) {
+ if (expr instanceof LiteralValue) {
+ return visitLiteral((LiteralValue) expr);
+ } else if (expr instanceof FieldReference) {
+ return visitFieldReference((FieldReference) expr);
+ } else if (expr instanceof GeneralScalarExpression) {
+ GeneralScalarExpression e = (GeneralScalarExpression) expr;
+ String name = e.name();
+ switch (name) {
+ case "IS_NULL":
+ return visitIsNull(build(e.children()[0]));
+ case "IS_NOT_NULL":
+ return visitIsNotNull(build(e.children()[0]));
+ case "=":
+ case "!=":
+ case "<=>":
+ case "<":
+ case "<=":
+ case ">":
+ case ">=":
+ return visitBinaryComparison(name, build(e.children()[0]), build(e.children()[1]));
+ case "+":
+ case "-":
+ case "*":
+ case "/":
+ case "%":
+ return visitBinaryArithmetic(name, build(e.children()[0]), build(e.children()[1]));
+ case "AND":
+ return visitAnd(name, build(e.children()[0]), build(e.children()[1]));
+ case "OR":
+ return visitOr(name, build(e.children()[0]), build(e.children()[1]));
+ case "NOT":
+ return visitNot(build(e.children()[0]));
+ case "&":
+ return visitBitwiseAnd(name, build(e.children()[0]), build(e.children()[1]));
+ case "|":
+ return visitBitwiseOr(name, build(e.children()[0]), build(e.children()[1]));
+ case "^":
+ return visitBitwiseXor(name, build(e.children()[0]), build(e.children()[1]));
+ case "~":
+ return visitBitwiseNot(build(e.children()[0]));
Review comment:
We can call it unary arithematic
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r816738788
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java
##########
@@ -0,0 +1,201 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.expressions;
+
+import java.io.Serializable;
+import java.util.Arrays;
+
+import org.apache.spark.annotation.Evolving;
+import org.apache.spark.sql.connector.util.V2ExpressionSQLBuilder;
+
+// scalastyle:off line.size.limit
+/**
+ * The general representation of SQL scalar expressions, which contains the upper-cased
+ * expression name and all the children expressions.
+ * <p>
+ * The currently supported SQL scalar expressions:
+ * <ol>
+ * <li>Name: IS_NULL
+ * <ul>
+ * <li>SQL semantic: `expr IS NULL`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: IS_NOT_NULL
+ * <ul>
+ * <li>SQL semantic: `expr IS NOT NULL`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: =
+ * <ul>
+ * <li>SQL semantic: `expr1 = expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: !=
+ * <ul>
+ * <li>SQL semantic: `expr1 != expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <>
+ * <ul>
+ * <li>SQL semantic: `expr1 <> expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <=>
+ * <ul>
+ * <li>SQL semantic: `expr1 <=> expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <
+ * <ul>
+ * <li>SQL semantic: `expr1 < expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <=
+ * <ul>
+ * <li>SQL semantic: `expr1 <= expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: >
+ * <ul>
+ * <li>SQL semantic: `expr1 > expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: >=
+ * <ul>
+ * <li>SQL semantic: `expr1 >= expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: +
+ * <ul>
+ * <li>SQL semantic: `expr1 + expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: -
Review comment:
another tricky thing here is `UnaryMinus`. Its name is also `-` but it only has one child.
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r815999682
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java
##########
@@ -0,0 +1,133 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.util;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.spark.sql.connector.expressions.Expression;
+import org.apache.spark.sql.connector.expressions.FieldReference;
+import org.apache.spark.sql.connector.expressions.GeneralScalarExpression;
+import org.apache.spark.sql.connector.expressions.LiteralValue;
+
+/**
+ * The builder to generate SQL from V2 expressions.
+ */
+public class V2ExpressionSQLBuilder {
+ public String build(Expression expr) {
+ if (expr instanceof LiteralValue) {
+ return visitLiteral((LiteralValue) expr);
+ } else if (expr instanceof FieldReference) {
+ return visitFieldReference((FieldReference) expr);
+ } else if (expr instanceof GeneralScalarExpression) {
+ GeneralScalarExpression e = (GeneralScalarExpression) expr;
+ String name = e.name();
+ switch (name) {
+ case "IS_NULL":
+ return visitIsNull(build(e.children()[0]));
+ case "IS_NOT_NULL":
+ return visitIsNotNull(build(e.children()[0]));
+ case "=":
+ case "!=":
+ case "<=>":
+ case "<":
+ case "<=":
+ case ">":
+ case ">=":
+ case "+":
+ case "-":
+ case "*":
+ case "/":
+ case "%":
+ case "&&":
+ case "||":
Review comment:
This is `Concat`, which is not a `BinaryOperator`. I don't think we translate it 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 #35494: [SPARK-37960][SQL][FOLLOWUP] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on pull request #35494:
URL: https://github.com/apache/spark/pull/35494#issuecomment-1038539857
> @beliefer this PR adds a new API. Can we file a new JIRA?
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] HyukjinKwon commented on a change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r806386913
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala
##########
@@ -194,6 +194,66 @@ abstract class JdbcDialect extends Serializable with Logging{
case _ => value
}
+ protected final val BINARY_COMPARISON = Set("=", "!=", "<=>", "<", "<=", ">", ">=",
+ "+", "-", "*", "/", "%", "&&", "||", "AND", "OR", "&", "|", "^")
+
+ /**
+ * Converts V2 expression to String representing a SQL expression.
+ * @param expr The V2 expression to be converted.
+ * @return Converted value.
+ */
+ @Since("3.3.0")
+ def compileExpression(expr: Expression): Option[String] = expr match {
+ case l @ LiteralValue(_, _) => Some(l.toString)
Review comment:
Yeah. There was a try to repalce them all but it was rejected as it's too invasive. We should better avoid them anyway.
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/ExpressionSQLBuilder.scala
##########
@@ -67,3 +122,8 @@ class ExpressionSQLBuilder(e: Expression) {
case _ => None
}
}
+
+object GeneralExpression {
+ def apply(expressions: Array[V2Expression], name: String = "UNDEFINED"): GeneralSQLExpression =
+ new GeneralSQLExpression(name, expressions)
Review comment:
I meant that removing this `object`, and dealing with this default argument in the constructor of `GeneralExpression` at the Java file.
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralSQLExpression.java
##########
@@ -22,20 +22,34 @@
import org.apache.spark.annotation.Evolving;
/**
- * The general SQL string corresponding to expression.
+ * The general V2 expression corresponding to V1 expression.
Review comment:
I meant documenting that this API should be used in the source that can understand SQL in this Javadoc.
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralSQLExpression.java
##########
@@ -22,20 +22,34 @@
import org.apache.spark.annotation.Evolving;
/**
- * The general SQL string corresponding to expression.
+ * The general V2 expression corresponding to V1 expression.
Review comment:
I meant documenting (mentioning) that this API should be used in the source that can understand SQL in this Javadoc.
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralSQLExpression.java
##########
@@ -22,20 +22,34 @@
import org.apache.spark.annotation.Evolving;
/**
- * The general SQL string corresponding to expression.
+ * The general V2 expression corresponding to V1 expression.
*
* @since 3.3.0
*/
@Evolving
public class GeneralSQLExpression implements Expression, Serializable {
Review comment:
Do you meant that we will use this for all V2 expressions? I thought this is one of the optional APIs.
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralSQLExpression.java
##########
@@ -22,20 +22,34 @@
import org.apache.spark.annotation.Evolving;
/**
- * The general SQL string corresponding to expression.
+ * The general V2 expression corresponding to V1 expression.
*
* @since 3.3.0
*/
@Evolving
public class GeneralSQLExpression implements Expression, Serializable {
Review comment:
Do you mean that we will use this for all V2 expressions? I thought this is one of the optional APIs.
--
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] HyukjinKwon commented on a change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r806386913
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala
##########
@@ -194,6 +194,66 @@ abstract class JdbcDialect extends Serializable with Logging{
case _ => value
}
+ protected final val BINARY_COMPARISON = Set("=", "!=", "<=>", "<", "<=", ">", ">=",
+ "+", "-", "*", "/", "%", "&&", "||", "AND", "OR", "&", "|", "^")
+
+ /**
+ * Converts V2 expression to String representing a SQL expression.
+ * @param expr The V2 expression to be converted.
+ * @return Converted value.
+ */
+ @Since("3.3.0")
+ def compileExpression(expr: Expression): Option[String] = expr match {
+ case l @ LiteralValue(_, _) => Some(l.toString)
Review comment:
Yeah. There was a try to repalce them all but it was rejected as it's too invasive. We should better avoid them anyway.
--
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] HyukjinKwon commented on a change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r806387551
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralSQLExpression.java
##########
@@ -22,20 +22,34 @@
import org.apache.spark.annotation.Evolving;
/**
- * The general SQL string corresponding to expression.
+ * The general V2 expression corresponding to V1 expression.
Review comment:
I meant documenting (mentioning) that this API should be used in the source that can understand SQL in this Javadoc.
--
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] HyukjinKwon commented on a change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r806387551
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralSQLExpression.java
##########
@@ -22,20 +22,34 @@
import org.apache.spark.annotation.Evolving;
/**
- * The general SQL string corresponding to expression.
+ * The general V2 expression corresponding to V1 expression.
Review comment:
I meant documenting that this API should be used in the source that can understand SQL in this Javadoc.
--
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] HyukjinKwon commented on a change in pull request #35494: [SPARK-37960][SQL][FOLLOWUP] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r805224208
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala
##########
@@ -194,6 +194,66 @@ abstract class JdbcDialect extends Serializable with Logging{
case _ => value
}
+ protected final val BINARY_COMPARISON = Set("=", "!=", "<=>", "<", "<=", ">", ">=",
+ "+", "-", "*", "/", "%", "&&", "||", "AND", "OR", "&", "|", "^")
+
+ /**
+ * Converts V2 expression to String representing a SQL expression.
+ * @param expr The V2 expression to be converted.
+ * @return Converted value.
Review comment:
Can we describe which value it is after the conversion?
--
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r811180186
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala
##########
@@ -0,0 +1,124 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.expressions.{Attribute, BinaryOperator, CaseWhen, EqualTo, Expression, IsNotNull, IsNull, Literal, Not}
+import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference, GeneralScalarExpression, LiteralValue}
+
+/**
+ * The builder to generate V2 expressions from catalyst expressions.
+ */
+class V2ExpressionBuilder(e: Expression) {
+
+ def build(): Option[V2Expression] = {
+ val expression = generateExpression(e)
+ expression.foreach {
+ case generated: GeneralScalarExpression =>
+ generateSQL(e).foreach(sql => generated.setSql(sql))
+ case _ =>
+ }
+ expression
+ }
+
+ private def generateExpression(expr: Expression): Option[V2Expression] = expr match {
+ case Literal(value, dataType) => Some(LiteralValue(value, dataType))
+ case attr: Attribute => Some(FieldReference.column(quoteIfNeeded(attr.name)))
+ case IsNull(col) => generateExpression(col)
+ .map(c => new GeneralScalarExpression("IS NULL", Array[V2Expression](c)))
Review comment:
this looks weird as a "expression name". Maybe `IS_NULL` is better.
--
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r813521184
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java
##########
@@ -0,0 +1,129 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.util;
+
+import org.apache.spark.sql.connector.expressions.Expression;
+import org.apache.spark.sql.connector.expressions.FieldReference;
+import org.apache.spark.sql.connector.expressions.GeneralScalarExpression;
+import org.apache.spark.sql.connector.expressions.LiteralValue;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * The builder to generate SQL from V2 expressions.
+ */
+public class V2ExpressionSQLBuilder {
+ public String build(Expression expr) throws Throwable {
Review comment:
where do we throw error?
--
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r811181435
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala
##########
@@ -0,0 +1,124 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.expressions.{Attribute, BinaryOperator, CaseWhen, EqualTo, Expression, IsNotNull, IsNull, Literal, Not}
+import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference, GeneralScalarExpression, LiteralValue}
+
+/**
+ * The builder to generate V2 expressions from catalyst expressions.
+ */
+class V2ExpressionBuilder(e: Expression) {
+
+ def build(): Option[V2Expression] = {
+ val expression = generateExpression(e)
+ expression.foreach {
+ case generated: GeneralScalarExpression =>
+ generateSQL(e).foreach(sql => generated.setSql(sql))
+ case _ =>
+ }
+ expression
+ }
+
+ private def generateExpression(expr: Expression): Option[V2Expression] = expr match {
+ case Literal(value, dataType) => Some(LiteralValue(value, dataType))
+ case attr: Attribute => Some(FieldReference.column(quoteIfNeeded(attr.name)))
+ case IsNull(col) => generateExpression(col)
+ .map(c => new GeneralScalarExpression("IS NULL", Array[V2Expression](c)))
+ case IsNotNull(col) => generateExpression(col)
+ .map(c => new GeneralScalarExpression("IS NOT NULL", Array[V2Expression](c)))
+ case b: BinaryOperator =>
+ val left = generateExpression(b.left)
+ val right = generateExpression(b.right)
+ if (left.isDefined && right.isDefined) {
+ Some(new GeneralScalarExpression(b.sqlOperator, Array[V2Expression](left.get, right.get)))
Review comment:
ditto, every expression name that we support should be documented in `GeneralScalarExpression`
--
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r811621380
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java
##########
@@ -0,0 +1,124 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.util;
+
+import org.apache.spark.sql.connector.expressions.Expression;
+import org.apache.spark.sql.connector.expressions.FieldReference;
+import org.apache.spark.sql.connector.expressions.GeneralScalarExpression;
+import org.apache.spark.sql.connector.expressions.LiteralValue;
+import org.apache.spark.sql.errors.QueryExecutionErrors;
+
+/**
+ * The builder to generate SQL from V2 expressions.
+ */
+public class V2ExpressionSQLBuilder {
+ public String build(Expression expr) throws Throwable {
+ if (expr instanceof LiteralValue) {
+ return visitLiteral((LiteralValue) expr);
+ } else if (expr instanceof FieldReference) {
+ return visitFieldReference((FieldReference) expr);
+ } else if (expr instanceof GeneralScalarExpression) {
+ GeneralScalarExpression e = (GeneralScalarExpression) expr;
+ String name = e.name();
+ switch (name) {
+ case "IS_NULL":
+ return visitIsNull(build(e.children()[0]));
+ case "IS_NOT_NULL":
+ return visitIsNotNull(build(e.children()[0]));
+ case "=":
+ case "!=":
+ case "<=>":
+ case "<":
+ case "<=":
+ case ">":
+ case ">=":
+ case "+":
+ case "-":
+ case "*":
+ case "/":
+ case "%":
+ case "&&":
+ case "||":
+ case "AND":
+ case "OR":
+ case "&":
+ case "|":
+ case "^":
+ return visitBinaryOperation(name, build(e.children()[0]), build(e.children()[1]));
+ case "NOT":
+ return visitNot(build(e.children()[0]));
+ case "CASE_WHEN":
+ return visitCaseWhen(e);
+ // TODO supports other expressions
+ default:
+ return visitUnexpectedExpr(expr);
+ }
+ } else {
+ return visitUnexpectedExpr(expr);
+ }
+ }
+
+ protected String visitLiteral(LiteralValue literalValue) {
+ return literalValue.toString();
+ }
+
+ protected String visitFieldReference(FieldReference fieldReference) {
+ return fieldReference.toString();
+ }
+
+ protected String visitIsNull(String v) {
+ return v + " IS NULL";
+ }
+
+ protected String visitIsNotNull(String v) {
+ return v + " IS NOT NULL";
+ }
+
+ protected String visitBinaryOperation(String name, String l, String r) {
+ return "(" + l + ") " + name + " (" + r + ")";
+ }
+
+ protected String visitNot(String v) {
+ return "NOT (" + v + ")";
+ }
+
+ protected String visitCaseWhen(GeneralScalarExpression e) throws Throwable {
+ GeneralScalarExpression branchExpression = (GeneralScalarExpression) e.children()[0];
+ StringBuilder sb = new StringBuilder("CASE");
Review comment:
Shall we have a `StringBuilder` per `V2ExpressionSQLBuilder` instance?
--
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r811621731
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala
##########
@@ -1978,4 +1978,8 @@ object QueryExecutionErrors {
errorClass = "INVALID_PARAMETER_VALUE",
messageParameters = Array("unit", "timestampadd", unit))
}
+
+ def unexpectedV2ExpressionError(expr: V2Expression): Throwable = {
+ new IllegalArgumentException(s"Unexpected V2 expression: $expr")
Review comment:
this is not a user-facing error, I think we can inline 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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r811145844
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java
##########
@@ -22,20 +22,46 @@
import org.apache.spark.annotation.Evolving;
/**
- * The general SQL string corresponding to expression.
+ * The general V2 expression corresponding to V1 expression.
Review comment:
can we follow the classdoc of `GeneralAggregateFunc`?
```
The general representation of SQL scalar expressions, which contains the upper-cased
expression name and all the children expressions.
```
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r816400436
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java
##########
@@ -0,0 +1,133 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.util;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.spark.sql.connector.expressions.Expression;
+import org.apache.spark.sql.connector.expressions.FieldReference;
+import org.apache.spark.sql.connector.expressions.GeneralScalarExpression;
+import org.apache.spark.sql.connector.expressions.LiteralValue;
+
+/**
+ * The builder to generate SQL from V2 expressions.
+ */
+public class V2ExpressionSQLBuilder {
+ public String build(Expression expr) {
+ if (expr instanceof LiteralValue) {
+ return visitLiteral((LiteralValue) expr);
+ } else if (expr instanceof FieldReference) {
+ return visitFieldReference((FieldReference) expr);
+ } else if (expr instanceof GeneralScalarExpression) {
+ GeneralScalarExpression e = (GeneralScalarExpression) expr;
+ String name = e.name();
+ switch (name) {
+ case "IS_NULL":
+ return visitIsNull(build(e.children()[0]));
+ case "IS_NOT_NULL":
+ return visitIsNotNull(build(e.children()[0]));
+ case "=":
+ case "!=":
+ case "<=>":
+ case "<":
+ case "<=":
+ case ">":
+ case ">=":
+ case "+":
+ case "-":
+ case "*":
+ case "/":
+ case "%":
+ case "&&":
+ case "||":
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r816000244
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java
##########
@@ -0,0 +1,133 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.util;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.spark.sql.connector.expressions.Expression;
+import org.apache.spark.sql.connector.expressions.FieldReference;
+import org.apache.spark.sql.connector.expressions.GeneralScalarExpression;
+import org.apache.spark.sql.connector.expressions.LiteralValue;
+
+/**
+ * The builder to generate SQL from V2 expressions.
+ */
+public class V2ExpressionSQLBuilder {
+ public String build(Expression expr) {
+ if (expr instanceof LiteralValue) {
+ return visitLiteral((LiteralValue) expr);
+ } else if (expr instanceof FieldReference) {
+ return visitFieldReference((FieldReference) expr);
+ } else if (expr instanceof GeneralScalarExpression) {
+ GeneralScalarExpression e = (GeneralScalarExpression) expr;
+ String name = e.name();
+ switch (name) {
+ case "IS_NULL":
+ return visitIsNull(build(e.children()[0]));
+ case "IS_NOT_NULL":
+ return visitIsNotNull(build(e.children()[0]));
+ case "=":
+ case "!=":
+ case "<=>":
+ case "<":
+ case "<=":
+ case ">":
+ case ">=":
+ case "+":
+ case "-":
+ case "*":
+ case "/":
+ case "%":
+ case "&&":
Review comment:
does spark have this operator?
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r816003871
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala
##########
@@ -0,0 +1,93 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.expressions.{Add, Attribute, BinaryArithmetic, BinaryOperator, BitwiseAnd, BitwiseOr, BitwiseXor, CaseWhen, Divide, EqualTo, Expression, IntegralDivide, IsNotNull, IsNull, Literal, Multiply, Not, Pmod, Remainder, Subtract}
+import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference, GeneralScalarExpression, LiteralValue}
+
+/**
+ * The builder to generate V2 expressions from catalyst expressions.
+ */
+class V2ExpressionBuilder(e: Expression) {
+
+ def build(): Option[V2Expression] = generateExpression(e)
+
+ private def canTranslate(b: BinaryOperator) = b match {
+ case add: Add => add.failOnError
+ case sub: Subtract => sub.failOnError
+ case mul: Multiply => mul.failOnError
+ case div: Divide => div.failOnError
+ case intDiv: IntegralDivide => intDiv.failOnError
Review comment:
https://github.com/apache/spark/pull/35494/files#r816003535
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r816005471
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala
##########
@@ -194,6 +195,24 @@ abstract class JdbcDialect extends Serializable with Logging{
case _ => value
}
+ /**
+ * Converts V2 expression to String representing a SQL expression.
+ * @param expr The V2 expression to be converted.
+ * @return Converted value.
+ */
+ @Since("3.3.0")
+ def compileExpression(expr: Expression): Option[String] = {
+ val V2ExpressionSQLBuilder = new V2ExpressionSQLBuilder()
+ try {
+ expr match {
+ case _: FieldReference => Some(V2ExpressionSQLBuilder.build(expr)).map(quoteIdentifier)
+ case _: GeneralScalarExpression => Some(V2ExpressionSQLBuilder.build(expr))
Review comment:
what are we doing here?
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r818339484
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.expressions.{Add, Attribute, BinaryArithmetic, BinaryOperator, BitwiseNot, CaseWhen, Divide, EqualTo, Expression, IsNotNull, IsNull, Literal, Multiply, Not, Remainder, Subtract, UnaryMinus}
+import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference, GeneralScalarExpression, LiteralValue}
+
+/**
+ * The builder to generate V2 expressions from catalyst expressions.
+ */
+class V2ExpressionBuilder(e: Expression) {
+
+ def build(): Option[V2Expression] = generateExpression(e)
+
+ private def canTranslate(b: BinaryOperator) = b match {
+ case add: Add => add.failOnError
+ case sub: Subtract => sub.failOnError
+ case mul: Multiply => mul.failOnError
+ case div: Divide => div.failOnError
+ case r: Remainder => r.failOnError
+ case o: BinaryOperator => !o.isInstanceOf[BinaryArithmetic]
Review comment:
this is not an allowlist. I think we should match
```
case _: BinaryArithmetic => true
case _: BinaryBinaryComparison => true
case _: And | _: Or | _: Not => true
case _ => false
```
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r818650936
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.expressions.{Add, And, Attribute, BinaryComparison, BinaryOperator, BitwiseAnd, BitwiseNot, BitwiseOr, BitwiseXor, CaseWhen, Divide, EqualTo, Expression, IsNotNull, IsNull, Literal, Multiply, Not, Or, Remainder, Subtract, UnaryMinus}
+import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference, GeneralScalarExpression, LiteralValue}
+
+/**
+ * The builder to generate V2 expressions from catalyst expressions.
+ */
+class V2ExpressionBuilder(e: Expression) {
+
+ def build(): Option[V2Expression] = generateExpression(e)
+
+ private def canTranslate(b: BinaryOperator) = b match {
+ case _: And | _: Or => true
+ case _: BinaryComparison => true
+ case _: BitwiseAnd | _: BitwiseOr | _: BitwiseXor => true
+ case add: Add => add.failOnError
+ case sub: Subtract => sub.failOnError
+ case mul: Multiply => mul.failOnError
+ case div: Divide => div.failOnError
+ case r: Remainder => r.failOnError
+ case _ => false
+ }
+
+ private def generateExpression(expr: Expression): Option[V2Expression] = expr match {
+ case Literal(value, dataType) => Some(LiteralValue(value, dataType))
+ case attr: Attribute => Some(FieldReference.column(quoteIfNeeded(attr.name)))
Review comment:
We should only quote it when print it, e.g. `FieldReference.toString`
--
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r806390170
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralSQLExpression.java
##########
@@ -22,20 +22,34 @@
import org.apache.spark.annotation.Evolving;
/**
- * The general SQL string corresponding to expression.
+ * The general V2 expression corresponding to V1 expression.
*
* @since 3.3.0
*/
@Evolving
public class GeneralSQLExpression implements Expression, Serializable {
Review comment:
Currently, we use this for V2 expressions appears in aggregate.
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/ExpressionSQLBuilder.scala
##########
@@ -67,3 +122,8 @@ class ExpressionSQLBuilder(e: Expression) {
case _ => None
}
}
+
+object GeneralExpression {
+ def apply(expressions: Array[V2Expression], name: String = "UNDEFINED"): GeneralSQLExpression =
+ new GeneralSQLExpression(name, expressions)
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] HyukjinKwon commented on a change in pull request #35494: [SPARK-37960][SQL][FOLLOWUP] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r805224185
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralSQLExpression.java
##########
@@ -22,20 +22,34 @@
import org.apache.spark.annotation.Evolving;
/**
- * The general SQL string corresponding to expression.
+ * The general V2 expression corresponding to V1 expression.
Review comment:
Can we document this that this assumes that SQL is able to be parsed in the source that implements this 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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r806390170
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralSQLExpression.java
##########
@@ -22,20 +22,34 @@
import org.apache.spark.annotation.Evolving;
/**
- * The general SQL string corresponding to expression.
+ * The general V2 expression corresponding to V1 expression.
*
* @since 3.3.0
*/
@Evolving
public class GeneralSQLExpression implements Expression, Serializable {
Review comment:
Currently, we use this for V2 expressions appears in aggregate.
--
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r813521886
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.expressions.{Add, Attribute, BinaryOperator, CaseWhen, Divide, EqualTo, Expression, IntegralDivide, IsNotNull, IsNull, Literal, Multiply, Not, Pmod, Remainder, Subtract}
+import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference, GeneralScalarExpression, LiteralValue}
+import org.apache.spark.sql.internal.SQLConf
+
+/**
+ * The builder to generate V2 expressions from catalyst expressions.
+ */
+class V2ExpressionBuilder(e: Expression) {
+
+ def build(): Option[V2Expression] = generateExpression(e)
+
+ private def isNotBinaryArithmeticCareAnsi(expr: Expression) = expr match {
+ case _: Add | _: Subtract | _: Multiply | _: Divide |
Review comment:
`BinaryArithmetic` has a `failOnError` flag, shall we check 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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r813522232
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/connector/V1ReadFallbackSuite.scala
##########
@@ -129,8 +127,8 @@ class TableWithV1ReadFallback(override val name: String) extends Table with Supp
override def schema(): StructType = V1ReadFallbackCatalog.schema
- override def capabilities(): util.Set[TableCapability] = {
- util.EnumSet.of(TableCapability.BATCH_READ)
+ override def capabilities(): java.util.Set[TableCapability] = {
+ java.util.EnumSet.of(TableCapability.BATCH_READ)
Review comment:
hmm, why do we touch these unrelated test suites?
--
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] baibaichen commented on pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
baibaichen commented on pull request #35494:
URL: https://github.com/apache/spark/pull/35494#issuecomment-1049954754
reactor => refactor?
--
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r813520858
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java
##########
@@ -20,22 +20,42 @@
import java.io.Serializable;
import org.apache.spark.annotation.Evolving;
+import org.apache.spark.sql.connector.util.V2ExpressionSQLBuilder;
/**
- * The general SQL string corresponding to expression.
+ * The general representation of SQL scalar expressions, which contains the upper-cased
+ * expression name and all the children expressions.
*
* @since 3.3.0
*/
@Evolving
-public class GeneralSQLExpression implements Expression, Serializable {
- private String sql;
+public class GeneralScalarExpression implements Expression, Serializable {
+ private String name;
+ private Expression[] children;
+ private String sql;
- public GeneralSQLExpression(String sql) {
- this.sql = sql;
- }
+ public GeneralScalarExpression(String name, Expression[] children) {
+ this.name = name;
+ this.children = children;
+ }
- public String sql() { return sql; }
+ public String name() { return name; }
+ public Expression[] children() { return children; }
- @Override
- public String toString() { return sql; }
+ @Override
+ public String toString() {
+ if (sql == null) {
+ V2ExpressionSQLBuilder builder = new V2ExpressionSQLBuilder();
+ try {
+ this.sql = builder.build(this);
+ } catch (Throwable e) {
+ // Attempt to get SQL failed.
+ }
+ }
+ if (sql == null) {
+ return name + "(" + children.toString() + ")";
Review comment:
`Expression[].toString` returns a meaningless string, I think it's better to use `children.mkString(", ")` (please use proper java 8 lambda syntax)
--
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r813582349
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.expressions.{Add, Attribute, BinaryOperator, CaseWhen, Divide, EqualTo, Expression, IntegralDivide, IsNotNull, IsNull, Literal, Multiply, Not, Pmod, Remainder, Subtract}
+import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference, GeneralScalarExpression, LiteralValue}
+import org.apache.spark.sql.internal.SQLConf
+
+/**
+ * The builder to generate V2 expressions from catalyst expressions.
+ */
+class V2ExpressionBuilder(e: Expression) {
+
+ def build(): Option[V2Expression] = generateExpression(e)
+
+ private def isNotBinaryArithmeticCareAnsi(expr: Expression) = expr match {
+ case _: Add | _: Subtract | _: Multiply | _: Divide |
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r811517388
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java
##########
@@ -22,20 +22,46 @@
import org.apache.spark.annotation.Evolving;
/**
- * The general SQL string corresponding to expression.
+ * The general V2 expression corresponding to V1 expression.
+ * <p>
+ * The currently supported expression:
+ * <ol>
+ * <li><pre>CASE WHEN expr1 THEN expr2 [WHEN expr3 THEN expr4]* [ELSE expr5] END</pre>
+ * Since 3.3.0</li>
+ * </ol>
*
* @since 3.3.0
*/
@Evolving
-public class GeneralSQLExpression implements Expression, Serializable {
- private String sql;
+public class GeneralScalarExpression implements Expression, Serializable {
+ private String name;
+ private Expression[] children;
+ private String sql;
Review comment:
I want PushedAggregates keeps the SQL.
```
val expected_plan_fragment =
"PushedAggregates: [COUNT(CASE WHEN ((SALARY) > (8000.00)) AND ((SALARY) < (10000.00))" +
" THEN SALARY ELSE 0.00 END), C..., " +
"PushedFilters: [], " +
"PushedGroupByColumns: [DEPT]"
```
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r816400954
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java
##########
@@ -0,0 +1,133 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.util;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.spark.sql.connector.expressions.Expression;
+import org.apache.spark.sql.connector.expressions.FieldReference;
+import org.apache.spark.sql.connector.expressions.GeneralScalarExpression;
+import org.apache.spark.sql.connector.expressions.LiteralValue;
+
+/**
+ * The builder to generate SQL from V2 expressions.
+ */
+public class V2ExpressionSQLBuilder {
+ public String build(Expression expr) {
+ if (expr instanceof LiteralValue) {
+ return visitLiteral((LiteralValue) expr);
+ } else if (expr instanceof FieldReference) {
+ return visitFieldReference((FieldReference) expr);
+ } else if (expr instanceof GeneralScalarExpression) {
+ GeneralScalarExpression e = (GeneralScalarExpression) expr;
+ String name = e.name();
+ switch (name) {
+ case "IS_NULL":
+ return visitIsNull(build(e.children()[0]));
+ case "IS_NOT_NULL":
+ return visitIsNotNull(build(e.children()[0]));
+ case "=":
+ case "!=":
+ case "<=>":
+ case "<":
+ case "<=":
+ case ">":
+ case ">=":
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r814550112
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala
##########
@@ -203,30 +219,30 @@ abstract class JdbcDialect extends Serializable with Logging{
def compileAggregate(aggFunction: AggregateFunc): Option[String] = {
aggFunction match {
case min: Min =>
- val sql = min.column match {
+ min.column match {
Review comment:
we can do `compileExpression(min.column).map(v => s"MIN($v)")` directly if https://github.com/apache/spark/pull/35494/files#r814549510 is done
--
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 change in pull request #35494: [SPARK-37960][SQL][FOLLOWUP] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r805453204
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala
##########
@@ -194,6 +194,66 @@ abstract class JdbcDialect extends Serializable with Logging{
case _ => value
}
+ protected final val BINARY_COMPARISON = Set("=", "!=", "<=>", "<", "<=", ">", ">=",
+ "+", "-", "*", "/", "%", "&&", "||", "AND", "OR", "&", "|", "^")
+
+ /**
+ * Converts V2 expression to String representing a SQL expression.
+ * @param expr The V2 expression to be converted.
+ * @return Converted value.
+ */
+ @Since("3.3.0")
+ def compileExpression(expr: Expression): Option[String] = expr match {
+ case l @ LiteralValue(_, _) => Some(l.toString)
Review comment:
Yes. Thank you for your reminder.
But `l: Literal` used in many places.
https://github.com/apache/spark/blob/1c0793a75b74ac7b55631e61d9e827e93647f1d2/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/DecimalPrecision.scala#L325
--
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 change in pull request #35494: [SPARK-37960][SQL][FOLLOWUP] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r805451553
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralSQLExpression.java
##########
@@ -22,20 +22,34 @@
import org.apache.spark.annotation.Evolving;
/**
- * The general SQL string corresponding to expression.
+ * The general V2 expression corresponding to V1 expression.
*
* @since 3.3.0
*/
@Evolving
public class GeneralSQLExpression implements Expression, Serializable {
Review comment:
We will use V2 expression to replace SQL 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] HyukjinKwon commented on a change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r806387895
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralSQLExpression.java
##########
@@ -22,20 +22,34 @@
import org.apache.spark.annotation.Evolving;
/**
- * The general SQL string corresponding to expression.
+ * The general V2 expression corresponding to V1 expression.
*
* @since 3.3.0
*/
@Evolving
public class GeneralSQLExpression implements Expression, Serializable {
Review comment:
Do you mean that we will use this for all V2 expressions? I thought this is one of the optional APIs.
--
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] HyukjinKwon commented on a change in pull request #35494: [SPARK-37960][SQL][FOLLOWUP] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r805224161
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralSQLExpression.java
##########
@@ -22,20 +22,34 @@
import org.apache.spark.annotation.Evolving;
/**
- * The general SQL string corresponding to expression.
+ * The general V2 expression corresponding to V1 expression.
*
* @since 3.3.0
*/
@Evolving
public class GeneralSQLExpression implements Expression, Serializable {
Review comment:
Can we rename it something like `SQLStringHolder`, `SQLStringRepresentation` and `SQLString`?
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralSQLExpression.java
##########
@@ -22,20 +22,34 @@
import org.apache.spark.annotation.Evolving;
/**
- * The general SQL string corresponding to expression.
+ * The general V2 expression corresponding to V1 expression.
Review comment:
Can we document this that this assumes that SQL is able to be parsed in the source that implements this expression?
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala
##########
@@ -194,6 +194,66 @@ abstract class JdbcDialect extends Serializable with Logging{
case _ => value
}
+ protected final val BINARY_COMPARISON = Set("=", "!=", "<=>", "<", "<=", ">", ">=",
+ "+", "-", "*", "/", "%", "&&", "||", "AND", "OR", "&", "|", "^")
+
+ /**
+ * Converts V2 expression to String representing a SQL expression.
+ * @param expr The V2 expression to be converted.
+ * @return Converted value.
Review comment:
Can we describe which value it is after the conversion?
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala
##########
@@ -194,6 +194,66 @@ abstract class JdbcDialect extends Serializable with Logging{
case _ => value
}
+ protected final val BINARY_COMPARISON = Set("=", "!=", "<=>", "<", "<=", ">", ">=",
+ "+", "-", "*", "/", "%", "&&", "||", "AND", "OR", "&", "|", "^")
+
+ /**
+ * Converts V2 expression to String representing a SQL expression.
+ * @param expr The V2 expression to be converted.
+ * @return Converted value.
+ */
+ @Since("3.3.0")
+ def compileExpression(expr: Expression): Option[String] = expr match {
+ case l @ LiteralValue(_, _) => Some(l.toString)
Review comment:
```suggestion
case l: LiteralValue => Some(l.toString)
```
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala
##########
@@ -194,6 +194,66 @@ abstract class JdbcDialect extends Serializable with Logging{
case _ => value
}
+ protected final val BINARY_COMPARISON = Set("=", "!=", "<=>", "<", "<=", ">", ">=",
+ "+", "-", "*", "/", "%", "&&", "||", "AND", "OR", "&", "|", "^")
+
+ /**
+ * Converts V2 expression to String representing a SQL expression.
+ * @param expr The V2 expression to be converted.
+ * @return Converted value.
+ */
+ @Since("3.3.0")
+ def compileExpression(expr: Expression): Option[String] = expr match {
+ case l @ LiteralValue(_, _) => Some(l.toString)
Review comment:
```suggestion
case lit: LiteralValue => Some(l.toString)
```
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala
##########
@@ -194,6 +194,66 @@ abstract class JdbcDialect extends Serializable with Logging{
case _ => value
}
+ protected final val BINARY_COMPARISON = Set("=", "!=", "<=>", "<", "<=", ">", ">=",
+ "+", "-", "*", "/", "%", "&&", "||", "AND", "OR", "&", "|", "^")
+
+ /**
+ * Converts V2 expression to String representing a SQL expression.
+ * @param expr The V2 expression to be converted.
+ * @return Converted value.
+ */
+ @Since("3.3.0")
+ def compileExpression(expr: Expression): Option[String] = expr match {
+ case l @ LiteralValue(_, _) => Some(l.toString)
Review comment:
should avoid `l` https://github.com/databricks/scala-style-guide#variable-naming-convention as a variable name.
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/ExpressionSQLBuilder.scala
##########
@@ -67,3 +122,8 @@ class ExpressionSQLBuilder(e: Expression) {
case _ => None
}
}
+
+object GeneralExpression {
+ def apply(expressions: Array[V2Expression], name: String = "UNDEFINED"): GeneralSQLExpression =
+ new GeneralSQLExpression(name, expressions)
Review comment:
Can we deal with this in GeneralExpression's constructor?
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralSQLExpression.java
##########
@@ -22,20 +22,34 @@
import org.apache.spark.annotation.Evolving;
/**
- * The general SQL string corresponding to expression.
+ * The general V2 expression corresponding to V1 expression.
*
* @since 3.3.0
*/
@Evolving
public class GeneralSQLExpression implements Expression, Serializable {
+ private String name;
+ private Expression[] children;
Review comment:
The indentation should be 2-spaced
--
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r806402239
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/ExpressionSQLBuilder.scala
##########
@@ -67,3 +122,8 @@ class ExpressionSQLBuilder(e: Expression) {
case _ => None
}
}
+
+object GeneralExpression {
+ def apply(expressions: Array[V2Expression], name: String = "UNDEFINED"): GeneralSQLExpression =
+ new GeneralSQLExpression(name, expressions)
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r811617740
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java
##########
@@ -0,0 +1,186 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.expressions;
+
+import java.io.Serializable;
+
+import org.apache.spark.annotation.Evolving;
+
+/**
+ * The general representation of SQL scalar expressions, which contains the upper-cased
+ * expression name and all the children expressions.
+ * <p>
+ * The currently supported expressions:
+ * <table border="1">
+ * <tr>
+ * <th>Expression name</th>
+ * <th>SQL scalar expression</th>
+ * <th>Since version</th>
+ * </tr>
+ * <tr>
+ * <td>IS_NULL</td>
+ * <td><pre>expr IS NULL</pre></td>
+ * <td>3.3.0</td>
+ * </tr>
+ * <tr>
+ * <td>IS_NOT_NULL</td>
+ * <td><pre>expr IS NOT NULL</pre></td>
+ * <td>3.3.0</td>
+ * </tr>
+ * <tr>
+ * <td>=</td>
+ * <td><pre>expr1 = expr2</pre></td>
+ * <td>3.3.0</td>
+ * </tr>
+ * <tr>
+ * <td>!=</td>
+ * <td><pre>expr1 != expr2</pre></td>
+ * <td>3.3.0</td>
+ * </tr>
+ * <tr>
+ * <td><=></td>
+ * <td><pre>expr1 <=> expr2</pre></td>
+ * <td>3.3.0</td>
+ * </tr>
+ * <tr>
+ * <td><</td>
+ * <td><pre>expr1 < expr2</pre></td>
+ * <td>3.3.0</td>
+ * </tr>
+ * <tr>
+ * <td><=</td>
+ * <td><pre>expr1 <= expr2</pre></td>
+ * <td>3.3.0</td>
+ * </tr>
+ * <tr>
+ * <td>></td>
+ * <td><pre>expr1 > expr2</pre></td>
+ * <td>3.3.0</td>
+ * </tr>
+ * <tr>
+ * <td>>=</td>
+ * <td><pre>expr1 >= expr2</pre></td>
+ * <td>3.3.0</td>
+ * </tr>
+ * <tr>
+ * <td>+</td>
+ * <td><pre>expr1 + expr2</pre></td>
Review comment:
Now, this comes to the tricky part. If ANSI mode is off, `expr1 + expr2` has a different behavior in Spark compared to other databases, because it won't fail for overflow.
One simple fix is: We don't translate math operations to v2 expression if ANSI mode is off. Or we can have a new expression name for ANSI-off add (but I'm not sure if any database can generate SQL for 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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r811217042
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala
##########
@@ -0,0 +1,124 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.expressions.{Attribute, BinaryOperator, CaseWhen, EqualTo, Expression, IsNotNull, IsNull, Literal, Not}
+import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference, GeneralScalarExpression, LiteralValue}
+
+/**
+ * The builder to generate V2 expressions from catalyst expressions.
+ */
+class V2ExpressionBuilder(e: Expression) {
+
+ def build(): Option[V2Expression] = {
+ val expression = generateExpression(e)
+ expression.foreach {
+ case generated: GeneralScalarExpression =>
+ generateSQL(e).foreach(sql => generated.setSql(sql))
+ case _ =>
+ }
+ expression
+ }
+
+ private def generateExpression(expr: Expression): Option[V2Expression] = expr match {
+ case Literal(value, dataType) => Some(LiteralValue(value, dataType))
+ case attr: Attribute => Some(FieldReference.column(quoteIfNeeded(attr.name)))
+ case IsNull(col) => generateExpression(col)
+ .map(c => new GeneralScalarExpression("IS NULL", Array[V2Expression](c)))
+ case IsNotNull(col) => generateExpression(col)
+ .map(c => new GeneralScalarExpression("IS NOT NULL", Array[V2Expression](c)))
+ case b: BinaryOperator =>
+ val left = generateExpression(b.left)
+ val right = generateExpression(b.right)
+ if (left.isDefined && right.isDefined) {
+ Some(new GeneralScalarExpression(b.sqlOperator, Array[V2Expression](left.get, right.get)))
+ } else {
+ None
+ }
+ case Not(eq: EqualTo) =>
+ val left = generateExpression(eq.left)
+ val right = generateExpression(eq.right)
+ if (left.isDefined && right.isDefined) {
+ Some(new GeneralScalarExpression("!=", Array[V2Expression](left.get, right.get)))
+ } else {
+ None
+ }
+ case Not(child) => generateExpression(child)
+ .map(v => new GeneralScalarExpression("NOT", Array[V2Expression](v)))
+ case CaseWhen(branches, elseValue) =>
+ val conditions = branches.map(_._1).flatMap(generateExpression)
+ val values = branches.map(_._2).flatMap(generateExpression)
+ if (conditions.length == branches.length && values.length == branches.length) {
+ val branchExpressions = conditions.zip(values).map { case (c, v) =>
+ new GeneralScalarExpression(Array[V2Expression](c, v))
+ }
+ val branchExpression = new GeneralScalarExpression(branchExpressions.toArray[V2Expression])
+ if (elseValue.isDefined) {
+ elseValue.flatMap(generateExpression).map { v =>
+ new GeneralScalarExpression("CASE WHEN", Array[V2Expression](branchExpression, v))
+ }
+ } else {
+ Some(new GeneralScalarExpression("CASE WHEN", Array[V2Expression](branchExpression)))
+ }
+ } else {
+ None
+ }
+ // TODO supports other expressions
+ case _ => None
+ }
+
+ private def generateSQL(expr: Expression): Option[String] = expr match {
Review comment:
I don't think this helps. What we need is a tool that can help source implementations to easily implement generating SQL string from v2 expressions, and can customize for special cases.
I'd like to see something like this
```
class V2ExpressionSQLBuilder {
pubic String build(expr: V2Expression) {
if (expr instanceof LiteralValue) {
return visitLiteral((LiteralValue) expr);
} else if ...
} else if (expr instanceof GeneralScalarExpression) {
GeneralScalarExpression e = (GeneralScalarExpression) expr;
String name = e.name;
if ("IS_NULL" == name) {
return visitIsNull(build(e.children()[0]));
} else if ...
} else {
return visitUnexpectedExpr(expr);
}
}
protected String visitLiteral...
protected String visitIsNull(String child)...
...
}
```
Then source implementations can use it directly, or extend it and overwrite some `visitXXX` methods for customization.
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r814544775
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java
##########
@@ -0,0 +1,86 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.expressions;
+
+import java.io.Serializable;
+import java.util.Arrays;
+
+import org.apache.spark.annotation.Evolving;
+import org.apache.spark.sql.connector.util.V2ExpressionSQLBuilder;
+
+// scalastyle:off line.size.limit
+/**
+ * The general representation of SQL scalar expressions, which contains the upper-cased
+ * expression name and all the children expressions.
+ * <p>
+ * The currently supported SQL scalar expressions:
+ * <ol>
+ * <li><pre>Expression name: IS_NULL</pre><pre>SQL scalar expression: `expr IS NULL`</pre> ANSI enabled: No, Since 3.3.0</li>
Review comment:
how about
```
Name: IS_NULL SQL semantic: `expr IS NULL` Version: 3.3.0
```
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r816742915
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java
##########
@@ -0,0 +1,165 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.util;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.spark.sql.connector.expressions.Expression;
+import org.apache.spark.sql.connector.expressions.FieldReference;
+import org.apache.spark.sql.connector.expressions.GeneralScalarExpression;
+import org.apache.spark.sql.connector.expressions.LiteralValue;
+
+/**
+ * The builder to generate SQL from V2 expressions.
+ */
+public class V2ExpressionSQLBuilder {
+ public String build(Expression expr) {
+ if (expr instanceof LiteralValue) {
+ return visitLiteral((LiteralValue) expr);
+ } else if (expr instanceof FieldReference) {
+ return visitFieldReference((FieldReference) expr);
+ } else if (expr instanceof GeneralScalarExpression) {
+ GeneralScalarExpression e = (GeneralScalarExpression) expr;
+ String name = e.name();
+ switch (name) {
+ case "IS_NULL":
+ return visitIsNull(build(e.children()[0]));
+ case "IS_NOT_NULL":
+ return visitIsNotNull(build(e.children()[0]));
+ case "=":
+ case "!=":
+ case "<=>":
+ case "<":
+ case "<=":
+ case ">":
+ case ">=":
+ return visitBinaryComparison(name, build(e.children()[0]), build(e.children()[1]));
+ case "+":
+ case "-":
+ case "*":
+ case "/":
+ case "%":
+ return visitBinaryArithmetic(name, build(e.children()[0]), build(e.children()[1]));
+ case "AND":
+ return visitAnd(name, build(e.children()[0]), build(e.children()[1]));
+ case "OR":
+ return visitOr(name, build(e.children()[0]), build(e.children()[1]));
+ case "NOT":
+ return visitNot(build(e.children()[0]));
+ case "&":
+ return visitBitwiseAnd(name, build(e.children()[0]), build(e.children()[1]));
+ case "|":
+ return visitBitwiseOr(name, build(e.children()[0]), build(e.children()[1]));
+ case "^":
+ return visitBitwiseXor(name, build(e.children()[0]), build(e.children()[1]));
+ case "~":
+ return visitBitwiseNot(build(e.children()[0]));
+ case "CASE_WHEN":
+ List<String> children = new ArrayList<>();
+ for (Expression child : e.children()) {
+ children.add(build(child));
+ }
+ return visitCaseWhen(children.toArray(new String[e.children().length]));
+ // TODO supports other expressions
+ default:
+ return visitUnexpectedExpr(expr);
+ }
+ } else {
+ return visitUnexpectedExpr(expr);
+ }
+ }
+
+ protected String visitLiteral(LiteralValue literalValue) {
+ return literalValue.toString();
+ }
+
+ protected String visitFieldReference(FieldReference fieldRef) {
+ if (fieldRef.fieldNames().length != 1) {
+ throw new IllegalArgumentException(
+ "FieldReference with field name has multiple or zero parts unsupported: " + fieldRef);
+ }
+ return fieldRef.fieldNames()[0];
Review comment:
We should quote the name, to generate standard sql 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 a change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r816742481
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java
##########
@@ -0,0 +1,165 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.util;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.spark.sql.connector.expressions.Expression;
+import org.apache.spark.sql.connector.expressions.FieldReference;
+import org.apache.spark.sql.connector.expressions.GeneralScalarExpression;
+import org.apache.spark.sql.connector.expressions.LiteralValue;
+
+/**
+ * The builder to generate SQL from V2 expressions.
+ */
+public class V2ExpressionSQLBuilder {
+ public String build(Expression expr) {
+ if (expr instanceof LiteralValue) {
+ return visitLiteral((LiteralValue) expr);
+ } else if (expr instanceof FieldReference) {
+ return visitFieldReference((FieldReference) expr);
+ } else if (expr instanceof GeneralScalarExpression) {
+ GeneralScalarExpression e = (GeneralScalarExpression) expr;
+ String name = e.name();
+ switch (name) {
+ case "IS_NULL":
+ return visitIsNull(build(e.children()[0]));
+ case "IS_NOT_NULL":
+ return visitIsNotNull(build(e.children()[0]));
+ case "=":
+ case "!=":
+ case "<=>":
+ case "<":
+ case "<=":
+ case ">":
+ case ">=":
+ return visitBinaryComparison(name, build(e.children()[0]), build(e.children()[1]));
+ case "+":
+ case "-":
+ case "*":
+ case "/":
+ case "%":
+ return visitBinaryArithmetic(name, build(e.children()[0]), build(e.children()[1]));
+ case "AND":
+ return visitAnd(name, build(e.children()[0]), build(e.children()[1]));
+ case "OR":
+ return visitOr(name, build(e.children()[0]), build(e.children()[1]));
+ case "NOT":
+ return visitNot(build(e.children()[0]));
+ case "&":
+ return visitBitwiseAnd(name, build(e.children()[0]), build(e.children()[1]));
+ case "|":
+ return visitBitwiseOr(name, build(e.children()[0]), build(e.children()[1]));
+ case "^":
+ return visitBitwiseXor(name, build(e.children()[0]), build(e.children()[1]));
+ case "~":
+ return visitBitwiseNot(build(e.children()[0]));
+ case "CASE_WHEN":
+ List<String> children = new ArrayList<>();
+ for (Expression child : e.children()) {
+ children.add(build(child));
+ }
+ return visitCaseWhen(children.toArray(new String[e.children().length]));
+ // TODO supports other expressions
+ default:
+ return visitUnexpectedExpr(expr);
+ }
+ } else {
+ return visitUnexpectedExpr(expr);
+ }
+ }
+
+ protected String visitLiteral(LiteralValue literalValue) {
+ return literalValue.toString();
+ }
+
+ protected String visitFieldReference(FieldReference fieldRef) {
+ if (fieldRef.fieldNames().length != 1) {
Review comment:
I don't see why more than one name parts are unsupported. It's only a problem in JDBC v2 and we should have a `V2ExpressionSQLBuilder` subclass in JDBC to fail with more than one name parts.
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r816003134
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java
##########
@@ -0,0 +1,133 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.util;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.spark.sql.connector.expressions.Expression;
+import org.apache.spark.sql.connector.expressions.FieldReference;
+import org.apache.spark.sql.connector.expressions.GeneralScalarExpression;
+import org.apache.spark.sql.connector.expressions.LiteralValue;
+
+/**
+ * The builder to generate SQL from V2 expressions.
+ */
+public class V2ExpressionSQLBuilder {
+ public String build(Expression expr) {
+ if (expr instanceof LiteralValue) {
+ return visitLiteral((LiteralValue) expr);
+ } else if (expr instanceof FieldReference) {
+ return visitFieldReference((FieldReference) expr);
+ } else if (expr instanceof GeneralScalarExpression) {
+ GeneralScalarExpression e = (GeneralScalarExpression) expr;
+ String name = e.name();
+ switch (name) {
+ case "IS_NULL":
+ return visitIsNull(build(e.children()[0]));
+ case "IS_NOT_NULL":
+ return visitIsNotNull(build(e.children()[0]));
+ case "=":
+ case "!=":
+ case "<=>":
+ case "<":
+ case "<=":
+ case ">":
+ case ">=":
+ case "+":
+ case "-":
+ case "*":
+ case "/":
+ case "%":
+ case "&&":
+ case "||":
+ case "AND":
+ case "OR":
+ case "&":
+ case "|":
+ case "^":
+ return visitBinaryOperation(name, build(e.children()[0]), build(e.children()[1]));
+ case "NOT":
+ return visitNot(build(e.children()[0]));
+ case "CASE_WHEN":
+ List<String> children = new ArrayList<>();
+ for (Expression child : e.children()) {
+ children.add(build(child));
+ }
+ return visitCaseWhen(children.toArray(new String[e.children().length]));
+ // TODO supports other expressions
Review comment:
`IntegralDivide` is missing here.
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r816642962
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala
##########
@@ -194,6 +195,24 @@ abstract class JdbcDialect extends Serializable with Logging{
case _ => value
}
+ /**
+ * Converts V2 expression to String representing a SQL expression.
+ * @param expr The V2 expression to be converted.
+ * @return Converted value.
+ */
+ @Since("3.3.0")
+ def compileExpression(expr: Expression): Option[String] = {
+ val V2ExpressionSQLBuilder = new V2ExpressionSQLBuilder()
+ try {
+ expr match {
+ case _: FieldReference => Some(V2ExpressionSQLBuilder.build(expr)).map(quoteIdentifier)
+ case _: GeneralScalarExpression => Some(V2ExpressionSQLBuilder.build(expr))
Review comment:
`FieldReference` need `quoteIdentifier` to transform.
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r816420278
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala
##########
@@ -0,0 +1,93 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.expressions.{Add, Attribute, BinaryArithmetic, BinaryOperator, BitwiseAnd, BitwiseOr, BitwiseXor, CaseWhen, Divide, EqualTo, Expression, IntegralDivide, IsNotNull, IsNull, Literal, Multiply, Not, Pmod, Remainder, Subtract}
+import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference, GeneralScalarExpression, LiteralValue}
+
+/**
+ * The builder to generate V2 expressions from catalyst expressions.
+ */
+class V2ExpressionBuilder(e: Expression) {
+
+ def build(): Option[V2Expression] = generateExpression(e)
+
+ private def canTranslate(b: BinaryOperator) = b match {
+ case add: Add => add.failOnError
+ case sub: Subtract => sub.failOnError
+ case mul: Multiply => mul.failOnError
+ case div: Divide => div.failOnError
+ case intDiv: IntegralDivide => intDiv.failOnError
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 change in pull request #35494: [SPARK-37960][SQL][FOLLOWUP] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r805451395
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralSQLExpression.java
##########
@@ -22,20 +22,34 @@
import org.apache.spark.annotation.Evolving;
/**
- * The general SQL string corresponding to expression.
+ * The general V2 expression corresponding to V1 expression.
Review comment:
I have tested, the SQL syntax is different between databases.
So ,we try to use V2 expression to replace SQL string.
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralSQLExpression.java
##########
@@ -22,20 +22,34 @@
import org.apache.spark.annotation.Evolving;
/**
- * The general SQL string corresponding to expression.
+ * The general V2 expression corresponding to V1 expression.
*
* @since 3.3.0
*/
@Evolving
public class GeneralSQLExpression implements Expression, Serializable {
Review comment:
We will use V2 expression to replace SQL string.
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala
##########
@@ -194,6 +194,66 @@ abstract class JdbcDialect extends Serializable with Logging{
case _ => value
}
+ protected final val BINARY_COMPARISON = Set("=", "!=", "<=>", "<", "<=", ">", ">=",
+ "+", "-", "*", "/", "%", "&&", "||", "AND", "OR", "&", "|", "^")
+
+ /**
+ * Converts V2 expression to String representing a SQL expression.
+ * @param expr The V2 expression to be converted.
+ * @return Converted value.
+ */
+ @Since("3.3.0")
+ def compileExpression(expr: Expression): Option[String] = expr match {
+ case l @ LiteralValue(_, _) => Some(l.toString)
Review comment:
Yes. Thank you for your reminder.
But `l: Literal` used in many places.
https://github.com/apache/spark/blob/1c0793a75b74ac7b55631e61d9e827e93647f1d2/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/DecimalPrecision.scala#L325
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/ExpressionSQLBuilder.scala
##########
@@ -67,3 +122,8 @@ class ExpressionSQLBuilder(e: Expression) {
case _ => None
}
}
+
+object GeneralExpression {
+ def apply(expressions: Array[V2Expression], name: String = "UNDEFINED"): GeneralSQLExpression =
+ new GeneralSQLExpression(name, expressions)
Review comment:
I'm sorry. I don't understand what you mean.
--
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] HyukjinKwon commented on a change in pull request #35494: [SPARK-37960][SQL][FOLLOWUP] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r805224220
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala
##########
@@ -194,6 +194,66 @@ abstract class JdbcDialect extends Serializable with Logging{
case _ => value
}
+ protected final val BINARY_COMPARISON = Set("=", "!=", "<=>", "<", "<=", ">", ">=",
+ "+", "-", "*", "/", "%", "&&", "||", "AND", "OR", "&", "|", "^")
+
+ /**
+ * Converts V2 expression to String representing a SQL expression.
+ * @param expr The V2 expression to be converted.
+ * @return Converted value.
+ */
+ @Since("3.3.0")
+ def compileExpression(expr: Expression): Option[String] = expr match {
+ case l @ LiteralValue(_, _) => Some(l.toString)
Review comment:
```suggestion
case l: LiteralValue => Some(l.toString)
```
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala
##########
@@ -194,6 +194,66 @@ abstract class JdbcDialect extends Serializable with Logging{
case _ => value
}
+ protected final val BINARY_COMPARISON = Set("=", "!=", "<=>", "<", "<=", ">", ">=",
+ "+", "-", "*", "/", "%", "&&", "||", "AND", "OR", "&", "|", "^")
+
+ /**
+ * Converts V2 expression to String representing a SQL expression.
+ * @param expr The V2 expression to be converted.
+ * @return Converted value.
+ */
+ @Since("3.3.0")
+ def compileExpression(expr: Expression): Option[String] = expr match {
+ case l @ LiteralValue(_, _) => Some(l.toString)
Review comment:
```suggestion
case lit: LiteralValue => Some(l.toString)
```
--
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 change in pull request #35494: [SPARK-37960][SQL][FOLLOWUP] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r805451395
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralSQLExpression.java
##########
@@ -22,20 +22,34 @@
import org.apache.spark.annotation.Evolving;
/**
- * The general SQL string corresponding to expression.
+ * The general V2 expression corresponding to V1 expression.
Review comment:
I have tested, the SQL syntax is different between databases.
So ,we try to use V2 expression to replace SQL 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] HyukjinKwon commented on a change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r806387187
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/ExpressionSQLBuilder.scala
##########
@@ -67,3 +122,8 @@ class ExpressionSQLBuilder(e: Expression) {
case _ => None
}
}
+
+object GeneralExpression {
+ def apply(expressions: Array[V2Expression], name: String = "UNDEFINED"): GeneralSQLExpression =
+ new GeneralSQLExpression(name, expressions)
Review comment:
I meant that removing this `object`, and dealing with this default argument in the constructor of `GeneralExpression` at the Java file.
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r814544775
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java
##########
@@ -0,0 +1,86 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.expressions;
+
+import java.io.Serializable;
+import java.util.Arrays;
+
+import org.apache.spark.annotation.Evolving;
+import org.apache.spark.sql.connector.util.V2ExpressionSQLBuilder;
+
+// scalastyle:off line.size.limit
+/**
+ * The general representation of SQL scalar expressions, which contains the upper-cased
+ * expression name and all the children expressions.
+ * <p>
+ * The currently supported SQL scalar expressions:
+ * <ol>
+ * <li><pre>Expression name: IS_NULL</pre><pre>SQL scalar expression: `expr IS NULL`</pre> ANSI enabled: No, Since 3.3.0</li>
Review comment:
how about
```
Name: IS_NULL SQL semantic: `child IS NULL` Version: 3.3.0
```
--
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r813522504
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java
##########
@@ -20,22 +20,42 @@
import java.io.Serializable;
import org.apache.spark.annotation.Evolving;
+import org.apache.spark.sql.connector.util.V2ExpressionSQLBuilder;
/**
- * The general SQL string corresponding to expression.
+ * The general representation of SQL scalar expressions, which contains the upper-cased
+ * expression name and all the children expressions.
*
Review comment:
Why do we remove the classdoc? Without the doc, how can data source developers match different expressions?
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r816003535
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java
##########
@@ -0,0 +1,133 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.util;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.spark.sql.connector.expressions.Expression;
+import org.apache.spark.sql.connector.expressions.FieldReference;
+import org.apache.spark.sql.connector.expressions.GeneralScalarExpression;
+import org.apache.spark.sql.connector.expressions.LiteralValue;
+
+/**
+ * The builder to generate SQL from V2 expressions.
+ */
+public class V2ExpressionSQLBuilder {
+ public String build(Expression expr) {
+ if (expr instanceof LiteralValue) {
+ return visitLiteral((LiteralValue) expr);
+ } else if (expr instanceof FieldReference) {
+ return visitFieldReference((FieldReference) expr);
+ } else if (expr instanceof GeneralScalarExpression) {
+ GeneralScalarExpression e = (GeneralScalarExpression) expr;
+ String name = e.name();
+ switch (name) {
+ case "IS_NULL":
+ return visitIsNull(build(e.children()[0]));
+ case "IS_NOT_NULL":
+ return visitIsNotNull(build(e.children()[0]));
+ case "=":
+ case "!=":
+ case "<=>":
+ case "<":
+ case "<=":
+ case ">":
+ case ">=":
+ case "+":
+ case "-":
+ case "*":
+ case "/":
+ case "%":
+ case "&&":
+ case "||":
+ case "AND":
+ case "OR":
+ case "&":
+ case "|":
+ case "^":
+ return visitBinaryOperation(name, build(e.children()[0]), build(e.children()[1]));
+ case "NOT":
+ return visitNot(build(e.children()[0]));
+ case "CASE_WHEN":
+ List<String> children = new ArrayList<>();
+ for (Expression child : e.children()) {
+ children.add(build(child));
+ }
+ return visitCaseWhen(children.toArray(new String[e.children().length]));
+ // TODO supports other expressions
Review comment:
Actually we should exclude it, as it's not a SQL standard.
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r816002530
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java
##########
@@ -0,0 +1,133 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.util;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.spark.sql.connector.expressions.Expression;
+import org.apache.spark.sql.connector.expressions.FieldReference;
+import org.apache.spark.sql.connector.expressions.GeneralScalarExpression;
+import org.apache.spark.sql.connector.expressions.LiteralValue;
+
+/**
+ * The builder to generate SQL from V2 expressions.
+ */
+public class V2ExpressionSQLBuilder {
+ public String build(Expression expr) {
+ if (expr instanceof LiteralValue) {
+ return visitLiteral((LiteralValue) expr);
+ } else if (expr instanceof FieldReference) {
+ return visitFieldReference((FieldReference) expr);
+ } else if (expr instanceof GeneralScalarExpression) {
+ GeneralScalarExpression e = (GeneralScalarExpression) expr;
+ String name = e.name();
+ switch (name) {
+ case "IS_NULL":
+ return visitIsNull(build(e.children()[0]));
+ case "IS_NOT_NULL":
+ return visitIsNotNull(build(e.children()[0]));
+ case "=":
+ case "!=":
+ case "<=>":
+ case "<":
+ case "<=":
+ case ">":
+ case ">=":
Review comment:
can we have a bit more visit methods? `visitBinaryComparison`, `visitBinaryArithmetic`. We should also have `visitAnd` and `visitOr` since there is a `visitNot`.
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r818340285
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/jdbc/H2Dialect.scala
##########
@@ -22,12 +22,33 @@ import java.util.Locale
import org.apache.spark.sql.AnalysisException
import org.apache.spark.sql.catalyst.analysis.{NoSuchNamespaceException, NoSuchTableException, TableAlreadyExistsException}
+import org.apache.spark.sql.connector.expressions.{Expression, FieldReference}
import org.apache.spark.sql.connector.expressions.aggregate.{AggregateFunc, GeneralAggregateFunc}
+import org.apache.spark.sql.connector.util.V2ExpressionSQLBuilder
private object H2Dialect extends JdbcDialect {
override def canHandle(url: String): Boolean =
url.toLowerCase(Locale.ROOT).startsWith("jdbc:h2")
+ class H2SQLBuilder extends V2ExpressionSQLBuilder {
+ override def visitFieldReference(fieldRef: FieldReference): String = {
+ if (fieldRef.fieldNames().length != 1) {
+ throw new IllegalArgumentException(
+ "FieldReference with field name has multiple or zero parts unsupported: " + fieldRef);
Review comment:
Is it only a limitation of H2? I think it's a limitation for all JDBC dialects today, and it's better to put this special sql builder in [JdbcDialects.scala](https://github.com/apache/spark/pull/35494/files#diff-1533255ad629a18e883f8186b303fdf4fae99043551ce20c5b5ea06d085e0b14)
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r818650568
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.expressions.{Add, And, Attribute, BinaryComparison, BinaryOperator, BitwiseAnd, BitwiseNot, BitwiseOr, BitwiseXor, CaseWhen, Divide, EqualTo, Expression, IsNotNull, IsNull, Literal, Multiply, Not, Or, Remainder, Subtract, UnaryMinus}
+import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference, GeneralScalarExpression, LiteralValue}
+
+/**
+ * The builder to generate V2 expressions from catalyst expressions.
+ */
+class V2ExpressionBuilder(e: Expression) {
+
+ def build(): Option[V2Expression] = generateExpression(e)
+
+ private def canTranslate(b: BinaryOperator) = b match {
+ case _: And | _: Or => true
+ case _: BinaryComparison => true
+ case _: BitwiseAnd | _: BitwiseOr | _: BitwiseXor => true
+ case add: Add => add.failOnError
+ case sub: Subtract => sub.failOnError
+ case mul: Multiply => mul.failOnError
+ case div: Divide => div.failOnError
+ case r: Remainder => r.failOnError
+ case _ => false
+ }
+
+ private def generateExpression(expr: Expression): Option[V2Expression] = expr match {
+ case Literal(value, dataType) => Some(LiteralValue(value, dataType))
+ case attr: Attribute => Some(FieldReference.column(quoteIfNeeded(attr.name)))
Review comment:
This is wrong. `FieldReference` should contain the row string of the column name parts. We should not quote it here.
--
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r813530816
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java
##########
@@ -0,0 +1,129 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.util;
+
+import org.apache.spark.sql.connector.expressions.Expression;
+import org.apache.spark.sql.connector.expressions.FieldReference;
+import org.apache.spark.sql.connector.expressions.GeneralScalarExpression;
+import org.apache.spark.sql.connector.expressions.LiteralValue;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * The builder to generate SQL from V2 expressions.
+ */
+public class V2ExpressionSQLBuilder {
+ public String build(Expression expr) throws Throwable {
Review comment:
```
protected String visitUnexpectedExpr(Expression expr) throws IllegalArgumentException {
throw new IllegalArgumentException("Unexpected V2 expression: " + expr);
}
```
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r816739583
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java
##########
@@ -0,0 +1,165 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.util;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.spark.sql.connector.expressions.Expression;
+import org.apache.spark.sql.connector.expressions.FieldReference;
+import org.apache.spark.sql.connector.expressions.GeneralScalarExpression;
+import org.apache.spark.sql.connector.expressions.LiteralValue;
+
+/**
+ * The builder to generate SQL from V2 expressions.
+ */
+public class V2ExpressionSQLBuilder {
+ public String build(Expression expr) {
+ if (expr instanceof LiteralValue) {
+ return visitLiteral((LiteralValue) expr);
+ } else if (expr instanceof FieldReference) {
+ return visitFieldReference((FieldReference) expr);
+ } else if (expr instanceof GeneralScalarExpression) {
+ GeneralScalarExpression e = (GeneralScalarExpression) expr;
+ String name = e.name();
+ switch (name) {
+ case "IS_NULL":
+ return visitIsNull(build(e.children()[0]));
+ case "IS_NOT_NULL":
+ return visitIsNotNull(build(e.children()[0]));
+ case "=":
+ case "!=":
+ case "<=>":
+ case "<":
+ case "<=":
+ case ">":
+ case ">=":
+ return visitBinaryComparison(name, build(e.children()[0]), build(e.children()[1]));
+ case "+":
+ case "-":
+ case "*":
+ case "/":
+ case "%":
+ return visitBinaryArithmetic(name, build(e.children()[0]), build(e.children()[1]));
+ case "AND":
+ return visitAnd(name, build(e.children()[0]), build(e.children()[1]));
+ case "OR":
+ return visitOr(name, build(e.children()[0]), build(e.children()[1]));
+ case "NOT":
+ return visitNot(build(e.children()[0]));
+ case "&":
+ return visitBitwiseAnd(name, build(e.children()[0]), build(e.children()[1]));
+ case "|":
+ return visitBitwiseOr(name, build(e.children()[0]), build(e.children()[1]));
+ case "^":
+ return visitBitwiseXor(name, build(e.children()[0]), build(e.children()[1]));
Review comment:
These are also binary arithematic
--
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r811217042
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala
##########
@@ -0,0 +1,124 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.expressions.{Attribute, BinaryOperator, CaseWhen, EqualTo, Expression, IsNotNull, IsNull, Literal, Not}
+import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference, GeneralScalarExpression, LiteralValue}
+
+/**
+ * The builder to generate V2 expressions from catalyst expressions.
+ */
+class V2ExpressionBuilder(e: Expression) {
+
+ def build(): Option[V2Expression] = {
+ val expression = generateExpression(e)
+ expression.foreach {
+ case generated: GeneralScalarExpression =>
+ generateSQL(e).foreach(sql => generated.setSql(sql))
+ case _ =>
+ }
+ expression
+ }
+
+ private def generateExpression(expr: Expression): Option[V2Expression] = expr match {
+ case Literal(value, dataType) => Some(LiteralValue(value, dataType))
+ case attr: Attribute => Some(FieldReference.column(quoteIfNeeded(attr.name)))
+ case IsNull(col) => generateExpression(col)
+ .map(c => new GeneralScalarExpression("IS NULL", Array[V2Expression](c)))
+ case IsNotNull(col) => generateExpression(col)
+ .map(c => new GeneralScalarExpression("IS NOT NULL", Array[V2Expression](c)))
+ case b: BinaryOperator =>
+ val left = generateExpression(b.left)
+ val right = generateExpression(b.right)
+ if (left.isDefined && right.isDefined) {
+ Some(new GeneralScalarExpression(b.sqlOperator, Array[V2Expression](left.get, right.get)))
+ } else {
+ None
+ }
+ case Not(eq: EqualTo) =>
+ val left = generateExpression(eq.left)
+ val right = generateExpression(eq.right)
+ if (left.isDefined && right.isDefined) {
+ Some(new GeneralScalarExpression("!=", Array[V2Expression](left.get, right.get)))
+ } else {
+ None
+ }
+ case Not(child) => generateExpression(child)
+ .map(v => new GeneralScalarExpression("NOT", Array[V2Expression](v)))
+ case CaseWhen(branches, elseValue) =>
+ val conditions = branches.map(_._1).flatMap(generateExpression)
+ val values = branches.map(_._2).flatMap(generateExpression)
+ if (conditions.length == branches.length && values.length == branches.length) {
+ val branchExpressions = conditions.zip(values).map { case (c, v) =>
+ new GeneralScalarExpression(Array[V2Expression](c, v))
+ }
+ val branchExpression = new GeneralScalarExpression(branchExpressions.toArray[V2Expression])
+ if (elseValue.isDefined) {
+ elseValue.flatMap(generateExpression).map { v =>
+ new GeneralScalarExpression("CASE WHEN", Array[V2Expression](branchExpression, v))
+ }
+ } else {
+ Some(new GeneralScalarExpression("CASE WHEN", Array[V2Expression](branchExpression)))
+ }
+ } else {
+ None
+ }
+ // TODO supports other expressions
+ case _ => None
+ }
+
+ private def generateSQL(expr: Expression): Option[String] = expr match {
Review comment:
I don't think this helps. What we need is a tool that can help source implementations to easily implement generating SQL string from v2 expressions, and can customize for special cases.
I'd like to see something like this as a public ds v2 API in `org.apache.spark.sql.connector.util`
```
class V2ExpressionSQLBuilder {
pubic String build(expr: V2Expression) {
if (expr instanceof LiteralValue) {
return visitLiteral((LiteralValue) expr);
} else if ...
} else if (expr instanceof GeneralScalarExpression) {
GeneralScalarExpression e = (GeneralScalarExpression) expr;
String name = e.name;
if ("IS_NULL" == name) {
return visitIsNull(build(e.children()[0]));
} else if ...
} else {
return visitUnexpectedExpr(expr);
}
}
protected String visitLiteral...
protected String visitIsNull(String child)...
...
}
```
Then source implementations can use it directly, or extend it and overwrite some `visitXXX` methods for customization.
--
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r811517388
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java
##########
@@ -22,20 +22,46 @@
import org.apache.spark.annotation.Evolving;
/**
- * The general SQL string corresponding to expression.
+ * The general V2 expression corresponding to V1 expression.
+ * <p>
+ * The currently supported expression:
+ * <ol>
+ * <li><pre>CASE WHEN expr1 THEN expr2 [WHEN expr3 THEN expr4]* [ELSE expr5] END</pre>
+ * Since 3.3.0</li>
+ * </ol>
*
* @since 3.3.0
*/
@Evolving
-public class GeneralSQLExpression implements Expression, Serializable {
- private String sql;
+public class GeneralScalarExpression implements Expression, Serializable {
+ private String name;
+ private Expression[] children;
+ private String sql;
Review comment:
I want `PushedAggregates` keeps the SQL.
```
val expected_plan_fragment =
"PushedAggregates: [COUNT(CASE WHEN ((SALARY) > (8000.00)) AND ((SALARY) < (10000.00))" +
" THEN SALARY ELSE 0.00 END), C..., " +
"PushedFilters: [], " +
"PushedGroupByColumns: [DEPT]"
```
--
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r810046024
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceStrategy.scala
##########
@@ -41,7 +41,7 @@ import org.apache.spark.sql.catalyst.streaming.StreamingRelationV2
import org.apache.spark.sql.catalyst.util.ExpressionSQLBuilder
import org.apache.spark.sql.connector.catalog.SupportsRead
import org.apache.spark.sql.connector.catalog.TableCapability._
-import org.apache.spark.sql.connector.expressions.{Expression => ExpressionV2, FieldReference, GeneralSQLExpression, NullOrdering, SortDirection, SortOrder => SortOrderV2, SortValue}
+import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference, NullOrdering, SortDirection, SortOrder => V2SortOrder, SortValue}
Review comment:
Can you give some examples of the "other place"?
--
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 edited a comment on pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan edited a comment on pull request #35494:
URL: https://github.com/apache/spark/pull/35494#issuecomment-1044616306
Instead of adding one v2 expression class for each catalyst expression, I agree it's much simpler to have a general v2 expression class that is a tree structure. (It's better to use SQL string but seems even basic SQL syntaxes diverge in different databases...)
However, we must clearly document the name for each supported v2 expression, similar to `GeneralAggregateFunc`.
For the naming, maybe `GeneralScalarExpression` to differentiate from `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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r811761664
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java
##########
@@ -22,20 +22,46 @@
import org.apache.spark.annotation.Evolving;
/**
- * The general SQL string corresponding to expression.
+ * The general V2 expression corresponding to V1 expression.
+ * <p>
+ * The currently supported expression:
+ * <ol>
+ * <li><pre>CASE WHEN expr1 THEN expr2 [WHEN expr3 THEN expr4]* [ELSE expr5] END</pre>
+ * Since 3.3.0</li>
+ * </ol>
*
* @since 3.3.0
*/
@Evolving
-public class GeneralSQLExpression implements Expression, Serializable {
- private String sql;
+public class GeneralScalarExpression implements Expression, Serializable {
+ private String name;
+ private Expression[] children;
+ private String sql;
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r811622809
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java
##########
@@ -22,20 +22,46 @@
import org.apache.spark.annotation.Evolving;
/**
- * The general SQL string corresponding to expression.
+ * The general V2 expression corresponding to V1 expression.
+ * <p>
+ * The currently supported expression:
+ * <ol>
+ * <li><pre>CASE WHEN expr1 THEN expr2 [WHEN expr3 THEN expr4]* [ELSE expr5] END</pre>
+ * Since 3.3.0</li>
+ * </ol>
*
* @since 3.3.0
*/
@Evolving
-public class GeneralSQLExpression implements Expression, Serializable {
- private String sql;
+public class GeneralScalarExpression implements Expression, Serializable {
+ private String name;
+ private Expression[] children;
+ private String sql;
Review comment:
setting SQL string is pretty ugly. Can't we just implement the `sql` method in this class with `V2ExpressionSQLBuilder`?
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r816740939
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala
##########
@@ -0,0 +1,91 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.expressions.{Add, Attribute, BinaryOperator, BitwiseNot, CaseWhen, Divide, EqualTo, Expression, IntegralDivide, IsNotNull, IsNull, Literal, Multiply, Not, Pmod, Remainder, Subtract}
+import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference, GeneralScalarExpression, LiteralValue}
+
+/**
+ * The builder to generate V2 expressions from catalyst expressions.
+ */
+class V2ExpressionBuilder(e: Expression) {
+
+ def build(): Option[V2Expression] = generateExpression(e)
+
+ private def canTranslate(b: BinaryOperator) = b match {
+ case add: Add => add.failOnError
+ case sub: Subtract => sub.failOnError
+ case mul: Multiply => mul.failOnError
+ case div: Divide => div.failOnError
+ case _: IntegralDivide => false
Review comment:
An allowlist is better, can we list all the supported expressions and put a `case _ => false` at the end?
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r816004560
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala
##########
@@ -0,0 +1,93 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.expressions.{Add, Attribute, BinaryArithmetic, BinaryOperator, BitwiseAnd, BitwiseOr, BitwiseXor, CaseWhen, Divide, EqualTo, Expression, IntegralDivide, IsNotNull, IsNull, Literal, Multiply, Not, Pmod, Remainder, Subtract}
+import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference, GeneralScalarExpression, LiteralValue}
+
+/**
+ * The builder to generate V2 expressions from catalyst expressions.
+ */
+class V2ExpressionBuilder(e: Expression) {
+
+ def build(): Option[V2Expression] = generateExpression(e)
+
+ private def canTranslate(b: BinaryOperator) = b match {
+ case add: Add => add.failOnError
+ case sub: Subtract => sub.failOnError
+ case mul: Multiply => mul.failOnError
+ case div: Divide => div.failOnError
+ case intDiv: IntegralDivide => intDiv.failOnError
+ case r: Remainder => r.failOnError
+ case p: Pmod => p.failOnError
+ case _: BitwiseAnd => true
+ case _: BitwiseOr => true
+ case _: BitwiseXor => true
+ case _ => false
+ }
+
+ private def generateExpression(expr: Expression): Option[V2Expression] = expr match {
+ case Literal(value, dataType) => Some(LiteralValue(value, dataType))
+ case attr: Attribute => Some(FieldReference.column(quoteIfNeeded(attr.name)))
+ case IsNull(col) => generateExpression(col)
+ .map(c => new GeneralScalarExpression("IS_NULL", Array[V2Expression](c)))
+ case IsNotNull(col) => generateExpression(col)
+ .map(c => new GeneralScalarExpression("IS_NOT_NULL", Array[V2Expression](c)))
+ case b: BinaryOperator
+ if !b.isInstanceOf[BinaryArithmetic] || canTranslate(b) =>
Review comment:
we only need `canTranslate(b)` 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] cloud-fan commented on a change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r815997727
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java
##########
@@ -0,0 +1,213 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.expressions;
+
+import java.io.Serializable;
+import java.util.Arrays;
+
+import org.apache.spark.annotation.Evolving;
+import org.apache.spark.sql.connector.util.V2ExpressionSQLBuilder;
+
+// scalastyle:off line.size.limit
+/**
+ * The general representation of SQL scalar expressions, which contains the upper-cased
+ * expression name and all the children expressions.
+ * <p>
+ * The currently supported SQL scalar expressions:
+ * <ol>
+ * <li>Name: IS_NULL
+ * <ul>
+ * <li>SQL semantic: `expr IS NULL`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: IS_NOT_NULL
+ * <ul>
+ * <li>SQL semantic: `expr IS NOT NULL`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: =
+ * <ul>
+ * <li>SQL semantic: `expr1 = expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: !=
+ * <ul>
+ * <li>SQL semantic: `expr1 != expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <>
+ * <ul>
+ * <li>SQL semantic: `expr1 <> expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <=>
+ * <ul>
+ * <li>SQL semantic: `expr1 <=> expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <
+ * <ul>
+ * <li>SQL semantic: `expr1 < expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: <=
+ * <ul>
+ * <li>SQL semantic: `expr1 <= expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: >
+ * <ul>
+ * <li>SQL semantic: `expr1 > expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: >=
+ * <ul>
+ * <li>SQL semantic: `expr1 >= expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: +
+ * <ul>
+ * <li>SQL semantic: `expr1 + expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: -
+ * <ul>
+ * <li>SQL semantic: `expr1 - expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: *
+ * <ul>
+ * <li>SQL semantic: `expr1 * expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: /
+ * <ul>
+ * <li>SQL semantic: `expr1 / expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: %
+ * <ul>
+ * <li>SQL semantic: `expr1 % expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: pmod
+ * <ul>
+ * <li>SQL semantic: `pmod(expr1, expr2)`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: &&
+ * <ul>
+ * <li>SQL semantic: `expr1 && expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: ||
+ * <ul>
+ * <li>SQL semantic: `expr1 || expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: AND
+ * <ul>
+ * <li>SQL semantic: `expr1 AND expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: OR
+ * <ul>
+ * <li>SQL semantic: `expr1 OR expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: &
+ * <ul>
+ * <li>SQL semantic: `expr1 & expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: |
+ * <ul>
+ * <li>SQL semantic: `expr1 | expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: ^
+ * <ul>
+ * <li>SQL semantic: `expr1 ^ expr2`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: NOT
+ * <ul>
+ * <li>SQL semantic: `NOT expr`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
+ * <li>Name: CASE_WHEN
+ * <ul>
+ * <li>SQL semantic: `CASE WHEN expr1 THEN expr2 [WHEN expr3 THEN expr4]* [ELSE expr5] END`</li>
+ * <li>Since version: 3.3.0</li>
+ * </ul>
+ * </li>
Review comment:
how about bit-wise NOT `~`? can we double-check if all the supported expressions are listed here?
--
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r811180944
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala
##########
@@ -0,0 +1,124 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.expressions.{Attribute, BinaryOperator, CaseWhen, EqualTo, Expression, IsNotNull, IsNull, Literal, Not}
+import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference, GeneralScalarExpression, LiteralValue}
+
+/**
+ * The builder to generate V2 expressions from catalyst expressions.
+ */
+class V2ExpressionBuilder(e: Expression) {
+
+ def build(): Option[V2Expression] = {
+ val expression = generateExpression(e)
+ expression.foreach {
+ case generated: GeneralScalarExpression =>
+ generateSQL(e).foreach(sql => generated.setSql(sql))
+ case _ =>
+ }
+ expression
+ }
+
+ private def generateExpression(expr: Expression): Option[V2Expression] = expr match {
+ case Literal(value, dataType) => Some(LiteralValue(value, dataType))
+ case attr: Attribute => Some(FieldReference.column(quoteIfNeeded(attr.name)))
+ case IsNull(col) => generateExpression(col)
+ .map(c => new GeneralScalarExpression("IS NULL", Array[V2Expression](c)))
Review comment:
and we should document it clearly in `GeneralScalarExpression`, otherwise source implementations don't know what expression name to match.
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r818355496
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.expressions.{Add, Attribute, BinaryArithmetic, BinaryOperator, BitwiseNot, CaseWhen, Divide, EqualTo, Expression, IsNotNull, IsNull, Literal, Multiply, Not, Remainder, Subtract, UnaryMinus}
+import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference, GeneralScalarExpression, LiteralValue}
+
+/**
+ * The builder to generate V2 expressions from catalyst expressions.
+ */
+class V2ExpressionBuilder(e: Expression) {
+
+ def build(): Option[V2Expression] = generateExpression(e)
+
+ private def canTranslate(b: BinaryOperator) = b match {
+ case add: Add => add.failOnError
+ case sub: Subtract => sub.failOnError
+ case mul: Multiply => mul.failOnError
+ case div: Divide => div.failOnError
+ case r: Remainder => r.failOnError
+ case o: BinaryOperator => !o.isInstanceOf[BinaryArithmetic]
Review comment:
How about?
```
private def canTranslate(b: BinaryOperator) = b match {
case _: And | _: Or => true
case _: BinaryComparison => true
case _: BitwiseAnd | _: BitwiseOr | _: BitwiseXor => true
case add: Add => add.failOnError
case sub: Subtract => sub.failOnError
case mul: Multiply => mul.failOnError
case div: Divide => div.failOnError
case r: Remainder => r.failOnError
case _ => false
}
```
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r814547531
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala
##########
@@ -0,0 +1,93 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.expressions.{Add, Attribute, BinaryArithmetic, BinaryOperator, BitwiseAnd, BitwiseOr, BitwiseXor, CaseWhen, Divide, EqualTo, Expression, IntegralDivide, IsNotNull, IsNull, Literal, Multiply, Not, Pmod, Remainder, Subtract}
+import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference, GeneralScalarExpression, LiteralValue}
+
+/**
+ * The builder to generate V2 expressions from catalyst expressions.
+ */
+class V2ExpressionBuilder(e: Expression) {
+
+ def build(): Option[V2Expression] = generateExpression(e)
+
+ private def buildEnabled(b: BinaryArithmetic) = b match {
Review comment:
Can this function take `BinaryOperator`? Then we can simplify the caller-side code.
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r814546156
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala
##########
@@ -0,0 +1,93 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.expressions.{Add, Attribute, BinaryArithmetic, BinaryOperator, BitwiseAnd, BitwiseOr, BitwiseXor, CaseWhen, Divide, EqualTo, Expression, IntegralDivide, IsNotNull, IsNull, Literal, Multiply, Not, Pmod, Remainder, Subtract}
+import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference, GeneralScalarExpression, LiteralValue}
+
+/**
+ * The builder to generate V2 expressions from catalyst expressions.
+ */
+class V2ExpressionBuilder(e: Expression) {
+
+ def build(): Option[V2Expression] = generateExpression(e)
+
+ private def buildEnabled(b: BinaryArithmetic) = b match {
Review comment:
```suggestion
private def canTranslate(b: BinaryArithmetic) = b match {
```
--
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 change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r813583648
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/connector/V1ReadFallbackSuite.scala
##########
@@ -129,8 +127,8 @@ class TableWithV1ReadFallback(override val name: String) extends Table with Supp
override def schema(): StructType = V1ReadFallbackCatalog.schema
- override def capabilities(): util.Set[TableCapability] = {
- util.EnumSet.of(TableCapability.BATCH_READ)
+ override def capabilities(): java.util.Set[TableCapability] = {
+ java.util.EnumSet.of(TableCapability.BATCH_READ)
Review comment:
compile error. cannot find `Set` in `org.apache.spark.sql.connector.util`.
--
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 #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on pull request #35494:
URL: https://github.com/apache/spark/pull/35494#issuecomment-1059164602
@cloud-fan Thank you for your hard work! @HyukjinKwon Thank you for your review. @baibaichen @xkrogen Thank you too.
--
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 #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #35494:
URL: https://github.com/apache/spark/pull/35494#issuecomment-1059159966
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r818650568
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.expressions.{Add, And, Attribute, BinaryComparison, BinaryOperator, BitwiseAnd, BitwiseNot, BitwiseOr, BitwiseXor, CaseWhen, Divide, EqualTo, Expression, IsNotNull, IsNull, Literal, Multiply, Not, Or, Remainder, Subtract, UnaryMinus}
+import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference, GeneralScalarExpression, LiteralValue}
+
+/**
+ * The builder to generate V2 expressions from catalyst expressions.
+ */
+class V2ExpressionBuilder(e: Expression) {
+
+ def build(): Option[V2Expression] = generateExpression(e)
+
+ private def canTranslate(b: BinaryOperator) = b match {
+ case _: And | _: Or => true
+ case _: BinaryComparison => true
+ case _: BitwiseAnd | _: BitwiseOr | _: BitwiseXor => true
+ case add: Add => add.failOnError
+ case sub: Subtract => sub.failOnError
+ case mul: Multiply => mul.failOnError
+ case div: Divide => div.failOnError
+ case r: Remainder => r.failOnError
+ case _ => false
+ }
+
+ private def generateExpression(expr: Expression): Option[V2Expression] = expr match {
+ case Literal(value, dataType) => Some(LiteralValue(value, dataType))
+ case attr: Attribute => Some(FieldReference.column(quoteIfNeeded(attr.name)))
Review comment:
This is wrong. `FieldReference` should contain the raw string of the column name parts. We should not quote it here.
--
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 change in pull request #35494: [SPARK-38196][SQL] Refactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r818647659
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java
##########
@@ -0,0 +1,201 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.expressions;
+
+import java.io.Serializable;
+import java.util.Arrays;
+
+import org.apache.spark.annotation.Evolving;
+import org.apache.spark.sql.connector.util.V2ExpressionSQLBuilder;
+
+// scalastyle:off line.size.limit
+/**
+ * The general representation of SQL scalar expressions, which contains the upper-cased
+ * expression name and all the children expressions.
+ * <p>
+ * The currently supported SQL scalar expressions:
+ * <ol>
+ * <li>Name: IS_NULL
Review comment:
To make it pretty, we can wrap the name with `<code>`: `<code>IS_NULL</code>`
--
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] HyukjinKwon commented on a change in pull request #35494: [SPARK-38196][SQL] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r806387895
##########
File path: sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralSQLExpression.java
##########
@@ -22,20 +22,34 @@
import org.apache.spark.annotation.Evolving;
/**
- * The general SQL string corresponding to expression.
+ * The general V2 expression corresponding to V1 expression.
*
* @since 3.3.0
*/
@Evolving
public class GeneralSQLExpression implements Expression, Serializable {
Review comment:
Do you meant that we will use this for all V2 expressions? I thought this is one of the optional APIs.
--
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 change in pull request #35494: [SPARK-37960][SQL][FOLLOWUP] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r805454676
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/ExpressionSQLBuilder.scala
##########
@@ -67,3 +122,8 @@ class ExpressionSQLBuilder(e: Expression) {
case _ => None
}
}
+
+object GeneralExpression {
+ def apply(expressions: Array[V2Expression], name: String = "UNDEFINED"): GeneralSQLExpression =
+ new GeneralSQLExpression(name, expressions)
Review comment:
I'm sorry. I don't understand what you mean.
--
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 #35494: [SPARK-37960][SQL][FOLLOWUP] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
beliefer commented on pull request #35494:
URL: https://github.com/apache/spark/pull/35494#issuecomment-1038539857
> @beliefer this PR adds a new API. Can we file a new JIRA?
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] HyukjinKwon commented on a change in pull request #35494: [SPARK-37960][SQL][FOLLOWUP] Reactor framework so as JDBC dialect could compile expression by self way
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35494:
URL: https://github.com/apache/spark/pull/35494#discussion_r805224372
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/ExpressionSQLBuilder.scala
##########
@@ -67,3 +122,8 @@ class ExpressionSQLBuilder(e: Expression) {
case _ => None
}
}
+
+object GeneralExpression {
+ def apply(expressions: Array[V2Expression], name: String = "UNDEFINED"): GeneralSQLExpression =
+ new GeneralSQLExpression(name, expressions)
Review comment:
Can we deal with this in GeneralExpression's constructor?
--
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