beliefer commented on code in PR #36663: URL: https://github.com/apache/spark/pull/36663#discussion_r903433839
########## sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/Extract.java: ########## @@ -0,0 +1,57 @@ +/* + * 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 org.apache.spark.annotation.Evolving; + +import java.io.Serializable; + +/** + * The general representation of extract expressions, which contains the upper-cased expression + * name and all the children expressions. Please also see {@link Extract} + * for the supported extract expressions. + * <p> + * The currently supported predicate expressions: + * <ol + * <li>Name: <code>EXTRACT</code> + * <ul> + * <li>SQL semantic: <code>EXTRACT(field FROM source)</code></li> + * <li>Since version: 3.4.0</li> + * </ul> + * </li> + * </ol> + * @since 3.4.0 + */ + +@Evolving +public class Extract implements Expression, Serializable { + + private Expression expression; + private String field; + + public Extract(Expression expression, String field) { + this.expression = expression; + this.field = field; + } + + public Expression expression() { return expression; } + public String field() { return field; } Review Comment: ```suggestion private String field; private Expression source; public Extract(String field, Expression source) { this.field = field; this.source = source; } public String field() { return field; } public Expression source() { return source; } ``` ########## sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/Extract.java: ########## @@ -0,0 +1,57 @@ +/* + * 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 org.apache.spark.annotation.Evolving; + +import java.io.Serializable; + +/** + * The general representation of extract expressions, which contains the upper-cased expression + * name and all the children expressions. Please also see {@link Extract} + * for the supported extract expressions. + * <p> + * The currently supported predicate expressions: + * <ol + * <li>Name: <code>EXTRACT</code> + * <ul> + * <li>SQL semantic: <code>EXTRACT(field FROM source)</code></li> Review Comment: Please remove these comments. ########## sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/Extract.java: ########## @@ -0,0 +1,57 @@ +/* + * 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 org.apache.spark.annotation.Evolving; + +import java.io.Serializable; + +/** + * The general representation of extract expressions, which contains the upper-cased expression + * name and all the children expressions. Please also see {@link Extract} Review Comment: `Represent an extract expression, which contains a field to be extracted and a source expression where the field should be extracted.` ########## sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java: ########## @@ -43,6 +44,10 @@ public String build(Expression expr) { } else if (expr instanceof Cast) { Cast cast = (Cast) expr; return visitCast(build(cast.expression()), cast.dataType()); + } else if (expr instanceof Extract) { + Extract e = (Extract) expr; + return visitExtract(e.field(), + Arrays.stream(e.children()).map(c -> build(c)).toArray(String[]::new)); Review Comment: ```suggestion Extract extract = (Extract) expr; return visitExtract(extract.field(), build(extract.source())); ``` ########## sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala: ########## @@ -262,6 +262,55 @@ class V2ExpressionBuilder( } else { None } + case date: DateAdd => + val childrenExpressions = date.children.flatMap(generateExpression(_)) + if (childrenExpressions.length == date.children.length) { + Some(new GeneralScalarExpression("DATE_ADD", childrenExpressions.toArray[V2Expression])) + } else { + None + } + case date: DateDiff => + val childrenExpressions = date.children.flatMap(generateExpression(_)) + if (childrenExpressions.length == date.children.length) { + Some(new GeneralScalarExpression("DATE_DIFF", childrenExpressions.toArray[V2Expression])) + } else { + None + } + case date: TruncDate => + val childrenExpressions = date.children.flatMap(generateExpression(_)) + if (childrenExpressions.length == date.children.length) { + Some(new GeneralScalarExpression("TRUNC", childrenExpressions.toArray[V2Expression])) + } else { + None + } + case Second(child, _) => + generateExpression(child).map(v => new V2Extract(v, "SECOND")) Review Comment: ```suggestion generateExpression(child).map(v => new V2Extract("SECOND", v)) ``` ########## sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/Extract.java: ########## @@ -0,0 +1,57 @@ +/* + * 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 org.apache.spark.annotation.Evolving; + +import java.io.Serializable; + +/** + * The general representation of extract expressions, which contains the upper-cased expression + * name and all the children expressions. Please also see {@link Extract} + * for the supported extract expressions. + * <p> + * The currently supported predicate expressions: + * <ol + * <li>Name: <code>EXTRACT</code> + * <ul> + * <li>SQL semantic: <code>EXTRACT(field FROM source)</code></li> + * <li>Since version: 3.4.0</li> + * </ul> + * </li> + * </ol> + * @since 3.4.0 + */ + +@Evolving +public class Extract implements Expression, Serializable { + + private Expression expression; + private String field; + + public Extract(Expression expression, String field) { + this.expression = expression; + this.field = field; + } + + public Expression expression() { return expression; } + public String field() { return field; } + + @Override + public Expression[] children() { return new Expression[]{ expression() }; } Review Comment: ```suggestion public Expression[] children() { return new Expression[]{ source() }; } ``` ########## sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java: ########## @@ -265,4 +273,8 @@ protected String visitTrim(String direction, String[] inputs) { return "TRIM(" + direction + " " + inputs[1] + " FROM " + inputs[0] + ")"; } } + + protected String visitExtract(String field, String[] inputs) { + return "EXTRACT(" + field + " FROM " + inputs[0] + ")"; + } Review Comment: ```suggestion protected String visitExtract(String field, String source) { return "EXTRACT(" + field + " FROM " + source + ")"; } ``` ########## sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala: ########## @@ -270,6 +270,17 @@ abstract class JdbcDialect extends Serializable with Logging{ s"${this.getClass.getSimpleName} does not support function: TRIM") } } + + override def visitExtract(field: String, inputs: Array[String]): String = { + if (isSupportedFunction(field)) { + super.visitExtract(field, inputs) + } else { + // The framework will catch the error and give up the push-down. + // Please see `JdbcDialect.compileExpression(expr: Expression)` for more details. + throw new UnsupportedOperationException( + s"${this.getClass.getSimpleName} does not support function: EXTRACT") + } + } Review Comment: ```suggestion override def visitExtract(field: String, source: String): String = { if (isSupportedFunction(field)) { super.visitExtract(field, source) } else { // The framework will catch the error and give up the push-down. // Please see `JdbcDialect.compileExpression(expr: Expression)` for more details. throw new UnsupportedOperationException( s"${this.getClass.getSimpleName} does not support function: EXTRACT") } } ``` ########## sql/core/src/main/scala/org/apache/spark/sql/jdbc/H2Dialect.scala: ########## @@ -121,4 +124,30 @@ private[sql] object H2Dialect extends JdbcDialect { } super.classifyException(message, e) } + + override def compileExpression(expr: Expression): Option[String] = { + val jdbcSQLBuilder = new H2JDBCSQLBuilder() + try { + Some(jdbcSQLBuilder.build(expr)) + } catch { + case NonFatal(e) => + logWarning("Error occurs while compiling V2 expression", e) + None + } + } + + class H2JDBCSQLBuilder extends JDBCSQLBuilder { + + override def visitExtract(field: String, inputs: Array[String]): String = { + field match { + case "DAY_OF_MONTH" => + "EXTRACT(DAY FROM " + inputs.mkString(",") + ")" + case "WEEK_OF_YEAR" => + "EXTRACT(ISO_WEEK FROM " + inputs.mkString(",") + ")" + case "YEAR_OF_WEEK" => + "EXTRACT(ISO_WEEK_YEAR FROM " + inputs.mkString(",") + ")" + case _ => super.visitSQLFunction(field, inputs) + } + } Review Comment: ```suggestion override def visitExtract(field: String, source: String): String = { field match { case "DAY_OF_MONTH" => "EXTRACT(DAY FROM " + source + ")" case "WEEK_OF_YEAR" => "EXTRACT(ISO_WEEK FROM " + source + ")" case "YEAR_OF_WEEK" => "EXTRACT(ISO_WEEK_YEAR FROM " + source + ")" case _ => super.visitExtract(field, source) } } ``` ########## sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala: ########## @@ -262,6 +262,55 @@ class V2ExpressionBuilder( } else { None } + case date: DateAdd => + val childrenExpressions = date.children.flatMap(generateExpression(_)) + if (childrenExpressions.length == date.children.length) { + Some(new GeneralScalarExpression("DATE_ADD", childrenExpressions.toArray[V2Expression])) + } else { + None + } + case date: DateDiff => + val childrenExpressions = date.children.flatMap(generateExpression(_)) + if (childrenExpressions.length == date.children.length) { + Some(new GeneralScalarExpression("DATE_DIFF", childrenExpressions.toArray[V2Expression])) + } else { + None + } + case date: TruncDate => + val childrenExpressions = date.children.flatMap(generateExpression(_)) + if (childrenExpressions.length == date.children.length) { + Some(new GeneralScalarExpression("TRUNC", childrenExpressions.toArray[V2Expression])) + } else { + None + } + case Second(child, _) => + generateExpression(child).map(v => new V2Extract(v, "SECOND")) Review Comment: Update other places 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
