[GitHub] [spark] cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL] Disallow to specify reserved properties in CREATE NAMESPACE syntax
cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL]
Disallow to specify reserved properties in CREATE NAMESPACE syntax
URL: https://github.com/apache/spark/pull/26806#discussion_r363833688
##
File path: docs/sql-migration-guide.md
##
@@ -264,6 +264,8 @@ license: |
- Since Spark 3.0, the function `percentile_approx` and its alias
`approx_percentile` only accept integral value with range in `[1, 2147483647]`
as its 3rd argument `accuracy`, fractional and string types are disallowed,
e.g. `percentile_approx(10.0, 0.2, 1.8D)` will cause `AnalysisException`. In
Spark version 2.4 and earlier, if `accuracy` is fractional or string value, it
will be coerced to an int value, `percentile_approx(10.0, 0.2, 1.8D)` is
operated as `percentile_approx(10.0, 0.2, 1)` which results in `10.0`.
+ - Since Spark 3.0, the words `location` and `comment` become reserved
database properties, it will fail with `ParseException` if we use them as
members of `DBPROTERTIES` in `CREATE DATABASE` and `ALTER DATABASE ... SET
PROPERTIES(...)`. We need their specific clauses to specify them, e.g. `CREATE
DATABASE test COMMENT 'any comment' LOCATION 'some path'`. We can set
`spark.sql.legacy.property.nonReserved` to `true` to ignore the
`ParseException`, in this case, these properties will be silently removed, e.g
`SET DBPROTERTIES('location'='/tmp')` will affect nothing. In Spark version 2.4
and earlier, these properties are neither reserved nor have side effects, e.g.
`SET DBPROTERTIES('location'='/tmp')` will not change the location of the
database but only create a headless property just like `'a'='b'`.
Review comment:
nit:
```
Since Spark 3.0, `location` and `comment` become reserved database
properties. Commands
will fail if we specify reserved properties in `CREATE DATABASE ...
DBPROTERTIES ` and
`ALTER DATABASE ... SET PROPERTIES`. ...
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL] Disallow to specify reserved properties in CREATE NAMESPACE syntax
cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL]
Disallow to specify reserved properties in CREATE NAMESPACE syntax
URL: https://github.com/apache/spark/pull/26806#discussion_r363833688
##
File path: docs/sql-migration-guide.md
##
@@ -264,6 +264,8 @@ license: |
- Since Spark 3.0, the function `percentile_approx` and its alias
`approx_percentile` only accept integral value with range in `[1, 2147483647]`
as its 3rd argument `accuracy`, fractional and string types are disallowed,
e.g. `percentile_approx(10.0, 0.2, 1.8D)` will cause `AnalysisException`. In
Spark version 2.4 and earlier, if `accuracy` is fractional or string value, it
will be coerced to an int value, `percentile_approx(10.0, 0.2, 1.8D)` is
operated as `percentile_approx(10.0, 0.2, 1)` which results in `10.0`.
+ - Since Spark 3.0, the words `location` and `comment` become reserved
database properties, it will fail with `ParseException` if we use them as
members of `DBPROTERTIES` in `CREATE DATABASE` and `ALTER DATABASE ... SET
PROPERTIES(...)`. We need their specific clauses to specify them, e.g. `CREATE
DATABASE test COMMENT 'any comment' LOCATION 'some path'`. We can set
`spark.sql.legacy.property.nonReserved` to `true` to ignore the
`ParseException`, in this case, these properties will be silently removed, e.g
`SET DBPROTERTIES('location'='/tmp')` will affect nothing. In Spark version 2.4
and earlier, these properties are neither reserved nor have side effects, e.g.
`SET DBPROTERTIES('location'='/tmp')` will not change the location of the
database but only create a headless property just like `'a'='b'`.
Review comment:
nit:
```
Since Spark 3.0, `location` and `comment` become reserved database
properties. Commands will fail
if we specify reserved properties in `CREATE DATABASE ... DBPROTERTIES ` and
`ALTER DATABASE ... SET PROPERTIES`. ...
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL] Disallow to specify reserved properties in CREATE NAMESPACE syntax
cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL]
Disallow to specify reserved properties in CREATE NAMESPACE syntax
URL: https://github.com/apache/spark/pull/26806#discussion_r363832515
##
File path:
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala
##
@@ -864,6 +865,33 @@ class DataSourceV2SQLSuite
}
}
+ test("CreateNameSpace: reserved properties") {
+withSQLConf((SQLConf.LEGACY_PROPERTY_NON_RESERVED.key, "false")) {
+ SupportsNamespaces.RESERVED_PROPERTIES.asScala.foreach { key =>
+val exception = intercept[ParseException] {
+ sql(s"CREATE NAMESPACE testcat.reservedTest WITH
DBPROPERTIES('$key'='dummyVal')")
+}
+assert(exception.getMessage.contains(s"$key is a reserved namespace
property"))
+ }
+}
+withSQLConf((SQLConf.LEGACY_PROPERTY_NON_RESERVED.key, "true")) {
+ SupportsNamespaces.RESERVED_PROPERTIES.asScala.foreach { key =>
+withNamespace("testcat.reservedTest") {
+ withTempDir { tmpDir =>
+val noEffectVal = tmpDir.getCanonicalPath
Review comment:
since it's no effect, can we just use a dumb string like "foo"?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL] Disallow to specify reserved properties in CREATE NAMESPACE syntax
cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL]
Disallow to specify reserved properties in CREATE NAMESPACE syntax
URL: https://github.com/apache/spark/pull/26806#discussion_r363832757
##
File path:
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala
##
@@ -961,6 +989,37 @@ class DataSourceV2SQLSuite
}
}
+ test("AlterNamespaceSetProperties: reserved properties") {
+withSQLConf((SQLConf.LEGACY_PROPERTY_NON_RESERVED.key, "false")) {
+ SupportsNamespaces.RESERVED_PROPERTIES.asScala.foreach { key =>
+withNamespace("testcat.reservedTest") {
+ sql("CREATE NAMESPACE testcat.reservedTest")
+ val exception = intercept[ParseException] {
+sql(s"ALTER NAMESPACE testcat.reservedTest SET PROPERTIES
('$key'='dummyVal')")
+ }
+ assert(exception.getMessage.contains(s"$key is a reserved namespace
property"))
+}
+ }
+}
+withSQLConf((SQLConf.LEGACY_PROPERTY_NON_RESERVED.key, "true")) {
+ SupportsNamespaces.RESERVED_PROPERTIES.asScala.foreach { key =>
+withNamespace("testcat.reservedTest") {
+ withTempDir { tmpDir =>
+val noEffectVal = tmpDir.getCanonicalPath
Review comment:
ditto
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL] Disallow to specify reserved properties in CREATE NAMESPACE syntax
cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL]
Disallow to specify reserved properties in CREATE NAMESPACE syntax
URL: https://github.com/apache/spark/pull/26806#discussion_r363645996
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
##
@@ -2135,6 +2135,15 @@ object SQLConf {
"defined by `from` and `to`.")
.booleanConf
.createWithDefault(false)
+
+ val LEGACY_PROPERTY_NON_RESERVED =
+buildConf("spark.sql.legacy.property.nonReserved")
+ .internal()
+ .doc("When true, all database and table properties are not reserved and
available for " +
+"create/alter syntaxes. But please be aware that the reserved
properties will still be " +
+"used by Spark internally and will ignore their user specified
values.")
Review comment:
`the reserved properties will be silently removed.`
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL] Disallow to specify reserved properties in CREATE NAMESPACE syntax
cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL] Disallow to specify reserved properties in CREATE NAMESPACE syntax URL: https://github.com/apache/spark/pull/26806#discussion_r363645463 ## File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala ## @@ -2498,6 +2498,26 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging } } + private def checkNamespaceProperties( Review comment: it's not just check now, how about `cleanNamespaceProperties`? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL] Disallow to specify reserved properties in CREATE NAMESPACE syntax
cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL]
Disallow to specify reserved properties in CREATE NAMESPACE syntax
URL: https://github.com/apache/spark/pull/26806#discussion_r363645019
##
File path: docs/sql-migration-guide.md
##
@@ -264,6 +264,8 @@ license: |
- Since Spark 3.0, the function `percentile_approx` and its alias
`approx_percentile` only accept integral value with range in `[1, 2147483647]`
as its 3rd argument `accuracy`, fractional and string types are disallowed,
e.g. `percentile_approx(10.0, 0.2, 1.8D)` will cause `AnalysisException`. In
Spark version 2.4 and earlier, if `accuracy` is fractional or string value, it
will be coerced to an int value, `percentile_approx(10.0, 0.2, 1.8D)` is
operated as `percentile_approx(10.0, 0.2, 1)` which results in `10.0`.
+ - Since Spark 3.0, the words `location` and `comment` become reserved
database properties, it will fail with `ParseException` if we use them as
members of `DBPROTERTIES` in `CREATE DATABASE` and `ALTER DATABASE ... SET
PROPERTIES(...)`. We need their specific clauses to specify them, e.g. `CREATE
DATABASE test COMMENT 'any comment' LOCATION 'some path'`. We can set
`spark.sql.legacy.property.nonReserved` to `true` to ignore the
`ParseException`, but notice that in this case, these properties will be
ignored too, e.g `SET DBPROTERTIES('location'='/tmp')` will affect nothing. In
Spark version 2.4 and earlier, these properties are neither reserved nor have
side effects, e.g. `SET DBPROTERTIES('location'='/tmp')` will not change the
location of the database but only create a headless property just like
`'a'='b'`.
Review comment:
`notice that in this case, these properties will be ignored too ...` We
don't need to highlight it as it's the same with 2.4
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL] Disallow to specify reserved properties in CREATE NAMESPACE syntax
cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL]
Disallow to specify reserved properties in CREATE NAMESPACE syntax
URL: https://github.com/apache/spark/pull/26806#discussion_r363645236
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
##
@@ -2498,6 +2498,26 @@ class AstBuilder(conf: SQLConf) extends
SqlBaseBaseVisitor[AnyRef] with Logging
}
}
+ private def checkNamespaceProperties(
+ properties: Map[String, String],
+ ctx: ParserRuleContext): Map[String, String] = withOrigin(ctx) {
+import SupportsNamespaces._
+if (!conf.getConf(SQLConf.LEGACY_PROPERTY_NON_RESERVED)) {
+ properties.foreach {
+case (PROP_LOCATION, _) =>
+ throw new ParseException(s"$PROP_LOCATION is a reserved" + s"
namespace property," +
Review comment:
why break down the string 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL] Disallow to specify reserved properties in CREATE NAMESPACE syntax
cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL]
Disallow to specify reserved properties in CREATE NAMESPACE syntax
URL: https://github.com/apache/spark/pull/26806#discussion_r363626739
##
File path: docs/sql-migration-guide.md
##
@@ -264,6 +264,8 @@ license: |
- Since Spark 3.0, the function `percentile_approx` and its alias
`approx_percentile` only accept integral value with range in `[1, 2147483647]`
as its 3rd argument `accuracy`, fractional and string types are disallowed,
e.g. `percentile_approx(10.0, 0.2, 1.8D)` will cause `AnalysisException`. In
Spark version 2.4 and earlier, if `accuracy` is fractional or string value, it
will be coerced to an int value, `percentile_approx(10.0, 0.2, 1.8D)` is
operated as `percentile_approx(10.0, 0.2, 1)` which results in `10.0`.
+ - Since Spark 3.0, the namespace properties `location` and `comment` become
reserved, it will fail with `ParseException` if we use them as members of
`DBPROTERTIES` in `CREATE NAMESPACE` and `ALTER NAMESPACE ... SET
PROPERTIES(...)`. We need their specific clauses to specify them, e.g. `CREATE
NAMESPACE a.b.c COMMENT 'any comment' LOCATION 'some path'`. We can set
`spark.sql.legacy.property.nonReserved` to `true` to ignore the
`ParseException`, but notice that in this case, these properties will produce
side effects, e.g `SET DBPROTERTIES('location'='/tmp')` might change the
location of the database. In Spark version 2.4 and earlier, these properties
are neither reserved nor have side effects, e.g. `SET
DBPROTERTIES('location'='/tmp')` will not change the location of the database
but create a headless property just like `'a'='b'`.
Review comment:
Since it's the migration guide, I think it's better to use the syntax of
Spark 2.4
```
it will fail with `ParseException` if we use them in `CREATE DATABASE ...
DBPROPERTIES` and `ALTER DATABASE ... SET DBPROPERTIES` ...
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL] Disallow to specify reserved properties in CREATE NAMESPACE syntax
cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL]
Disallow to specify reserved properties in CREATE NAMESPACE syntax
URL: https://github.com/apache/spark/pull/26806#discussion_r363626405
##
File path: docs/sql-migration-guide.md
##
@@ -264,6 +264,8 @@ license: |
- Since Spark 3.0, the function `percentile_approx` and its alias
`approx_percentile` only accept integral value with range in `[1, 2147483647]`
as its 3rd argument `accuracy`, fractional and string types are disallowed,
e.g. `percentile_approx(10.0, 0.2, 1.8D)` will cause `AnalysisException`. In
Spark version 2.4 and earlier, if `accuracy` is fractional or string value, it
will be coerced to an int value, `percentile_approx(10.0, 0.2, 1.8D)` is
operated as `percentile_approx(10.0, 0.2, 1)` which results in `10.0`.
+ - Since Spark 3.0, the namespace properties `location` and `comment` become
reserved, it will fail with `ParseException` if we use them as members of
`DBPROTERTIES` in `CREATE NAMESPACE` and `ALTER NAMESPACE ... SET
PROPERTIES(...)`. We need their specific clauses to specify them, e.g. `CREATE
NAMESPACE a.b.c COMMENT 'any comment' LOCATION 'some path'`. We can set
`spark.sql.legacy.property.nonReserved` to `true` to ignore the
`ParseException`, but notice that in this case, these properties will produce
side effects, e.g `SET DBPROTERTIES('location'='/tmp')` might change the
location of the database. In Spark version 2.4 and earlier, these properties
are neither reserved nor have side effects, e.g. `SET
DBPROTERTIES('location'='/tmp')` will not change the location of the database
but create a headless property just like `'a'='b'`.
+
Review comment:
hey, we should ignore them (i.e. remove them from properties), so that they
won't have side effect.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL] Disallow to specify reserved properties in CREATE NAMESPACE syntax
cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL]
Disallow to specify reserved properties in CREATE NAMESPACE syntax
URL: https://github.com/apache/spark/pull/26806#discussion_r363583751
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
##
@@ -2525,11 +2526,17 @@ class AstBuilder(conf: SQLConf) extends
SqlBaseBaseVisitor[AnyRef] with Logging
var properties = ctx.tablePropertyList.asScala.headOption
.map(visitPropertyKeyValues)
.getOrElse(Map.empty)
-Option(ctx.comment).map(string).map {
- properties += SupportsNamespaces.PROP_COMMENT -> _
+
+if (properties.keySet.exists(RESERVED_PROPERTIES.contains)) {
+ throw new ParseException(s"Disallow to specify reserved properties,
including" +
Review comment:
I think it's OK that the legacy config can only ignore the reserved
properties but not treat them as user-side properties. At least the query won't
fail.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL] Disallow to specify reserved properties in CREATE NAMESPACE syntax
cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL]
Disallow to specify reserved properties in CREATE NAMESPACE syntax
URL: https://github.com/apache/spark/pull/26806#discussion_r363581297
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
##
@@ -2525,11 +2526,17 @@ class AstBuilder(conf: SQLConf) extends
SqlBaseBaseVisitor[AnyRef] with Logging
var properties = ctx.tablePropertyList.asScala.headOption
.map(visitPropertyKeyValues)
.getOrElse(Map.empty)
-Option(ctx.comment).map(string).map {
- properties += SupportsNamespaces.PROP_COMMENT -> _
+
+if (properties.keySet.exists(RESERVED_PROPERTIES.contains)) {
+ throw new ParseException(s"Disallow to specify reserved properties,
including" +
Review comment:
ah so in Spark 2, the reserved properties are simply ignored?
BTW this error message is for end users, we should try our best to make it
helpful, even if it means more code.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL] Disallow to specify reserved properties in CREATE NAMESPACE syntax
cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL]
Disallow to specify reserved properties in CREATE NAMESPACE syntax
URL: https://github.com/apache/spark/pull/26806#discussion_r363315150
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
##
@@ -2525,11 +2526,17 @@ class AstBuilder(conf: SQLConf) extends
SqlBaseBaseVisitor[AnyRef] with Logging
var properties = ctx.tablePropertyList.asScala.headOption
.map(visitPropertyKeyValues)
.getOrElse(Map.empty)
-Option(ctx.comment).map(string).map {
- properties += SupportsNamespaces.PROP_COMMENT -> _
+
+if (properties.keySet.exists(RESERVED_PROPERTIES.contains)) {
+ throw new ParseException(s"Disallow to specify reserved properties,
including" +
Review comment:
after some more thought, I think this is a breaking change. Existing queries
may already use the "comment" property. Can we add a legacy config and update
migration guide?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL] Disallow to specify reserved properties in CREATE NAMESPACE syntax
cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL]
Disallow to specify reserved properties in CREATE NAMESPACE syntax
URL: https://github.com/apache/spark/pull/26806#discussion_r363312915
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
##
@@ -2525,11 +2526,17 @@ class AstBuilder(conf: SQLConf) extends
SqlBaseBaseVisitor[AnyRef] with Logging
var properties = ctx.tablePropertyList.asScala.headOption
.map(visitPropertyKeyValues)
.getOrElse(Map.empty)
-Option(ctx.comment).map(string).map {
- properties += SupportsNamespaces.PROP_COMMENT -> _
+
+if (properties.keySet.exists(RESERVED_PROPERTIES.contains)) {
+ throw new ParseException(s"Disallow to specify reserved properties,
including" +
Review comment:
we can make the error message better
```
properties.foreach { case (key, _)
if (key == PROP_COMMENT) {
fail(s"$key is a reserved namespace property, please use the COMMENT
clause to specify it.")
}
...
}
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL] Disallow to specify reserved properties in CREATE NAMESPACE syntax
cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL]
Disallow to specify reserved properties in CREATE NAMESPACE syntax
URL: https://github.com/apache/spark/pull/26806#discussion_r363312915
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
##
@@ -2525,11 +2526,17 @@ class AstBuilder(conf: SQLConf) extends
SqlBaseBaseVisitor[AnyRef] with Logging
var properties = ctx.tablePropertyList.asScala.headOption
.map(visitPropertyKeyValues)
.getOrElse(Map.empty)
-Option(ctx.comment).map(string).map {
- properties += SupportsNamespaces.PROP_COMMENT -> _
+
+if (properties.keySet.exists(RESERVED_PROPERTIES.contains)) {
+ throw new ParseException(s"Disallow to specify reserved properties,
including" +
Review comment:
we can make the error message better
```
properties.foreach { case (key, _)
if (RESERVED_PROPERTIES.contains(key)) {
fail(s"$key is a reserved namespace property.")
}
}
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL] Disallow to specify reserved properties in CREATE NAMESPACE syntax
cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL]
Disallow to specify reserved properties in CREATE NAMESPACE syntax
URL: https://github.com/apache/spark/pull/26806#discussion_r363312915
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
##
@@ -2525,11 +2526,17 @@ class AstBuilder(conf: SQLConf) extends
SqlBaseBaseVisitor[AnyRef] with Logging
var properties = ctx.tablePropertyList.asScala.headOption
.map(visitPropertyKeyValues)
.getOrElse(Map.empty)
-Option(ctx.comment).map(string).map {
- properties += SupportsNamespaces.PROP_COMMENT -> _
+
+if (properties.keySet.exists(RESERVED_PROPERTIES.contains)) {
+ throw new ParseException(s"Disallow to specify reserved properties,
including" +
Review comment:
we can make the error message better
```
properties.foreach { case (key, _)
if (RESERVED_PROPERTIES.contains(key)) {
fail(s"$key is a reserved properties.")
}
}
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL] Disallow to specify reserved properties in CREATE NAMESPACE syntax
cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL]
Disallow to specify reserved properties in CREATE NAMESPACE syntax
URL: https://github.com/apache/spark/pull/26806#discussion_r363151225
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
##
@@ -2525,17 +2526,21 @@ class AstBuilder(conf: SQLConf) extends
SqlBaseBaseVisitor[AnyRef] with Logging
var properties = ctx.tablePropertyList.asScala.headOption
.map(visitPropertyKeyValues)
.getOrElse(Map.empty)
-Option(ctx.comment).map(string).map {
- properties += SupportsNamespaces.PROP_COMMENT -> _
+
+if
(properties.keySet.intersect(RESERVED_PROPERTIES.asScala.toSet).nonEmpty) {
+ throw new ParseException(s"Disallow to specify reserved properties,
including" +
+s" ${RESERVED_PROPERTIES.asScala.mkString("(", ",", ")")}.", ctx)
+}
+
+Option(ctx.comment).map(string).foreach {
+ properties += PROP_COMMENT -> _
}
-ctx.locationSpec.asScala.headOption.map(visitLocationSpec).map {
- properties += SupportsNamespaces.PROP_LOCATION -> _
+ctx.locationSpec.asScala.headOption.map(visitLocationSpec).foreach {
Review comment:
let's leave it as it is for 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL] Disallow to specify reserved properties in CREATE NAMESPACE syntax
cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL]
Disallow to specify reserved properties in CREATE NAMESPACE syntax
URL: https://github.com/apache/spark/pull/26806#discussion_r363151296
##
File path:
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala
##
@@ -25,6 +25,7 @@ import org.apache.spark.sql.catalyst.catalog.{BucketSpec,
CatalogTable, CatalogT
import org.apache.spark.sql.catalyst.plans.logical._
import org.apache.spark.sql.catalyst.rules.Rule
import org.apache.spark.sql.connector.catalog.{CatalogManager, CatalogPlugin,
LookupCatalog, SupportsNamespaces, Table, TableCatalog, TableChange, V1Table}
+import org.apache.spark.sql.connector.catalog.CatalogV2Util.isSessionCatalog
Review comment:
unnecessary change
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL] Disallow to specify reserved properties in CREATE NAMESPACE syntax
cloud-fan commented on a change in pull request #26806: [SPARK-30183][SQL]
Disallow to specify reserved properties in CREATE NAMESPACE syntax
URL: https://github.com/apache/spark/pull/26806#discussion_r363150965
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
##
@@ -2525,11 +2526,17 @@ class AstBuilder(conf: SQLConf) extends
SqlBaseBaseVisitor[AnyRef] with Logging
var properties = ctx.tablePropertyList.asScala.headOption
.map(visitPropertyKeyValues)
.getOrElse(Map.empty)
-Option(ctx.comment).map(string).map {
- properties += SupportsNamespaces.PROP_COMMENT -> _
+
+if
(properties.keySet.intersect(RESERVED_PROPERTIES.asScala.toSet).nonEmpty) {
Review comment:
nit:
```
properties.keySet.exists(RESERVED_PROPERTIES.contains)
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
