You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "dtenedor (via GitHub)" <gi...@apache.org> on 2023/05/16 22:29:06 UTC
[GitHub] [spark] dtenedor opened a new pull request, #41191: [SPARK-43529][SQL] Support general expressions as OPTIONS values in the parser
dtenedor opened a new pull request, #41191:
URL: https://github.com/apache/spark/pull/41191
### What changes were proposed in this pull request?
This PR updates the SQL parser to support general expressions in the syntax for OPTIONS values, rather than restricting to a few types of literals only.
* For now, the `AstBuilder` manually checks that the provided expressions are in fact literals, and throws an error message otherwise.
* In the future, we can expand to support other types of expressions as well.
* In the interim, the error message that users encounter in these cases improves from a general "syntax error at or near <location>" to instead indicate that the syntax is valid, but only literals are supported in these contexts.
### Why are the changes needed?
This makes it possible to allow more features in this area in the future, such as accepting constant but non-literal expression trees and then constant-folding them.
### Does this PR introduce _any_ user-facing change?
Yes, the SQL syntax changes.
### How was this patch tested?
This PR adds new unit test coverage.
--
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] gengliangwang commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1207432514
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveTableSpec.scala:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.analysis
+
+import org.apache.spark.SparkThrowable
+import org.apache.spark.sql.catalyst.expressions.{Expression, Literal}
+import org.apache.spark.sql.catalyst.plans.logical._
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.TreePattern.COMMAND
+import org.apache.spark.sql.errors.QueryCompilationErrors
+
+/**
+ * This rule is responsible for matching against unresolved table specifications in commands with
+ * OPTIONS lists. The parser produces such lists as maps from strings to unresolved expressions.
+ * After otherwise resolving such expressions in the analyzer, this rule converts them to resolved
+ * table specifications wherein these OPTIONS list values are represented as strings instead, for
+ * convenience. The 'resolveOption' is for resolving function calls within each table option.
+ */
+case class ResolveTableSpec(resolveOption: Expression => Expression) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ plan.resolveOperatorsWithPruning(_.containsAnyPattern(COMMAND), ruleId) {
+ case t @ CreateTable(_, _, _, u: UnresolvedTableSpec, _) =>
Review Comment:
nit: we can write this as
```
case t: CreateTable if t.tableSpec.isInstanceOf[UnresolvedTableSpec]
```
so that we don't need to take care of the pattern match here when `CreateTable` is refactored.
Same for the belows.
--
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] gengliangwang commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1217099067
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -158,30 +158,37 @@ class ResolveSessionCatalog(val catalogManager: CatalogManager)
// For CREATE TABLE [AS SELECT], we should use the v1 command if the catalog is resolved to the
// session catalog and the table provider is not v2.
- case c @ CreateTable(ResolvedV1Identifier(ident), _, _, _, _) =>
+ case c @ CreateTable(ResolvedV1Identifier(ident), _, _, u: UnresolvedTableSpec, _,
Review Comment:
QQ: why separate UnresolvedOptionsList from UnresolvedTableSpec? Now the rule has to match both UnresolvedTableSpec and UnresolvedOptionsList.
--
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] dtenedor commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1221882136
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveTableSpec.scala:
##########
@@ -0,0 +1,89 @@
+/*
+ * 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.analysis
+
+import org.apache.spark.SparkThrowable
+import org.apache.spark.sql.catalyst.expressions.{Expression, Literal}
+import org.apache.spark.sql.catalyst.plans.logical._
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.TreePattern.COMMAND
+import org.apache.spark.sql.errors.QueryCompilationErrors
+import org.apache.spark.sql.types.{ArrayType, MapType, StructType}
+
+/**
+ * This object is responsible for processing unresolved table specifications in commands with
+ * OPTIONS lists. The parser produces such lists as maps from strings to unresolved expressions.
+ * After otherwise resolving such expressions in the analyzer, here we convert them to resolved
+ * table specifications wherein these OPTIONS list values are represented as strings instead, for
+ * convenience.
+ */
+object ResolveTableSpec extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ plan.resolveOperatorsWithPruning(_.containsAnyPattern(COMMAND), ruleId) {
+ case t: CreateTable =>
+ resolveTableSpec(t, s => t.copy(tableSpec = s))
+ case t: CreateTableAsSelect =>
+ resolveTableSpec(t, s => t.copy(tableSpec = s))
+ case t: ReplaceTable =>
+ resolveTableSpec(t, s => t.copy(tableSpec = s))
+ case t: ReplaceTableAsSelect =>
+ resolveTableSpec(t, s => t.copy(tableSpec = s))
+ }
+ }
+
+ /** Helper method to resolve the table specification within a logical plan. */
+ private def resolveTableSpec(
+ t: HasTableSpec, withNewSpec: TableSpec => LogicalPlan): LogicalPlan = t.tableSpec match {
+ case u: UnresolvedTableSpec if t.unresolvedOptionsList.allOptionsResolved =>
+ val newOptions: Seq[(String, String)] = t.unresolvedOptionsList.options.map {
+ case (key: String, null) =>
+ (key, null)
+ case (key: String, value: Expression) =>
+ val newValue: String = try {
+ val dt = value.dataType
+ value match {
+ case Literal(null, _) =>
+ "null"
Review Comment:
It should be "null", this part computes the string value for the table option.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -437,6 +437,12 @@ trait V2CreateTablePlan extends LogicalPlan {
def withPartitioning(rewritten: Seq[Transform]): V2CreateTablePlan
}
+/** A trait used for logical plan nodes that create or replace new table specifications. */
+trait HasTableSpec extends LogicalPlan {
+ def tableSpec: TableSpec
+ def unresolvedOptionsList: UnresolvedOptionsList
Review Comment:
Good point, I renamed this to `optionsListExpressions` to clarify.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -437,6 +437,12 @@ trait V2CreateTablePlan extends LogicalPlan {
def withPartitioning(rewritten: Seq[Transform]): V2CreateTablePlan
}
+/** A trait used for logical plan nodes that create or replace new table specifications. */
+trait HasTableSpec extends LogicalPlan {
Review Comment:
I had originally created the trait to help with matching in the `ResolveTableSpec` rule. It's not strictly needed; I removed the trait now and updated the rule to inspect relevant fields instead.
I originally made `UnresolvedOptionsList` part of `UnresolvedTableSpec` :) but you suggested moving it out, which was helpful:
--
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] dtenedor commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as OPTIONS values in the parser
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1199511465
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -3187,7 +3189,42 @@ class AstBuilder extends SqlBaseParserBaseVisitor[AnyRef] with SQLConfHelper wit
ctx: PropertyListContext): Map[String, String] = withOrigin(ctx) {
val properties = ctx.property.asScala.map { property =>
val key = visitPropertyKey(property.key)
- val value = visitPropertyValue(property.value)
+ // If the expression provided for the option value is a literal, use the string
+ // representation of the contents of the literal value provided here. Otherwise, if this
+ // expression is constant but non-literal, evaluate the expression and use its value instead.
+ val value = if (property.value == null) {
+ null
+ } else {
+ val parsed = expression(property.value)
+ def error: Throwable = {
+ new ParseException(
+ errorClass = "INVALID_SQL_SYNTAX",
+ messageParameters = Map(
+ "inputString" ->
+ s"option or property key $key is invalid; only constant expressions are supported"),
+ ctx)
+ }
+ val plan = try {
+ val analyzer: Analyzer = ResolveDefaultColumns.DefaultColumnAnalyzer
Review Comment:
Good point. I updated this to avoid using the analyzer, and just check if the expression is foldable and `eval` it instead. This supports all types of literals and some basic operations now. Function calls are not supported yet; I can support them later, but that change is more invasive.
--
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] dtenedor commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1215045517
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -1382,11 +1407,59 @@ case class DropIndex(
copy(table = newChild)
}
-case class TableSpec(
+trait TableSpec {
+ def properties: Map[String, String]
+ def provider: Option[String]
+ def location: Option[String]
+ def comment: Option[String]
+ def serde: Option[SerdeInfo]
+ def external: Boolean
+ def withNewLocation(newLocation: Option[String]): TableSpec
+ def treePatternBits: BitSet
+}
+
+case class UnresolvedTableSpec(
+ properties: Map[String, String],
+ provider: Option[String],
+ optionsExpressions: Map[String, Expression],
Review Comment:
Thanks for the good suggestion, this makes it simpler! This 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] dtenedor commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1218746688
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -158,30 +158,37 @@ class ResolveSessionCatalog(val catalogManager: CatalogManager)
// For CREATE TABLE [AS SELECT], we should use the v1 command if the catalog is resolved to the
// session catalog and the table provider is not v2.
- case c @ CreateTable(ResolvedV1Identifier(ident), _, _, _, _) =>
+ case c @ CreateTable(ResolvedV1Identifier(ident), _, _, u: UnresolvedTableSpec, _,
Review Comment:
Sure, I switched it back to using `ResolveTableSpec` as a separate rule.
--
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] dtenedor commented on pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on PR #41191:
URL: https://github.com/apache/spark/pull/41191#issuecomment-1585283990
> LGTM except for the last comment. Sorry for the slow response, I was focusing on another project.
No worries, thanks for the careful reviews :)
--
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] gengliangwang commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1209789661
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveTableSpec.scala:
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.analysis
+
+import org.apache.spark.SparkThrowable
+import org.apache.spark.sql.catalyst.expressions.{Expression, Literal}
+import org.apache.spark.sql.catalyst.plans.logical._
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.TreePattern.COMMAND
+import org.apache.spark.sql.errors.QueryCompilationErrors
+
+/**
+ * This rule is responsible for matching against unresolved table specifications in commands with
+ * OPTIONS lists. The parser produces such lists as maps from strings to unresolved expressions.
+ * After otherwise resolving such expressions in the analyzer, this rule converts them to resolved
+ * table specifications wherein these OPTIONS list values are represented as strings instead, for
+ * convenience. The 'resolveOption' is for resolving function calls within each table option.
+ */
+case class ResolveTableSpec(resolveOption: Expression => Expression) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ plan.resolveOperatorsWithPruning(_.containsAnyPattern(COMMAND), ruleId) {
+ case t: CreateTable if t.tableSpec.isInstanceOf[UnresolvedTableSpec] =>
+ t.copy(tableSpec = resolveTableSpec(t.tableSpec))
+ case t: CreateTableAsSelect if t.tableSpec.isInstanceOf[UnresolvedTableSpec] =>
+ t.copy(tableSpec = resolveTableSpec(t.tableSpec))
+ case t: ReplaceTable if t.tableSpec.isInstanceOf[UnresolvedTableSpec] =>
+ t.copy(tableSpec = resolveTableSpec(t.tableSpec))
+ case t: ReplaceTableAsSelect if t.tableSpec.isInstanceOf[UnresolvedTableSpec] =>
+ t.copy(tableSpec = resolveTableSpec(t.tableSpec))
+ }
+ }
+
+ private def resolveTableSpec(t: TableSpec): TableSpec = {
+ val u = t.asInstanceOf[UnresolvedTableSpec]
+ val newOptions: Map[String, String] = u.optionsExpressions.map {
+ case (key: String, null) =>
+ (key, null)
+ case (key: String, value: Expression) =>
+ val newValue: String = try {
+ resolveOption(value) match {
+ case lit: Literal =>
Review Comment:
Do we need error handling if the resolve result is not literal?
--
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] dtenedor commented on pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on PR #41191:
URL: https://github.com/apache/spark/pull/41191#issuecomment-1578888442
Once again the CI is actually passing; the one failure is flaky/unrelated.
--
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] gengliangwang commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1209789175
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -2406,6 +2407,15 @@ class Analyzer(override val catalogManager: CatalogManager) extends RuleExecutor
}
}
+ /** Helper method to resolve function calls within expressions of TableSpec options. */
+ def ResolveTableSpecOption(expression: Expression): Expression = {
+ val analyzed = executeSameContext(Project(Seq(Alias(expression, "col")()), OneRowRelation()))
+ val folded = ConstantFolding(analyzed)
+ folded match {
+ case Project(Seq(Alias(expression, _)), _) => expression
Review Comment:
Do we need error handling if the pattern doesn't 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] gengliangwang commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1209791087
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala:
##########
@@ -927,9 +927,14 @@ abstract class TreeNode[BaseType <: TreeNode[BaseType]] extends Product with Tre
redactMapString(map.asCaseSensitiveMap().asScala, maxFields)
case map: Map[_, _] =>
redactMapString(map, maxFields)
- case t: TableSpec =>
- t.copy(properties = Utils.redact(t.properties).toMap,
- options = Utils.redact(t.options).toMap) :: Nil
+ case t: UnresolvedTableSpec =>
+ val newOptionsList = Utils.redact(t.optionsExpressions.map { case (key, value) =>
+ (key, value.sql)
+ }).toMap
+ ResolvedTableSpec(
Review Comment:
Why it becomes a `ResolvedTableSpec` 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] dtenedor commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general expressions as OPTIONS values in the parser
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1196991836
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -3187,7 +3189,37 @@ class AstBuilder extends SqlBaseParserBaseVisitor[AnyRef] with SQLConfHelper wit
ctx: PropertyListContext): Map[String, String] = withOrigin(ctx) {
val properties = ctx.property.asScala.map { property =>
val key = visitPropertyKey(property.key)
- val value = visitPropertyValue(property.value)
+ // If the expression provided for the option value is a literal, use the string
+ // representation of the contents of the literal value provided here. Otherwise, if this
+ // expression is constant but non-literal, evaluate the expression and use its value instead.
+ val value = if (property.value == null) {
+ null
+ } else {
+ val parsed = expression(property.value)
+ def error: Throwable = {
+ new ParseException(
+ errorClass = "INVALID_SQL_SYNTAX",
+ messageParameters = Map(
+ "inputString" ->
+ s"option or property key $key is invalid; only constant expressions are supported"),
+ ctx)
+ }
+ val plan = try {
+ val analyzer: Analyzer = ResolveDefaultColumns.DefaultColumnAnalyzer
+ val analyzed = analyzer.execute(Project(Seq(Alias(parsed, "col")()), OneRowRelation()))
+ analyzer.checkAnalysis(analyzed)
+ ConstantFolding(analyzed)
+ } catch {
+ case _: AnalysisException => throw error
+ }
+ val result: Expression = plan.collectFirst {
+ case Project(Seq(a: Alias), OneRowRelation()) => a.child
+ }.get
+ result match {
+ case expr if expr.foldable => expr.eval().toString
Review Comment:
This 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] dtenedor commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1199511465
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -3187,7 +3189,42 @@ class AstBuilder extends SqlBaseParserBaseVisitor[AnyRef] with SQLConfHelper wit
ctx: PropertyListContext): Map[String, String] = withOrigin(ctx) {
val properties = ctx.property.asScala.map { property =>
val key = visitPropertyKey(property.key)
- val value = visitPropertyValue(property.value)
+ // If the expression provided for the option value is a literal, use the string
+ // representation of the contents of the literal value provided here. Otherwise, if this
+ // expression is constant but non-literal, evaluate the expression and use its value instead.
+ val value = if (property.value == null) {
+ null
+ } else {
+ val parsed = expression(property.value)
+ def error: Throwable = {
+ new ParseException(
+ errorClass = "INVALID_SQL_SYNTAX",
+ messageParameters = Map(
+ "inputString" ->
+ s"option or property key $key is invalid; only constant expressions are supported"),
+ ctx)
+ }
+ val plan = try {
+ val analyzer: Analyzer = ResolveDefaultColumns.DefaultColumnAnalyzer
Review Comment:
Good point. I updated this to avoid using the analyzer, and just check if the expression is foldable and `eval` it instead. This supports all types of literals and some basic operations now. Function calls are not supported yet; I can support them later, but that change is more invasive.
--
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] dtenedor commented on pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on PR #41191:
URL: https://github.com/apache/spark/pull/41191#issuecomment-1583566600
(Note, this is passing all CI again.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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] gengliangwang commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general expressions as OPTIONS values in the parser
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1196885982
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -3187,7 +3187,24 @@ class AstBuilder extends SqlBaseParserBaseVisitor[AnyRef] with SQLConfHelper wit
ctx: PropertyListContext): Map[String, String] = withOrigin(ctx) {
val properties = ctx.property.asScala.map { property =>
val key = visitPropertyKey(property.key)
- val value = visitPropertyValue(property.value)
+ // A property value can be String, Integer, Boolean or Decimal. Here we extract the property
+ // value based on whether its a string, integer, boolean or decimal literal.
+ val value =
+ if (property.value == null) {
+ null
+ } else {
+ expression(property.value) match {
+ case Literal(str: UTF8String, StringType) => str.toString
+ case Literal(_, BooleanType) => property.value.getText.toLowerCase(Locale.ROOT)
+ case Literal(_, IntegerType | DecimalType()) => property.value.getText
Review Comment:
Seems no new syntax is supported after this PR, while the title says support general expressions... I prefer having the constant folding in this PR
--
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] dtenedor commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as OPTIONS values in the parser
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1197029651
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -3187,7 +3189,37 @@ class AstBuilder extends SqlBaseParserBaseVisitor[AnyRef] with SQLConfHelper wit
ctx: PropertyListContext): Map[String, String] = withOrigin(ctx) {
val properties = ctx.property.asScala.map { property =>
val key = visitPropertyKey(property.key)
- val value = visitPropertyValue(property.value)
+ // If the expression provided for the option value is a literal, use the string
+ // representation of the contents of the literal value provided here. Otherwise, if this
+ // expression is constant but non-literal, evaluate the expression and use its value instead.
+ val value = if (property.value == null) {
+ null
+ } else {
+ val parsed = expression(property.value)
+ def error: Throwable = {
+ new ParseException(
+ errorClass = "INVALID_SQL_SYNTAX",
+ messageParameters = Map(
+ "inputString" ->
+ s"option or property key $key is invalid; only constant expressions are supported"),
+ ctx)
+ }
+ val plan = try {
+ val analyzer: Analyzer = ResolveDefaultColumns.DefaultColumnAnalyzer
+ val analyzed = analyzer.execute(Project(Seq(Alias(parsed, "col")()), OneRowRelation()))
+ analyzer.checkAnalysis(analyzed)
+ ConstantFolding(analyzed)
+ } catch {
+ case _: AnalysisException => throw error
+ }
+ val result: Expression = plan.collectFirst {
+ case Project(Seq(a: Alias), OneRowRelation()) => a.child
+ }.get
+ result match {
+ case expr if expr.foldable => expr.eval().toString
Review Comment:
Update: I found that using `Cast` does not work in all cases, such as timestamps since we do not currently support casting timestamps to strings. I switched this back to use `.toString` and added a comment explaining this. It seems to work well for different types of 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] dtenedor commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1207445651
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveTableSpec.scala:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.analysis
+
+import org.apache.spark.SparkThrowable
+import org.apache.spark.sql.catalyst.expressions.{Expression, Literal}
+import org.apache.spark.sql.catalyst.plans.logical._
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.TreePattern.COMMAND
+import org.apache.spark.sql.errors.QueryCompilationErrors
+
+/**
+ * This rule is responsible for matching against unresolved table specifications in commands with
+ * OPTIONS lists. The parser produces such lists as maps from strings to unresolved expressions.
+ * After otherwise resolving such expressions in the analyzer, this rule converts them to resolved
+ * table specifications wherein these OPTIONS list values are represented as strings instead, for
+ * convenience. The 'resolveOption' is for resolving function calls within each table option.
+ */
+case class ResolveTableSpec(resolveOption: Expression => Expression) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ plan.resolveOperatorsWithPruning(_.containsAnyPattern(COMMAND), ruleId) {
+ case t @ CreateTable(_, _, _, u: UnresolvedTableSpec, _) =>
Review Comment:
Sounds good, 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] dtenedor commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1218746345
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveTableSpec.scala:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.analysis
+
+import org.apache.spark.SparkThrowable
+import org.apache.spark.sql.catalyst.expressions.{Alias, Expression, Literal}
+import org.apache.spark.sql.catalyst.optimizer.ConstantFolding
+import org.apache.spark.sql.catalyst.plans.logical._
+import org.apache.spark.sql.errors.QueryCompilationErrors
+
+/**
+ * This object is responsible for processing unresolved table specifications in commands with
+ * OPTIONS lists. The parser produces such lists as maps from strings to unresolved expressions.
+ * After otherwise resolving such expressions in the analyzer, here we convert them to resolved
+ * table specifications wherein these OPTIONS list values are represented as strings instead, for
+ * convenience.
+ */
+object ResolveTableSpec {
+ def apply(u: UnresolvedTableSpec, opts: UnresolvedOptionsList): ResolvedTableSpec = {
+ val newOptions: Seq[(String, String)] = opts.options.map {
+ case (key: String, null) =>
+ (key, null)
+ case (key: String, value: Expression) =>
+ val newValue: String = try {
+ constantFold(value) match {
Review Comment:
Sounds good, this 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] gengliangwang commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1204980014
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -1385,8 +1389,37 @@ case class DropIndex(
case class TableSpec(
properties: Map[String, String],
provider: Option[String],
- options: Map[String, String],
+ // Note that the map value can be null if the input option only provided a key.
+ optionsList: Map[String, Expression],
location: Option[String],
comment: Option[String],
serde: Option[SerdeInfo],
- external: Boolean)
+ external: Boolean) extends TreePatternBits {
+ /** This is a convenience method to obtain the options list as a map of strings to strings. */
+ def options: Map[String, String] = optionsList.map { case (key, value) =>
+ val newValue = if (value == null) {
+ null
+ } else value match {
+ case Literal(litVal: String, _) => litVal
+ case Literal(litVal: UTF8String, _) => litVal.toString
+ case _ => value.sql
Review Comment:
How can a non-literal expression convert back to itself with `toExpressionOptions`?
--
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] gengliangwang commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1206049315
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -1385,8 +1389,49 @@ case class DropIndex(
case class TableSpec(
properties: Map[String, String],
provider: Option[String],
- options: Map[String, String],
+ // Note that the map value can be null if the input option only provided a key.
+ optionsList: Map[String, Expression],
location: Option[String],
comment: Option[String],
serde: Option[SerdeInfo],
- external: Boolean)
+ external: Boolean) extends TreePatternBits {
+ /** This is a convenience method to obtain the options list as a map of strings to strings. */
+ lazy val options: Map[String, String] = {
Review Comment:
This looks hacky. Ideally, a SQL query should work even when the optimizer rule `ConstantFolding`.
Sorry for getting back and forth: shall we have a `UnresolvedTableSpec` which contains `optionsList: Map[String, Expression]`. And another rule to resolve `UnresolvedTableSpec` as `TableSpec` if the expression in the map is all resolved? We can try using expression.eval() in the new rule.
--
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] dtenedor commented on pull request #41191: [SPARK-43529][SQL] Support general constant expressions as OPTIONS values in the parser
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on PR #41191:
URL: https://github.com/apache/spark/pull/41191#issuecomment-1553328363
Note @MaxGekk @gengliangwang it says the GitHub actions CI failed, but actually this was just an unrelated PySpark flake and they actually passed :)
--
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] dtenedor commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general expressions as OPTIONS values in the parser
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1196972501
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -3187,7 +3187,24 @@ class AstBuilder extends SqlBaseParserBaseVisitor[AnyRef] with SQLConfHelper wit
ctx: PropertyListContext): Map[String, String] = withOrigin(ctx) {
val properties = ctx.property.asScala.map { property =>
val key = visitPropertyKey(property.key)
- val value = visitPropertyValue(property.value)
+ // A property value can be String, Integer, Boolean or Decimal. Here we extract the property
+ // value based on whether its a string, integer, boolean or decimal literal.
+ val value =
+ if (property.value == null) {
+ null
+ } else {
+ expression(property.value) match {
+ case Literal(str: UTF8String, StringType) => str.toString
+ case Literal(_, BooleanType) => property.value.getText.toLowerCase(Locale.ROOT)
+ case Literal(_, IntegerType | DecimalType()) => property.value.getText
Review Comment:
OK, this 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] gengliangwang commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1217103535
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -158,30 +158,37 @@ class ResolveSessionCatalog(val catalogManager: CatalogManager)
// For CREATE TABLE [AS SELECT], we should use the v1 command if the catalog is resolved to the
// session catalog and the table provider is not v2.
- case c @ CreateTable(ResolvedV1Identifier(ident), _, _, _, _) =>
+ case c @ CreateTable(ResolvedV1Identifier(ident), _, _, u: UnresolvedTableSpec, _,
Review Comment:
Nit: shall we have a single rule for resolving the table spec? This rule is for resolving session catalogs.
--
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] gengliangwang commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1209790677
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -1382,11 +1383,64 @@ case class DropIndex(
copy(table = newChild)
}
-case class TableSpec(
+trait TableSpec {
+ def properties: Map[String, String]
+ def provider: Option[String]
+ def options: Map[String, String]
+ def location: Option[String]
+ def comment: Option[String]
+ def serde: Option[SerdeInfo]
+ def external: Boolean
+ def copy(
+ properties: Map[String, String] = properties,
+ provider: Option[String] = provider,
+ options: Map[String, String] = options,
+ location: Option[String] = location,
+ comment: Option[String] = comment,
+ serde: Option[SerdeInfo] = serde,
+ external: Boolean = external): TableSpec
+}
+
+case class UnresolvedTableSpec(
+ properties: Map[String, String],
+ provider: Option[String],
+ optionsExpressions: Map[String, Expression],
+ location: Option[String],
+ comment: Option[String],
+ serde: Option[SerdeInfo],
+ external: Boolean) extends TableSpec {
+ override def options: Map[String, String] = {
+ throw SparkException.internalError("Invalid call to 'options' method for unresolved TableSpec")
+ }
+ override def copy(
+ properties: Map[String, String],
+ provider: Option[String],
+ options: Map[String, String],
+ location: Option[String],
+ comment: Option[String],
+ serde: Option[SerdeInfo],
+ external: Boolean = external): TableSpec = {
+ UnresolvedTableSpec(
+ properties, provider, optionsExpressions, location, comment, serde, external)
+ }
+}
+
+case class ResolvedTableSpec(
properties: Map[String, String],
provider: Option[String],
options: Map[String, String],
location: Option[String],
comment: Option[String],
serde: Option[SerdeInfo],
- external: Boolean)
+ external: Boolean) extends TableSpec {
+ override def copy(
Review Comment:
case class has built-in support for method copy. Why do we need this?
--
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] gengliangwang commented on pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on PR #41191:
URL: https://github.com/apache/spark/pull/41191#issuecomment-1567902613
@dtenedor Also, we need end-to-end test cases
--
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] gengliangwang commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1201267669
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -1389,4 +1389,8 @@ case class TableSpec(
location: Option[String],
comment: Option[String],
serde: Option[SerdeInfo],
- external: Boolean)
+ external: Boolean,
+ // If non-empty, this contains the values of the 'options' list as parsed but unresolved
+ // expressions. The analyzer will then constant-fold them to literals and clear this field,
+ // assigning the result to 'options'.
+ unresolvedOptions: Map[String, Option[Expression]] = Map.empty)
Review Comment:
Why `Option[Expression]`? Can we make it `Map[String, 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] dtenedor commented on pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on PR #41191:
URL: https://github.com/apache/spark/pull/41191#issuecomment-1581260144
The main commit responding to the last round of code review comments is here: https://github.com/apache/spark/pull/41191/commits/65f9a9239770613f18e849ee532996c850b48044.
--
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] gengliangwang commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1223945570
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveTableSpec.scala:
##########
@@ -0,0 +1,90 @@
+/*
+ * 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.analysis
+
+import org.apache.spark.SparkThrowable
+import org.apache.spark.sql.catalyst.expressions.{Expression, Literal}
+import org.apache.spark.sql.catalyst.plans.logical._
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.TreePattern.COMMAND
+import org.apache.spark.sql.errors.QueryCompilationErrors
+import org.apache.spark.sql.types.{ArrayType, MapType, StructType}
+
+/**
+ * This object is responsible for processing unresolved table specifications in commands with
+ * OPTIONS lists. The parser produces such lists as maps from strings to unresolved expressions.
+ * After otherwise resolving such expressions in the analyzer, here we convert them to resolved
+ * table specifications wherein these OPTIONS list values are represented as strings instead, for
+ * convenience.
+ */
+object ResolveTableSpec extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ plan.resolveOperatorsWithPruning(_.containsAnyPattern(COMMAND), ruleId) {
+ case t: CreateTable =>
+ resolveTableSpec(t, t.tableSpec, t.optionsListExpressions, s => t.copy(tableSpec = s))
+ case t: CreateTableAsSelect =>
+ resolveTableSpec(t, t.tableSpec, t.optionsListExpressions, s => t.copy(tableSpec = s))
+ case t: ReplaceTable =>
+ resolveTableSpec(t, t.tableSpec, t.optionsListExpressions, s => t.copy(tableSpec = s))
+ case t: ReplaceTableAsSelect =>
+ resolveTableSpec(t, t.tableSpec, t.optionsListExpressions, s => t.copy(tableSpec = s))
+ }
+ }
+
+ /** Helper method to resolve the table specification within a logical plan. */
+ private def resolveTableSpec(
+ input: LogicalPlan, tableSpec: TableSpec, optionsListExpressions: OptionsListExpressions,
+ withNewSpec: TableSpec => LogicalPlan): LogicalPlan = tableSpec match {
+ case u: UnresolvedTableSpec if optionsListExpressions.allOptionsResolved =>
+ val newOptions: Seq[(String, String)] = optionsListExpressions.options.map {
+ case (key: String, null) =>
+ (key, null)
+ case (key: String, value: Expression) =>
+ val newValue: String = try {
+ val dt = value.dataType
+ value match {
+ case Literal(null, _) =>
+ "null"
Review Comment:
```suggestion
null
```
--
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] gengliangwang commented on pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on PR #41191:
URL: https://github.com/apache/spark/pull/41191#issuecomment-1586560032
This has been iterated for many rounds. I will make a follow-up PR to further make the code simpler.
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] dtenedor commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1212368577
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveTableSpec.scala:
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.analysis
+
+import org.apache.spark.SparkThrowable
+import org.apache.spark.sql.catalyst.expressions.{Expression, Literal}
+import org.apache.spark.sql.catalyst.plans.logical._
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.TreePattern.COMMAND
+import org.apache.spark.sql.errors.QueryCompilationErrors
+
+/**
+ * This rule is responsible for matching against unresolved table specifications in commands with
+ * OPTIONS lists. The parser produces such lists as maps from strings to unresolved expressions.
+ * After otherwise resolving such expressions in the analyzer, this rule converts them to resolved
+ * table specifications wherein these OPTIONS list values are represented as strings instead, for
+ * convenience. The 'resolveOption' is for resolving function calls within each table option.
+ */
+case class ResolveTableSpec(resolveOption: Expression => Expression) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ plan.resolveOperatorsWithPruning(_.containsAnyPattern(COMMAND), ruleId) {
+ case t: CreateTable if t.tableSpec.isInstanceOf[UnresolvedTableSpec] =>
+ t.copy(tableSpec = resolveTableSpec(t.tableSpec))
+ case t: CreateTableAsSelect if t.tableSpec.isInstanceOf[UnresolvedTableSpec] =>
+ t.copy(tableSpec = resolveTableSpec(t.tableSpec))
+ case t: ReplaceTable if t.tableSpec.isInstanceOf[UnresolvedTableSpec] =>
+ t.copy(tableSpec = resolveTableSpec(t.tableSpec))
+ case t: ReplaceTableAsSelect if t.tableSpec.isInstanceOf[UnresolvedTableSpec] =>
+ t.copy(tableSpec = resolveTableSpec(t.tableSpec))
+ }
+ }
+
+ private def resolveTableSpec(t: TableSpec): TableSpec = {
+ val u = t.asInstanceOf[UnresolvedTableSpec]
+ val newOptions: Map[String, String] = u.optionsExpressions.map {
+ case (key: String, null) =>
+ (key, null)
+ case (key: String, value: Expression) =>
+ val newValue: String = try {
+ resolveOption(value) match {
+ case lit: Literal =>
Review Comment:
Sure, the `catch` block checked for `MatchError`, but I added a `case _` to make this explicit.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -2406,6 +2407,15 @@ class Analyzer(override val catalogManager: CatalogManager) extends RuleExecutor
}
}
+ /** Helper method to resolve function calls within expressions of TableSpec options. */
+ def ResolveTableSpecOption(expression: Expression): Expression = {
Review Comment:
👍 I was able to remove this entirely, and `ResolveTableSpecOption` is no longer a separate analyzer rule. Instead, `ResolveSessionCatalog` now simply just resolves the `TableSpec` when needed directly.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -2406,6 +2407,15 @@ class Analyzer(override val catalogManager: CatalogManager) extends RuleExecutor
}
}
+ /** Helper method to resolve function calls within expressions of TableSpec options. */
+ def ResolveTableSpecOption(expression: Expression): Expression = {
+ val analyzed = executeSameContext(Project(Seq(Alias(expression, "col")()), OneRowRelation()))
+ val folded = ConstantFolding(analyzed)
+ folded match {
+ case Project(Seq(Alias(expression, _)), _) => expression
Review Comment:
Good Q: now that we just apply constant-folding here, we do not need to check if the pattern does not match, because the constant folding we invoked will not change the operators and number of projection expressions in the query plan. I added a comment to this effect here.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -1382,11 +1383,64 @@ case class DropIndex(
copy(table = newChild)
}
-case class TableSpec(
+trait TableSpec {
+ def properties: Map[String, String]
+ def provider: Option[String]
+ def options: Map[String, String]
+ def location: Option[String]
+ def comment: Option[String]
+ def serde: Option[SerdeInfo]
+ def external: Boolean
+ def copy(
+ properties: Map[String, String] = properties,
+ provider: Option[String] = provider,
+ options: Map[String, String] = options,
+ location: Option[String] = location,
+ comment: Option[String] = comment,
+ serde: Option[SerdeInfo] = serde,
+ external: Boolean = external): TableSpec
+}
+
+case class UnresolvedTableSpec(
+ properties: Map[String, String],
+ provider: Option[String],
+ optionsExpressions: Map[String, Expression],
+ location: Option[String],
+ comment: Option[String],
+ serde: Option[SerdeInfo],
+ external: Boolean) extends TableSpec {
+ override def options: Map[String, String] = {
+ throw SparkException.internalError("Invalid call to 'options' method for unresolved TableSpec")
+ }
+ override def copy(
+ properties: Map[String, String],
+ provider: Option[String],
+ options: Map[String, String],
+ location: Option[String],
+ comment: Option[String],
+ serde: Option[SerdeInfo],
+ external: Boolean = external): TableSpec = {
+ UnresolvedTableSpec(
+ properties, provider, optionsExpressions, location, comment, serde, external)
+ }
+}
+
+case class ResolvedTableSpec(
properties: Map[String, String],
provider: Option[String],
options: Map[String, String],
location: Option[String],
comment: Option[String],
serde: Option[SerdeInfo],
- external: Boolean)
+ external: Boolean) extends TableSpec {
+ override def copy(
Review Comment:
I only added to copy the `TableSpec` with a new `location`. I switched this to just `withNewLocation` instead to simplify.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala:
##########
@@ -927,9 +927,14 @@ abstract class TreeNode[BaseType <: TreeNode[BaseType]] extends Product with Tre
redactMapString(map.asCaseSensitiveMap().asScala, maxFields)
case map: Map[_, _] =>
redactMapString(map, maxFields)
- case t: TableSpec =>
- t.copy(properties = Utils.redact(t.properties).toMap,
- options = Utils.redact(t.options).toMap) :: Nil
+ case t: UnresolvedTableSpec =>
+ val newOptionsList = Utils.redact(t.optionsExpressions.map { case (key, value) =>
+ (key, value.sql)
+ }).toMap
+ ResolvedTableSpec(
Review Comment:
I updated this to keep the object as an `UnresolvedTableSpec` here instead.
--
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] dtenedor commented on pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on PR #41191:
URL: https://github.com/apache/spark/pull/41191#issuecomment-1575087553
Note: the CI is actually passing, the pyspark failure is spurious/unrelated.
<img width="312" alt="image" src="https://github.com/apache/spark/assets/99207096/02176772-3e7c-4cbd-8308-297c6fd85066">
--
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] dtenedor commented on pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on PR #41191:
URL: https://github.com/apache/spark/pull/41191#issuecomment-1574498654
Hi @gengliangwang I implemented your suggestion, the code is simpler now, this is ready for another look.
--
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] gengliangwang commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1217279661
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveTableSpec.scala:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.analysis
+
+import org.apache.spark.SparkThrowable
+import org.apache.spark.sql.catalyst.expressions.{Alias, Expression, Literal}
+import org.apache.spark.sql.catalyst.optimizer.ConstantFolding
+import org.apache.spark.sql.catalyst.plans.logical._
+import org.apache.spark.sql.errors.QueryCompilationErrors
+
+/**
+ * This object is responsible for processing unresolved table specifications in commands with
+ * OPTIONS lists. The parser produces such lists as maps from strings to unresolved expressions.
+ * After otherwise resolving such expressions in the analyzer, here we convert them to resolved
+ * table specifications wherein these OPTIONS list values are represented as strings instead, for
+ * convenience.
+ */
+object ResolveTableSpec {
+ def apply(u: UnresolvedTableSpec, opts: UnresolvedOptionsList): ResolvedTableSpec = {
+ val newOptions: Seq[(String, String)] = opts.options.map {
+ case (key: String, null) =>
+ (key, null)
+ case (key: String, value: Expression) =>
+ val newValue: String = try {
+ constantFold(value) match {
Review Comment:
Shall we just use `value.eval()` since it is foldable?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general expressions as OPTIONS values in the parser
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1196089093
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -3187,7 +3187,24 @@ class AstBuilder extends SqlBaseParserBaseVisitor[AnyRef] with SQLConfHelper wit
ctx: PropertyListContext): Map[String, String] = withOrigin(ctx) {
val properties = ctx.property.asScala.map { property =>
val key = visitPropertyKey(property.key)
- val value = visitPropertyValue(property.value)
+ // A property value can be String, Integer, Boolean or Decimal. Here we extract the property
+ // value based on whether its a string, integer, boolean or decimal literal.
+ val value =
+ if (property.value == null) {
+ null
+ } else {
+ expression(property.value) match {
+ case Literal(str: UTF8String, StringType) => str.toString
+ case Literal(_, BooleanType) => property.value.getText.toLowerCase(Locale.ROOT)
+ case Literal(_, IntegerType | DecimalType()) => property.value.getText
Review Comment:
Are there any reasons to support only literals but not foldable 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] gerashegalov commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "gerashegalov (via GitHub)" <gi...@apache.org>.
gerashegalov commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1201060367
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/ResolveTableSpec.scala:
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.analysis
+
+import org.apache.spark.sql.AnalysisException
+import org.apache.spark.sql.catalyst.expressions._
+import org.apache.spark.sql.catalyst.optimizer.ConstantFolding
+import org.apache.spark.sql.catalyst.plans.logical._
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.TreePattern.COMMAND
+import org.apache.spark.sql.connector.catalog.CatalogManager
+
+/**
+ * This rule is responsible for analyzing expressions passed in as values for OPTIONS lists for
+ * commands such as CREATE TABLE. These expressions may be constant but non-literal, in which case
+ * we perform constant folding here.
+ */
+case class ResolveTableSpec(catalogManager: CatalogManager) extends Rule[LogicalPlan] {
+ def apply(plan: LogicalPlan): LogicalPlan = {
+ plan.transformUpWithPruning(_.containsPattern(COMMAND), ruleId) {
+ case c: CreateTable if c.tableSpec.unresolvedOptions.nonEmpty =>
+ val newOptions = c.tableSpec.unresolvedOptions.map(resolveOption)
+ c.copy(tableSpec = c.tableSpec.copy(options = newOptions, unresolvedOptions = Map.empty))
+ }
+ }
+
+ private def resolveOption(kv: (String, Option[Expression])): (String, String) = {
+ val (key, value) = kv
+ value.map { parsed: Expression =>
+ val plan = try {
+ lazy val analyzer = new Analyzer(catalogManager)
+ val analyzed = analyzer.execute(Project(Seq(Alias(parsed, "col")()), OneRowRelation()))
+ analyzer.checkAnalysis(analyzed)
+ ConstantFolding(analyzed)
+ } catch {
+ case _: AnalysisException =>
+ throw optionNotConstantError(key)
+ }
+ val result: Expression = plan.collectFirst {
+ case Project(Seq(a: Alias), OneRowRelation()) => a.child
+ }.get
+ val newValue = result match {
+ case expr if expr.isInstanceOf[Literal] =>
+ // Note: we use 'toString' here instead of using a Cast expression to support some types
+ // of literals where casting to string is not supported.
+ expr.toString
+ case _ =>
+ throw optionNotConstantError(key)
+ }
Review Comment:
L56 potentially risks an NoSuchElementException for None, or if not just for cleaner style I'd suggest
```suggestion
val result: Option[Expression] = plan.collectFirst {
case Project(Seq(a: Alias), OneRowRelation()) => a.child
}
val newValue = result match {
case Some(expr@Literal(_, _)) =>
// Note: we use 'toString' here instead of using a Cast expression to support some types
// of literals where casting to string is not supported.
expr.toString
case _ =>
throw optionNotConstantError(key)
}
```
--
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] dtenedor commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1201270442
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/ResolveTableSpec.scala:
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.analysis
+
+import org.apache.spark.sql.AnalysisException
+import org.apache.spark.sql.catalyst.expressions._
+import org.apache.spark.sql.catalyst.optimizer.ConstantFolding
+import org.apache.spark.sql.catalyst.plans.logical._
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.TreePattern.COMMAND
+import org.apache.spark.sql.connector.catalog.CatalogManager
+
+/**
+ * This rule is responsible for analyzing expressions passed in as values for OPTIONS lists for
+ * commands such as CREATE TABLE. These expressions may be constant but non-literal, in which case
+ * we perform constant folding here.
+ */
+case class ResolveTableSpec(catalogManager: CatalogManager) extends Rule[LogicalPlan] {
+ def apply(plan: LogicalPlan): LogicalPlan = {
+ plan.transformUpWithPruning(_.containsPattern(COMMAND), ruleId) {
+ case c: CreateTable if c.tableSpec.unresolvedOptions.nonEmpty =>
+ val newOptions = c.tableSpec.unresolvedOptions.map(resolveOption)
+ c.copy(tableSpec = c.tableSpec.copy(options = newOptions, unresolvedOptions = Map.empty))
+ }
+ }
+
+ private def resolveOption(kv: (String, Option[Expression])): (String, String) = {
+ val (key, value) = kv
+ value.map { parsed: Expression =>
+ val plan = try {
+ lazy val analyzer = new Analyzer(catalogManager)
+ val analyzed = analyzer.execute(Project(Seq(Alias(parsed, "col")()), OneRowRelation()))
+ analyzer.checkAnalysis(analyzed)
+ ConstantFolding(analyzed)
+ } catch {
+ case _: AnalysisException =>
+ throw optionNotConstantError(key)
+ }
+ val result: Expression = plan.collectFirst {
+ case Project(Seq(a: Alias), OneRowRelation()) => a.child
+ }.get
+ val newValue = result match {
+ case expr if expr.isInstanceOf[Literal] =>
+ // Note: we use 'toString' here instead of using a Cast expression to support some types
+ // of literals where casting to string is not supported.
+ expr.toString
+ case _ =>
+ throw optionNotConstantError(key)
+ }
Review Comment:
Good idea, this 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] gengliangwang commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1221015578
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveTableSpec.scala:
##########
@@ -0,0 +1,89 @@
+/*
+ * 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.analysis
+
+import org.apache.spark.SparkThrowable
+import org.apache.spark.sql.catalyst.expressions.{Expression, Literal}
+import org.apache.spark.sql.catalyst.plans.logical._
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.TreePattern.COMMAND
+import org.apache.spark.sql.errors.QueryCompilationErrors
+import org.apache.spark.sql.types.{ArrayType, MapType, StructType}
+
+/**
+ * This object is responsible for processing unresolved table specifications in commands with
+ * OPTIONS lists. The parser produces such lists as maps from strings to unresolved expressions.
+ * After otherwise resolving such expressions in the analyzer, here we convert them to resolved
+ * table specifications wherein these OPTIONS list values are represented as strings instead, for
+ * convenience.
+ */
+object ResolveTableSpec extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ plan.resolveOperatorsWithPruning(_.containsAnyPattern(COMMAND), ruleId) {
+ case t: CreateTable =>
+ resolveTableSpec(t, s => t.copy(tableSpec = s))
+ case t: CreateTableAsSelect =>
+ resolveTableSpec(t, s => t.copy(tableSpec = s))
+ case t: ReplaceTable =>
+ resolveTableSpec(t, s => t.copy(tableSpec = s))
+ case t: ReplaceTableAsSelect =>
+ resolveTableSpec(t, s => t.copy(tableSpec = s))
+ }
+ }
+
+ /** Helper method to resolve the table specification within a logical plan. */
+ private def resolveTableSpec(
+ t: HasTableSpec, withNewSpec: TableSpec => LogicalPlan): LogicalPlan = t.tableSpec match {
+ case u: UnresolvedTableSpec if t.unresolvedOptionsList.allOptionsResolved =>
+ val newOptions: Seq[(String, String)] = t.unresolvedOptionsList.options.map {
+ case (key: String, null) =>
+ (key, null)
+ case (key: String, value: Expression) =>
+ val newValue: String = try {
+ val dt = value.dataType
+ value match {
+ case Literal(null, _) =>
+ "null"
Review Comment:
should it be `null` or `"null"`?
--
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] dtenedor commented on pull request #41191: [SPARK-43529][SQL] Support general literal expressions as OPTIONS values in the parser
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on PR #41191:
URL: https://github.com/apache/spark/pull/41191#issuecomment-1556009814
Update on this: I thought of a simpler way to break apart this work into smaller pieces so we can make gradual improvements. Let me make a commit and then I can ping this thread again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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] dtenedor commented on pull request #41191: [SPARK-43529][SQL] Support general literal expressions as OPTIONS values in the parser
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on PR #41191:
URL: https://github.com/apache/spark/pull/41191#issuecomment-1557836990
This is done, I have deferred the constant folding logic to the analyzer. This is ready for another look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general expressions as OPTIONS values in the parser
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1196979589
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -3187,7 +3189,37 @@ class AstBuilder extends SqlBaseParserBaseVisitor[AnyRef] with SQLConfHelper wit
ctx: PropertyListContext): Map[String, String] = withOrigin(ctx) {
val properties = ctx.property.asScala.map { property =>
val key = visitPropertyKey(property.key)
- val value = visitPropertyValue(property.value)
+ // If the expression provided for the option value is a literal, use the string
+ // representation of the contents of the literal value provided here. Otherwise, if this
+ // expression is constant but non-literal, evaluate the expression and use its value instead.
+ val value = if (property.value == null) {
+ null
+ } else {
+ val parsed = expression(property.value)
+ def error: Throwable = {
+ new ParseException(
+ errorClass = "INVALID_SQL_SYNTAX",
+ messageParameters = Map(
+ "inputString" ->
+ s"option or property key $key is invalid; only constant expressions are supported"),
+ ctx)
+ }
+ val plan = try {
+ val analyzer: Analyzer = ResolveDefaultColumns.DefaultColumnAnalyzer
+ val analyzed = analyzer.execute(Project(Seq(Alias(parsed, "col")()), OneRowRelation()))
+ analyzer.checkAnalysis(analyzed)
+ ConstantFolding(analyzed)
+ } catch {
+ case _: AnalysisException => throw error
+ }
+ val result: Expression = plan.collectFirst {
+ case Project(Seq(a: Alias), OneRowRelation()) => a.child
+ }.get
+ result match {
+ case expr if expr.foldable => expr.eval().toString
Review Comment:
How about to wrap `expr` by the `Cast` to string expression and invoke `eval`. `toString` and casting to string can output different results if we care of them.
--
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] dtenedor commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1206022553
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/rules/RuleIdCollection.scala:
##########
@@ -92,6 +92,7 @@ object RuleIdCollection {
"org.apache.spark.sql.catalyst.analysis.ResolveLateralColumnAliasReference" ::
"org.apache.spark.sql.catalyst.analysis.ResolveOrderByAll" ::
"org.apache.spark.sql.catalyst.analysis.ResolveRowLevelCommandAssignments" ::
+ "org.apache.spark.sql.catalyst.analysis.ResolveTableSpec" ::
Review Comment:
Sure, this 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] gengliangwang closed pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang closed pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
URL: https://github.com/apache/spark/pull/41191
--
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] gengliangwang commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1209792806
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -2406,6 +2407,15 @@ class Analyzer(override val catalogManager: CatalogManager) extends RuleExecutor
}
}
+ /** Helper method to resolve function calls within expressions of TableSpec options. */
+ def ResolveTableSpecOption(expression: Expression): Expression = {
Review Comment:
If we manage to transform UnresolvedTableSpec in the method `mapExpressions`, probably we don't need this method.
--
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] dtenedor commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1203242441
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/ResolveTableSpec.scala:
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.analysis
+
+import org.apache.spark.sql.AnalysisException
+import org.apache.spark.sql.catalyst.expressions._
+import org.apache.spark.sql.catalyst.optimizer.ConstantFolding
+import org.apache.spark.sql.catalyst.plans.logical._
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.TreePattern.COMMAND
+import org.apache.spark.sql.connector.catalog.CatalogManager
+
+/**
+ * This rule is responsible for analyzing expressions passed in as values for OPTIONS lists for
+ * commands such as CREATE TABLE. These expressions may be constant but non-literal, in which case
+ * we perform constant folding here.
+ */
+case class ResolveTableSpec(catalogManager: CatalogManager) extends Rule[LogicalPlan] {
Review Comment:
Sounds good, done!
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -1389,4 +1389,8 @@ case class TableSpec(
location: Option[String],
comment: Option[String],
serde: Option[SerdeInfo],
- external: Boolean)
+ external: Boolean,
+ // If non-empty, this contains the values of the 'options' list as parsed but unresolved
+ // expressions. The analyzer will then constant-fold them to literals and clear this field,
+ // assigning the result to 'options'.
+ unresolvedOptions: Map[String, Option[Expression]] = Map.empty)
Review Comment:
We talked offline, I changed it to `Map[String, Expression]` per this suggestion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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] gengliangwang commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as OPTIONS values in the parser
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1198544619
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -3187,7 +3189,42 @@ class AstBuilder extends SqlBaseParserBaseVisitor[AnyRef] with SQLConfHelper wit
ctx: PropertyListContext): Map[String, String] = withOrigin(ctx) {
val properties = ctx.property.asScala.map { property =>
val key = visitPropertyKey(property.key)
- val value = visitPropertyValue(property.value)
+ // If the expression provided for the option value is a literal, use the string
+ // representation of the contents of the literal value provided here. Otherwise, if this
+ // expression is constant but non-literal, evaluate the expression and use its value instead.
+ val value = if (property.value == null) {
+ null
+ } else {
+ val parsed = expression(property.value)
+ def error: Throwable = {
+ new ParseException(
+ errorClass = "INVALID_SQL_SYNTAX",
+ messageParameters = Map(
+ "inputString" ->
+ s"option or property key $key is invalid; only constant expressions are supported"),
+ ctx)
+ }
+ val plan = try {
+ val analyzer: Analyzer = ResolveDefaultColumns.DefaultColumnAnalyzer
Review Comment:
@dtenedor It's too hacky to use the analyzer within the parser. Could you verify it in the analyzer?
--
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] gengliangwang commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1201267349
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/ResolveTableSpec.scala:
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.analysis
+
+import org.apache.spark.sql.AnalysisException
+import org.apache.spark.sql.catalyst.expressions._
+import org.apache.spark.sql.catalyst.optimizer.ConstantFolding
+import org.apache.spark.sql.catalyst.plans.logical._
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.TreePattern.COMMAND
+import org.apache.spark.sql.connector.catalog.CatalogManager
+
+/**
+ * This rule is responsible for analyzing expressions passed in as values for OPTIONS lists for
+ * commands such as CREATE TABLE. These expressions may be constant but non-literal, in which case
+ * we perform constant folding here.
+ */
+case class ResolveTableSpec(catalogManager: CatalogManager) extends Rule[LogicalPlan] {
Review Comment:
I wonder if we can avoid creating a new rule for this.
Probably all we need is to make `mapExpressions` supports the type `Map[String, Expression]` https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/QueryPlan.scala#L217
With that, analyzer will analyze the expression and the optimizer will do the constant folding.
--
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] gengliangwang commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1204970784
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/rules/RuleIdCollection.scala:
##########
@@ -92,6 +92,7 @@ object RuleIdCollection {
"org.apache.spark.sql.catalyst.analysis.ResolveLateralColumnAliasReference" ::
"org.apache.spark.sql.catalyst.analysis.ResolveOrderByAll" ::
"org.apache.spark.sql.catalyst.analysis.ResolveRowLevelCommandAssignments" ::
+ "org.apache.spark.sql.catalyst.analysis.ResolveTableSpec" ::
Review Comment:
Let remove this
--
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] dtenedor commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1206028445
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -1385,8 +1389,37 @@ case class DropIndex(
case class TableSpec(
properties: Map[String, String],
provider: Option[String],
- options: Map[String, String],
+ // Note that the map value can be null if the input option only provided a key.
+ optionsList: Map[String, Expression],
location: Option[String],
comment: Option[String],
serde: Option[SerdeInfo],
- external: Boolean)
+ external: Boolean) extends TreePatternBits {
+ /** This is a convenience method to obtain the options list as a map of strings to strings. */
+ def options: Map[String, String] = optionsList.map { case (key, value) =>
+ val newValue = if (value == null) {
+ null
+ } else value match {
+ case Literal(litVal: String, _) => litVal
+ case Literal(litVal: UTF8String, _) => litVal.toString
+ case _ => value.sql
Review Comment:
I realize that in this case, the constant-folding step was not able to convert the option value to a literal, so it was non-constant; we return an appropriate error message in this case 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] dtenedor commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1207391313
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -1385,8 +1389,49 @@ case class DropIndex(
case class TableSpec(
properties: Map[String, String],
provider: Option[String],
- options: Map[String, String],
+ // Note that the map value can be null if the input option only provided a key.
+ optionsList: Map[String, Expression],
location: Option[String],
comment: Option[String],
serde: Option[SerdeInfo],
- external: Boolean)
+ external: Boolean) extends TreePatternBits {
+ /** This is a convenience method to obtain the options list as a map of strings to strings. */
+ lazy val options: Map[String, String] = {
Review Comment:
Sure, this 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] dtenedor commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general expressions as OPTIONS values in the parser
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1196805611
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -3187,7 +3187,24 @@ class AstBuilder extends SqlBaseParserBaseVisitor[AnyRef] with SQLConfHelper wit
ctx: PropertyListContext): Map[String, String] = withOrigin(ctx) {
val properties = ctx.property.asScala.map { property =>
val key = visitPropertyKey(property.key)
- val value = visitPropertyValue(property.value)
+ // A property value can be String, Integer, Boolean or Decimal. Here we extract the property
+ // value based on whether its a string, integer, boolean or decimal literal.
+ val value =
+ if (property.value == null) {
+ null
+ } else {
+ expression(property.value) match {
+ case Literal(str: UTF8String, StringType) => str.toString
+ case Literal(_, BooleanType) => property.value.getText.toLowerCase(Locale.ROOT)
+ case Literal(_, IntegerType | DecimalType()) => property.value.getText
Review Comment:
Yea we could support foldable expressions in the future. And constant-fold them, and then store the text of the resulting constant in the option value instead.
My idea was to separate concerns by doing just the parser change here first, but not doing this constant folding or changing the behavior yet. This PR only supports the same types of literal values for the option values as before the PR, simply improving error messages in other cases from generic `syntax error` to more descriptive messages indicating that only certain literals are supported. Then in another PR, we could implement the constant-folding.
But if you prefer, I can implement the constant-folding in this PR as well, please let me know.
--
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] gengliangwang commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1214006267
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -1382,11 +1407,59 @@ case class DropIndex(
copy(table = newChild)
}
-case class TableSpec(
+trait TableSpec {
+ def properties: Map[String, String]
+ def provider: Option[String]
+ def location: Option[String]
+ def comment: Option[String]
+ def serde: Option[SerdeInfo]
+ def external: Boolean
+ def withNewLocation(newLocation: Option[String]): TableSpec
+ def treePatternBits: BitSet
+}
+
+case class UnresolvedTableSpec(
+ properties: Map[String, String],
+ provider: Option[String],
+ optionsExpressions: Map[String, Expression],
Review Comment:
@dtenedor I got a new idea about this.
How about we have a new expression for `optionsExpressions`
```
case class UnresolvedTableOptions(options: Seq[(String, Expression)])
extends Expression with Unevaluable {
override def nullable: Boolean = true
override def dataType: DataType = MapType[StringType, StringType]
override def children: Seq[Expression] = optionsExpressions.map(_._2)
override protected def withNewChildrenInternal(
newChildren: IndexedSeq[Expression]): Expression = {
assert(options.length == newChildren.length)
...
}
}
}
```
So that we don't need to handle the treePatternBits or hacking the method `mapExpressions`.
Note that here the Options is `Seq` instead of `Map` to make sure the expression order is fixed.
--
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] gengliangwang commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1221034438
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -437,6 +437,12 @@ trait V2CreateTablePlan extends LogicalPlan {
def withPartitioning(rewritten: Seq[Transform]): V2CreateTablePlan
}
+/** A trait used for logical plan nodes that create or replace new table specifications. */
+trait HasTableSpec extends LogicalPlan {
Review Comment:
I wonder why we create a new trait here. Shall we simply make `UnresolvedOptionsList` part of`UnresolvedTableSpec`?
--
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] gengliangwang commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1221035571
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -437,6 +437,12 @@ trait V2CreateTablePlan extends LogicalPlan {
def withPartitioning(rewritten: Seq[Transform]): V2CreateTablePlan
}
+/** A trait used for logical plan nodes that create or replace new table specifications. */
+trait HasTableSpec extends LogicalPlan {
+ def tableSpec: TableSpec
+ def unresolvedOptionsList: UnresolvedOptionsList
Review Comment:
Even when a logical plan like `CreateTable` is resolved, it will carry the method `unresolvedOptionsList`
--
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] dtenedor commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1224881802
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveTableSpec.scala:
##########
@@ -0,0 +1,90 @@
+/*
+ * 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.analysis
+
+import org.apache.spark.SparkThrowable
+import org.apache.spark.sql.catalyst.expressions.{Expression, Literal}
+import org.apache.spark.sql.catalyst.plans.logical._
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.TreePattern.COMMAND
+import org.apache.spark.sql.errors.QueryCompilationErrors
+import org.apache.spark.sql.types.{ArrayType, MapType, StructType}
+
+/**
+ * This object is responsible for processing unresolved table specifications in commands with
+ * OPTIONS lists. The parser produces such lists as maps from strings to unresolved expressions.
+ * After otherwise resolving such expressions in the analyzer, here we convert them to resolved
+ * table specifications wherein these OPTIONS list values are represented as strings instead, for
+ * convenience.
+ */
+object ResolveTableSpec extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ plan.resolveOperatorsWithPruning(_.containsAnyPattern(COMMAND), ruleId) {
+ case t: CreateTable =>
+ resolveTableSpec(t, t.tableSpec, t.optionsListExpressions, s => t.copy(tableSpec = s))
+ case t: CreateTableAsSelect =>
+ resolveTableSpec(t, t.tableSpec, t.optionsListExpressions, s => t.copy(tableSpec = s))
+ case t: ReplaceTable =>
+ resolveTableSpec(t, t.tableSpec, t.optionsListExpressions, s => t.copy(tableSpec = s))
+ case t: ReplaceTableAsSelect =>
+ resolveTableSpec(t, t.tableSpec, t.optionsListExpressions, s => t.copy(tableSpec = s))
+ }
+ }
+
+ /** Helper method to resolve the table specification within a logical plan. */
+ private def resolveTableSpec(
+ input: LogicalPlan, tableSpec: TableSpec, optionsListExpressions: OptionsListExpressions,
+ withNewSpec: TableSpec => LogicalPlan): LogicalPlan = tableSpec match {
+ case u: UnresolvedTableSpec if optionsListExpressions.allOptionsResolved =>
+ val newOptions: Seq[(String, String)] = optionsListExpressions.options.map {
+ case (key: String, null) =>
+ (key, null)
+ case (key: String, value: Expression) =>
+ val newValue: String = try {
+ val dt = value.dataType
+ value match {
+ case Literal(null, _) =>
+ "null"
Review Comment:
Sounds good, 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] gengliangwang commented on a diff in pull request #41191: [SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #41191:
URL: https://github.com/apache/spark/pull/41191#discussion_r1223945423
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveTableSpec.scala:
##########
@@ -0,0 +1,89 @@
+/*
+ * 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.analysis
+
+import org.apache.spark.SparkThrowable
+import org.apache.spark.sql.catalyst.expressions.{Expression, Literal}
+import org.apache.spark.sql.catalyst.plans.logical._
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.TreePattern.COMMAND
+import org.apache.spark.sql.errors.QueryCompilationErrors
+import org.apache.spark.sql.types.{ArrayType, MapType, StructType}
+
+/**
+ * This object is responsible for processing unresolved table specifications in commands with
+ * OPTIONS lists. The parser produces such lists as maps from strings to unresolved expressions.
+ * After otherwise resolving such expressions in the analyzer, here we convert them to resolved
+ * table specifications wherein these OPTIONS list values are represented as strings instead, for
+ * convenience.
+ */
+object ResolveTableSpec extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ plan.resolveOperatorsWithPruning(_.containsAnyPattern(COMMAND), ruleId) {
+ case t: CreateTable =>
+ resolveTableSpec(t, s => t.copy(tableSpec = s))
+ case t: CreateTableAsSelect =>
+ resolveTableSpec(t, s => t.copy(tableSpec = s))
+ case t: ReplaceTable =>
+ resolveTableSpec(t, s => t.copy(tableSpec = s))
+ case t: ReplaceTableAsSelect =>
+ resolveTableSpec(t, s => t.copy(tableSpec = s))
+ }
+ }
+
+ /** Helper method to resolve the table specification within a logical plan. */
+ private def resolveTableSpec(
+ t: HasTableSpec, withNewSpec: TableSpec => LogicalPlan): LogicalPlan = t.tableSpec match {
+ case u: UnresolvedTableSpec if t.unresolvedOptionsList.allOptionsResolved =>
+ val newOptions: Seq[(String, String)] = t.unresolvedOptionsList.options.map {
+ case (key: String, null) =>
+ (key, null)
+ case (key: String, value: Expression) =>
+ val newValue: String = try {
+ val dt = value.dataType
+ value match {
+ case Literal(null, _) =>
+ "null"
Review Comment:
But string can be null too. If it is `"null"`, then the literal should be `Literal("null", StringType)`
--
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