You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/11/10 04:20:15 UTC
[GitHub] [spark] amaliujia opened a new pull request, #38595: [SPARK-41090][SQL] Enhance Dataset.createTempView testing coverage for `db_name.view_name`
amaliujia opened a new pull request, #38595:
URL: https://github.com/apache/spark/pull/38595
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
We can also test `createTempView` with db name included into the parameter.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
Improve testing coverage.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
NO
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
UT
--
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] amaliujia commented on pull request #38595: [SPARK-41090][SQL] Fix view not found issue for `db_name.view_name`
Posted by GitBox <gi...@apache.org>.
amaliujia commented on PR #38595:
URL: https://github.com/apache/spark/pull/38595#issuecomment-1311060692
a bunch of tests are failing due to this simple fix. We need more discussions probably on how to to fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #38595: [SPARK-41090][SQL] Throw Exception for `db_name.view_name` when creating temp view by Dataset API
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38595:
URL: https://github.com/apache/spark/pull/38595#discussion_r1022520366
##########
sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala:
##########
@@ -3799,13 +3798,21 @@ class Dataset[T] private[sql](
global: Boolean): CreateViewCommand = {
val viewType = if (global) GlobalTempView else LocalTempView
- val tableIdentifier = try {
- sparkSession.sessionState.sqlParser.parseTableIdentifier(viewName)
+ val identifier = try {
+ sparkSession.sessionState.sqlParser.parseMultipartIdentifier(viewName)
} catch {
case _: ParseException => throw QueryCompilationErrors.invalidViewNameError(viewName)
}
+
+ if (!SQLConf.get.allowsTempViewCreationWithMultipleNameparts && identifier.size > 1) {
+ // Temporary view names should NOT contain database prefix like "database.table"
+ throw new AnalysisException(
+ errorClass = "TEMP_VIEW_NAME_CONTAINS_UNSUPPORTED_NAME_PARTS",
+ messageParameters = Map("actualName" -> viewName))
+ }
+
CreateViewCommand(
- name = tableIdentifier,
+ name = TableIdentifier.apply(identifier.last),
Review Comment:
```suggestion
name = TableIdentifier(identifier.head),
```
##########
sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala:
##########
@@ -3799,13 +3798,21 @@ class Dataset[T] private[sql](
global: Boolean): CreateViewCommand = {
val viewType = if (global) GlobalTempView else LocalTempView
- val tableIdentifier = try {
- sparkSession.sessionState.sqlParser.parseTableIdentifier(viewName)
+ val identifier = try {
+ sparkSession.sessionState.sqlParser.parseMultipartIdentifier(viewName)
} catch {
case _: ParseException => throw QueryCompilationErrors.invalidViewNameError(viewName)
}
+
+ if (!SQLConf.get.allowsTempViewCreationWithMultipleNameparts && identifier.size > 1) {
+ // Temporary view names should NOT contain database prefix like "database.table"
+ throw new AnalysisException(
+ errorClass = "TEMP_VIEW_NAME_CONTAINS_UNSUPPORTED_NAME_PARTS",
+ messageParameters = Map("actualName" -> viewName))
+ }
+
CreateViewCommand(
- name = tableIdentifier,
+ name = TableIdentifier.apply(identifier.last),
Review Comment:
```suggestion
name = TableIdentifier(identifier.last),
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #38595: [SPARK-41090][SQL] Throw Exception for `db_name.view_name` when creating temp view by Dataset API
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38595:
URL: https://github.com/apache/spark/pull/38595#discussion_r1022515695
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -933,6 +933,11 @@
],
"sqlState" : "42000"
},
+ "TEMP_VIEW_NAME_CONTAINS_UNSUPPORTED_NAME_PARTS" : {
Review Comment:
error class name doesn't have to be super clear to make it short. How about `TEMP_VIEW_NAME_TOO_MANY_NAME_PARTS `
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] amaliujia commented on a diff in pull request #38595: [SPARK-41090][SQL] Enhance Dataset.createTempView testing coverage for `db_name.view_name`
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38595:
URL: https://github.com/apache/spark/pull/38595#discussion_r1018736363
##########
sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala:
##########
@@ -1135,21 +1135,27 @@ class DatasetSuite extends QueryTest
}
test("createTempView") {
- val dataset = Seq(1, 2, 3).toDS()
- dataset.createOrReplaceTempView("tempView")
-
- // Overrides the existing temporary view with same name
- // No exception should be thrown here.
- dataset.createOrReplaceTempView("tempView")
-
- // Throws AnalysisException if temp view with same name already exists
- val e = intercept[AnalysisException](
- dataset.createTempView("tempView"))
- intercept[AnalysisException](dataset.createTempView("tempView"))
- checkError(e,
- errorClass = "TEMP_TABLE_OR_VIEW_ALREADY_EXISTS",
- parameters = Map("relationName" -> "`tempView`"))
- dataset.sparkSession.catalog.dropTempView("tempView")
+ withDatabase("test_db") {
+ val dataset = Seq(1, 2, 3).toDS()
+ dataset.createOrReplaceTempView("tempView")
+
+ // Overrides the existing temporary view with same name
+ // No exception should be thrown here.
+ dataset.createOrReplaceTempView("tempView")
+
+ // Throws AnalysisException if temp view with same name already exists
+ val e = intercept[AnalysisException](
+ dataset.createTempView("tempView"))
+ intercept[AnalysisException](dataset.createTempView("tempView"))
+ checkError(e,
+ errorClass = "TEMP_TABLE_OR_VIEW_ALREADY_EXISTS",
+ parameters = Map("relationName" -> "`tempView`"))
+ dataset.sparkSession.catalog.dropTempView("tempView")
+
+ spark.sql("CREATE DATABASE IF NOT EXISTS test_db")
+ dataset.createTempView("test_db.tempView")
+ spark.catalog.tableExists("test_db.tempView")
Review Comment:
Oops. Let me check....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #38595: [SPARK-41090][SQL] Throw Exception for `db_name.view_name` when creating temp view by Dataset API
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38595:
URL: https://github.com/apache/spark/pull/38595#discussion_r1021102010
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -933,6 +933,11 @@
],
"sqlState" : "42000"
},
+ "TEMP_VIEW_DOES_NOT_BELONG_TO_A_DATABASE" : {
Review Comment:
Can we update the error class name as well? And the caller side should use `parseMultiPartIdentifier`
--
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] amaliujia commented on pull request #38595: [SPARK-41090][SQL] Fix view not found issue for `db_name.view_name`
Posted by GitBox <gi...@apache.org>.
amaliujia commented on PR #38595:
URL: https://github.com/apache/spark/pull/38595#issuecomment-1310762222
@cloud-fan I added a fix but not sure about the following:
Looks like for Dataset/DataFrame temp view, it does not link a temp view to a DB. Do we still want to follow this design?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #38595: [SPARK-41090][SQL] Enhance Dataset.createTempView testing coverage for `db_name.view_name`
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38595:
URL: https://github.com/apache/spark/pull/38595#discussion_r1018735442
##########
sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala:
##########
@@ -1135,21 +1135,27 @@ class DatasetSuite extends QueryTest
}
test("createTempView") {
- val dataset = Seq(1, 2, 3).toDS()
- dataset.createOrReplaceTempView("tempView")
-
- // Overrides the existing temporary view with same name
- // No exception should be thrown here.
- dataset.createOrReplaceTempView("tempView")
-
- // Throws AnalysisException if temp view with same name already exists
- val e = intercept[AnalysisException](
- dataset.createTempView("tempView"))
- intercept[AnalysisException](dataset.createTempView("tempView"))
- checkError(e,
- errorClass = "TEMP_TABLE_OR_VIEW_ALREADY_EXISTS",
- parameters = Map("relationName" -> "`tempView`"))
- dataset.sparkSession.catalog.dropTempView("tempView")
+ withDatabase("test_db") {
+ val dataset = Seq(1, 2, 3).toDS()
+ dataset.createOrReplaceTempView("tempView")
+
+ // Overrides the existing temporary view with same name
+ // No exception should be thrown here.
+ dataset.createOrReplaceTempView("tempView")
+
+ // Throws AnalysisException if temp view with same name already exists
+ val e = intercept[AnalysisException](
+ dataset.createTempView("tempView"))
+ intercept[AnalysisException](dataset.createTempView("tempView"))
+ checkError(e,
+ errorClass = "TEMP_TABLE_OR_VIEW_ALREADY_EXISTS",
+ parameters = Map("relationName" -> "`tempView`"))
+ dataset.sparkSession.catalog.dropTempView("tempView")
+
+ spark.sql("CREATE DATABASE IF NOT EXISTS test_db")
+ dataset.createTempView("test_db.tempView")
+ spark.catalog.tableExists("test_db.tempView")
Review Comment:
shouldn't we check the result of `spark.catalog.tableExists`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #38595: [SPARK-41090][SQL] Fix view not found issue for `db_name.view_name`
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38595:
URL: https://github.com/apache/spark/pull/38595#discussion_r1019835019
##########
sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala:
##########
@@ -3804,6 +3804,13 @@ class Dataset[T] private[sql](
} catch {
case _: ParseException => throw QueryCompilationErrors.invalidViewNameError(viewName)
}
+
+ if (tableIdentifier.database.isDefined) {
+ throw new AnalysisException(
+ errorClass = "TEMP_VIEW_DOES_NOT_BELONG_TO_A_DATABASE",
Review Comment:
In the parser side, we have this
```
val tableIdentifier = visitMultipartIdentifier(ctx.multipartIdentifier).asTableIdentifier
if (tableIdentifier.database.isDefined) {
// Temporary view names should NOT contain database prefix like "database.table"
throw QueryParsingErrors
.notAllowedToAddDBPrefixForTempViewError(tableIdentifier.database.get, ctx)
}
```
shall we reuse the error class 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] amaliujia commented on a diff in pull request #38595: [SPARK-41090][SQL] Throw Exception for `db_name.view_name` when creating temp view by Dataset API
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38595:
URL: https://github.com/apache/spark/pull/38595#discussion_r1022185871
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -933,6 +933,11 @@
],
"sqlState" : "42000"
},
+ "TEMP_VIEW_DOES_NOT_BELONG_TO_A_DATABASE" : {
Review Comment:
Sure so I
1. Updated the error class name and config key to reflect the multiple name parts.
2. use `parseMultiPartIdentifier` to catch more than 1 part.
3. fix 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] cloud-fan commented on a diff in pull request #38595: [SPARK-41090][SQL] Throw Exception for `db_name.view_name` when creating temp view by Dataset API
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38595:
URL: https://github.com/apache/spark/pull/38595#discussion_r1021102010
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -933,6 +933,11 @@
],
"sqlState" : "42000"
},
+ "TEMP_VIEW_DOES_NOT_BELONG_TO_A_DATABASE" : {
Review Comment:
Can we update the error class name as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #38595: [SPARK-41090][SQL] Throw Exception for `db_name.view_name` when creating temp view by Dataset API
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on PR #38595:
URL: https://github.com/apache/spark/pull/38595#issuecomment-1317932109
thanks, merging to master!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] amaliujia commented on pull request #38595: [SPARK-41090][SQL] Enhance Dataset.createTempView testing coverage for `db_name.view_name`
Posted by GitBox <gi...@apache.org>.
amaliujia commented on PR #38595:
URL: https://github.com/apache/spark/pull/38595#issuecomment-1309750194
cc @cloud-fan
Looks like the `createTempView` works well. Please see my added test case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #38595: [SPARK-41090][SQL] Throw Exception for `db_name.view_name` when creating temp view by Dataset API
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38595:
URL: https://github.com/apache/spark/pull/38595#discussion_r1021044706
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -933,6 +933,11 @@
],
"sqlState" : "42000"
},
+ "TEMP_VIEW_DOES_NOT_BELONG_TO_A_DATABASE" : {
Review Comment:
Since we are touching this error, let's take the chance to refine it. How about
```
"CREATE_TEMP_VIEW_SINGLE_PART_NAME": {
"message": [
"CREATE TEMPORARY VIEW or the corresponding Dataset APIs only accept single-part view names, but got: <actualName>"
]
}
```
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -933,6 +933,11 @@
],
"sqlState" : "42000"
},
+ "TEMP_VIEW_DOES_NOT_BELONG_TO_A_DATABASE" : {
Review Comment:
cc @MaxGekk
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #38595: [SPARK-41090][SQL] Throw Exception for `db_name.view_name` when creating temp view by Dataset API
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38595:
URL: https://github.com/apache/spark/pull/38595#discussion_r1021045167
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -2162,6 +2162,16 @@ object SQLConf {
.booleanConf
.createWithDefault(false)
+ val ALLOW_TEMP_VIEW_CREATION_WITH_DATABASE_NAME =
+ buildConf("spark.sql.legacy.allowTempViewCreationWithDatabaseName")
+ .internal()
+ .doc("When true, DataSet createTempView will allow the view creation even if " +
Review Comment:
let's say `temp view creation Dataset APIs`, as there are a bunch 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] cloud-fan commented on a diff in pull request #38595: [SPARK-41090][SQL] Throw Exception for `db_name.view_name` when creating temp view by Dataset API
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38595:
URL: https://github.com/apache/spark/pull/38595#discussion_r1021062648
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -933,6 +933,11 @@
],
"sqlState" : "42000"
},
+ "TEMP_VIEW_DOES_NOT_BELONG_TO_A_DATABASE" : {
Review Comment:
oh I see
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #38595: [SPARK-41090][SQL] Throw Exception for `db_name.view_name` when creating temp view
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #38595:
URL: https://github.com/apache/spark/pull/38595#issuecomment-1312406724
Can one of the admins verify this patch?
--
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] amaliujia commented on a diff in pull request #38595: [SPARK-41090][SQL] Fix view not found issue for `db_name.view_name`
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38595:
URL: https://github.com/apache/spark/pull/38595#discussion_r1019834165
##########
sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala:
##########
@@ -1135,21 +1135,27 @@ class DatasetSuite extends QueryTest
}
test("createTempView") {
- val dataset = Seq(1, 2, 3).toDS()
- dataset.createOrReplaceTempView("tempView")
-
- // Overrides the existing temporary view with same name
- // No exception should be thrown here.
- dataset.createOrReplaceTempView("tempView")
-
- // Throws AnalysisException if temp view with same name already exists
- val e = intercept[AnalysisException](
- dataset.createTempView("tempView"))
- intercept[AnalysisException](dataset.createTempView("tempView"))
- checkError(e,
- errorClass = "TEMP_TABLE_OR_VIEW_ALREADY_EXISTS",
- parameters = Map("relationName" -> "`tempView`"))
- dataset.sparkSession.catalog.dropTempView("tempView")
+ withDatabase("test_db") {
+ val dataset = Seq(1, 2, 3).toDS()
+ dataset.createOrReplaceTempView("tempView")
+
+ // Overrides the existing temporary view with same name
+ // No exception should be thrown here.
+ dataset.createOrReplaceTempView("tempView")
+
+ // Throws AnalysisException if temp view with same name already exists
+ val e = intercept[AnalysisException](
+ dataset.createTempView("tempView"))
+ intercept[AnalysisException](dataset.createTempView("tempView"))
+ checkError(e,
+ errorClass = "TEMP_TABLE_OR_VIEW_ALREADY_EXISTS",
+ parameters = Map("relationName" -> "`tempView`"))
+ dataset.sparkSession.catalog.dropTempView("tempView")
+
+ spark.sql("CREATE DATABASE IF NOT EXISTS test_db")
+ dataset.createTempView("test_db.tempView")
Review Comment:
Choose to throw when DB name presents.
Waiting for test job to see if there is existing test case that needs an update.
--
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] amaliujia commented on a diff in pull request #38595: [SPARK-41090][SQL] Throw Exception for `db_name.view_name` when creating temp view by Dataset API
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38595:
URL: https://github.com/apache/spark/pull/38595#discussion_r1021050959
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -933,6 +933,11 @@
],
"sqlState" : "42000"
},
+ "TEMP_VIEW_DOES_NOT_BELONG_TO_A_DATABASE" : {
Review Comment:
I believe still need ` case _: ParseException => throw QueryCompilationErrors.invalidViewNameError(viewName)`
Per my understanding that will happen if someone uses `$#Qviewname` as view name which is not allowed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #38595: [SPARK-41090][SQL] Fix view not found issue for `db_name.view_name`
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38595:
URL: https://github.com/apache/spark/pull/38595#discussion_r1019785842
##########
sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala:
##########
@@ -1135,21 +1135,27 @@ class DatasetSuite extends QueryTest
}
test("createTempView") {
- val dataset = Seq(1, 2, 3).toDS()
- dataset.createOrReplaceTempView("tempView")
-
- // Overrides the existing temporary view with same name
- // No exception should be thrown here.
- dataset.createOrReplaceTempView("tempView")
-
- // Throws AnalysisException if temp view with same name already exists
- val e = intercept[AnalysisException](
- dataset.createTempView("tempView"))
- intercept[AnalysisException](dataset.createTempView("tempView"))
- checkError(e,
- errorClass = "TEMP_TABLE_OR_VIEW_ALREADY_EXISTS",
- parameters = Map("relationName" -> "`tempView`"))
- dataset.sparkSession.catalog.dropTempView("tempView")
+ withDatabase("test_db") {
+ val dataset = Seq(1, 2, 3).toDS()
+ dataset.createOrReplaceTempView("tempView")
+
+ // Overrides the existing temporary view with same name
+ // No exception should be thrown here.
+ dataset.createOrReplaceTempView("tempView")
+
+ // Throws AnalysisException if temp view with same name already exists
+ val e = intercept[AnalysisException](
+ dataset.createTempView("tempView"))
+ intercept[AnalysisException](dataset.createTempView("tempView"))
+ checkError(e,
+ errorClass = "TEMP_TABLE_OR_VIEW_ALREADY_EXISTS",
+ parameters = Map("relationName" -> "`tempView`"))
+ dataset.sparkSession.catalog.dropTempView("tempView")
+
+ spark.sql("CREATE DATABASE IF NOT EXISTS test_db")
+ dataset.createTempView("test_db.tempView")
Review Comment:
local temp view can't have a database, the expected behavior should be failure 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] amaliujia commented on a diff in pull request #38595: [SPARK-41090][SQL] Fix view not found issue for `db_name.view_name`
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38595:
URL: https://github.com/apache/spark/pull/38595#discussion_r1020516505
##########
sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala:
##########
@@ -3804,6 +3804,13 @@ class Dataset[T] private[sql](
} catch {
case _: ParseException => throw QueryCompilationErrors.invalidViewNameError(viewName)
}
+
+ if (tableIdentifier.database.isDefined) {
+ throw new AnalysisException(
+ errorClass = "TEMP_VIEW_DOES_NOT_BELONG_TO_A_DATABASE",
Review Comment:
The config idea sounds good.
I found the parser side is using `_LEGACY_ERROR_TEMP_0054`.
```
def notAllowedToAddDBPrefixForTempViewError(
database: String,
ctx: CreateViewContext): Throwable = {
new ParseException(
errorClass = "_LEGACY_ERROR_TEMP_0054",
messageParameters = Map("database" -> database),
ctx)
}
```
We can use the same error message body but at least we should rename this error class name.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] amaliujia commented on a diff in pull request #38595: [SPARK-41090][SQL] Fix view not found issue for `db_name.view_name`
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38595:
URL: https://github.com/apache/spark/pull/38595#discussion_r1019816022
##########
sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala:
##########
@@ -1135,21 +1135,27 @@ class DatasetSuite extends QueryTest
}
test("createTempView") {
- val dataset = Seq(1, 2, 3).toDS()
- dataset.createOrReplaceTempView("tempView")
-
- // Overrides the existing temporary view with same name
- // No exception should be thrown here.
- dataset.createOrReplaceTempView("tempView")
-
- // Throws AnalysisException if temp view with same name already exists
- val e = intercept[AnalysisException](
- dataset.createTempView("tempView"))
- intercept[AnalysisException](dataset.createTempView("tempView"))
- checkError(e,
- errorClass = "TEMP_TABLE_OR_VIEW_ALREADY_EXISTS",
- parameters = Map("relationName" -> "`tempView`"))
- dataset.sparkSession.catalog.dropTempView("tempView")
+ withDatabase("test_db") {
+ val dataset = Seq(1, 2, 3).toDS()
+ dataset.createOrReplaceTempView("tempView")
+
+ // Overrides the existing temporary view with same name
+ // No exception should be thrown here.
+ dataset.createOrReplaceTempView("tempView")
+
+ // Throws AnalysisException if temp view with same name already exists
+ val e = intercept[AnalysisException](
+ dataset.createTempView("tempView"))
+ intercept[AnalysisException](dataset.createTempView("tempView"))
+ checkError(e,
+ errorClass = "TEMP_TABLE_OR_VIEW_ALREADY_EXISTS",
+ parameters = Map("relationName" -> "`tempView`"))
+ dataset.sparkSession.catalog.dropTempView("tempView")
+
+ spark.sql("CREATE DATABASE IF NOT EXISTS test_db")
+ dataset.createTempView("test_db.tempView")
Review Comment:
yes that is what I was thinking.
Let me update to reflect that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #38595: [SPARK-41090][SQL] Fix view not found issue for `db_name.view_name`
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38595:
URL: https://github.com/apache/spark/pull/38595#discussion_r1019835252
##########
sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala:
##########
@@ -3804,6 +3804,13 @@ class Dataset[T] private[sql](
} catch {
case _: ParseException => throw QueryCompilationErrors.invalidViewNameError(viewName)
}
+
+ if (tableIdentifier.database.isDefined) {
+ throw new AnalysisException(
+ errorClass = "TEMP_VIEW_DOES_NOT_BELONG_TO_A_DATABASE",
Review Comment:
And this is a breaking change, can we add a legacy config to restore the previous behavior? (ignore the database)
--
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] amaliujia commented on a diff in pull request #38595: [SPARK-41090][SQL] Fix view not found issue for `db_name.view_name`
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38595:
URL: https://github.com/apache/spark/pull/38595#discussion_r1019836218
##########
sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala:
##########
@@ -3804,6 +3804,13 @@ class Dataset[T] private[sql](
} catch {
case _: ParseException => throw QueryCompilationErrors.invalidViewNameError(viewName)
}
+
+ if (tableIdentifier.database.isDefined) {
+ throw new AnalysisException(
+ errorClass = "TEMP_VIEW_DOES_NOT_BELONG_TO_A_DATABASE",
Review Comment:
What should be default behavior? By default ignore the database or throw exception?
--
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] amaliujia commented on a diff in pull request #38595: [SPARK-41090][SQL] Enhance Dataset.createTempView testing coverage for `db_name.view_name`
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38595:
URL: https://github.com/apache/spark/pull/38595#discussion_r1018641366
##########
sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala:
##########
@@ -1135,21 +1135,27 @@ class DatasetSuite extends QueryTest
}
test("createTempView") {
- val dataset = Seq(1, 2, 3).toDS()
- dataset.createOrReplaceTempView("tempView")
-
- // Overrides the existing temporary view with same name
- // No exception should be thrown here.
- dataset.createOrReplaceTempView("tempView")
-
- // Throws AnalysisException if temp view with same name already exists
- val e = intercept[AnalysisException](
- dataset.createTempView("tempView"))
- intercept[AnalysisException](dataset.createTempView("tempView"))
- checkError(e,
- errorClass = "TEMP_TABLE_OR_VIEW_ALREADY_EXISTS",
- parameters = Map("relationName" -> "`tempView`"))
- dataset.sparkSession.catalog.dropTempView("tempView")
+ withDatabase("test_db") {
+ val dataset = Seq(1, 2, 3).toDS()
+ dataset.createOrReplaceTempView("tempView")
+
+ // Overrides the existing temporary view with same name
+ // No exception should be thrown here.
+ dataset.createOrReplaceTempView("tempView")
+
+ // Throws AnalysisException if temp view with same name already exists
+ val e = intercept[AnalysisException](
+ dataset.createTempView("tempView"))
+ intercept[AnalysisException](dataset.createTempView("tempView"))
+ checkError(e,
+ errorClass = "TEMP_TABLE_OR_VIEW_ALREADY_EXISTS",
+ parameters = Map("relationName" -> "`tempView`"))
+ dataset.sparkSession.catalog.dropTempView("tempView")
+
+ spark.sql("CREATE DATABASE IF NOT EXISTS test_db")
+ dataset.createTempView("test_db.tempView")
+ spark.catalog.tableExists("test_db.tempView")
Review Comment:
Here is the case that I added.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #38595: [SPARK-41090][SQL] Fix view not found issue for `db_name.view_name`
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38595:
URL: https://github.com/apache/spark/pull/38595#discussion_r1019839623
##########
sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala:
##########
@@ -3804,6 +3804,13 @@ class Dataset[T] private[sql](
} catch {
case _: ParseException => throw QueryCompilationErrors.invalidViewNameError(viewName)
}
+
+ if (tableIdentifier.database.isDefined) {
+ throw new AnalysisException(
+ errorClass = "TEMP_VIEW_DOES_NOT_BELONG_TO_A_DATABASE",
Review Comment:
I think throwing exception is a better default.
--
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] amaliujia commented on a diff in pull request #38595: [SPARK-41090][SQL] Enhance Dataset.createTempView testing coverage for `db_name.view_name`
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38595:
URL: https://github.com/apache/spark/pull/38595#discussion_r1019505789
##########
sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala:
##########
@@ -1135,21 +1135,27 @@ class DatasetSuite extends QueryTest
}
test("createTempView") {
- val dataset = Seq(1, 2, 3).toDS()
- dataset.createOrReplaceTempView("tempView")
-
- // Overrides the existing temporary view with same name
- // No exception should be thrown here.
- dataset.createOrReplaceTempView("tempView")
-
- // Throws AnalysisException if temp view with same name already exists
- val e = intercept[AnalysisException](
- dataset.createTempView("tempView"))
- intercept[AnalysisException](dataset.createTempView("tempView"))
- checkError(e,
- errorClass = "TEMP_TABLE_OR_VIEW_ALREADY_EXISTS",
- parameters = Map("relationName" -> "`tempView`"))
- dataset.sparkSession.catalog.dropTempView("tempView")
+ withDatabase("test_db") {
+ val dataset = Seq(1, 2, 3).toDS()
+ dataset.createOrReplaceTempView("tempView")
+
+ // Overrides the existing temporary view with same name
+ // No exception should be thrown here.
+ dataset.createOrReplaceTempView("tempView")
+
+ // Throws AnalysisException if temp view with same name already exists
+ val e = intercept[AnalysisException](
+ dataset.createTempView("tempView"))
+ intercept[AnalysisException](dataset.createTempView("tempView"))
+ checkError(e,
+ errorClass = "TEMP_TABLE_OR_VIEW_ALREADY_EXISTS",
+ parameters = Map("relationName" -> "`tempView`"))
+ dataset.sparkSession.catalog.dropTempView("tempView")
+
+ spark.sql("CREATE DATABASE IF NOT EXISTS test_db")
+ dataset.createTempView("test_db.tempView")
+ spark.catalog.tableExists("test_db.tempView")
Review Comment:
Updated with a fix.
--
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] amaliujia commented on a diff in pull request #38595: [SPARK-41090][SQL] Throw Exception for `db_name.view_name` when creating temp view by Dataset API
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38595:
URL: https://github.com/apache/spark/pull/38595#discussion_r1023261314
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala:
##########
@@ -542,11 +542,11 @@ private[sql] object QueryParsingErrors extends QueryErrorsBase {
}
def notAllowedToAddDBPrefixForTempViewError(
- database: String,
+ viewName: String,
Review Comment:
Good idea. Done.
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -933,6 +933,11 @@
],
"sqlState" : "42000"
},
+ "TEMP_VIEW_NAME_CONTAINS_UNSUPPORTED_NAME_PARTS" : {
Review Comment:
I see. Updated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #38595: [SPARK-41090][SQL] Throw Exception for `db_name.view_name` when creating temp view by Dataset API
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38595:
URL: https://github.com/apache/spark/pull/38595#discussion_r1021044871
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -2162,6 +2162,16 @@ object SQLConf {
.booleanConf
.createWithDefault(false)
+ val ALLOW_TEMP_VIEW_CREATION_WITH_DATABASE_NAME =
+ buildConf("spark.sql.legacy.allowTempViewCreationWithDatabaseName")
+ .internal()
+ .doc("When true, DataSet createTempView will allow the view creation even if " +
Review Comment:
```suggestion
.doc("When true, Dataset createTempView will allow the view creation even if " +
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #38595: [SPARK-41090][SQL] Throw Exception for `db_name.view_name` when creating temp view by Dataset API
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38595:
URL: https://github.com/apache/spark/pull/38595#discussion_r1021045529
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -933,6 +933,11 @@
],
"sqlState" : "42000"
},
+ "TEMP_VIEW_DOES_NOT_BELONG_TO_A_DATABASE" : {
Review Comment:
By doing this, we don't need https://github.com/apache/spark/pull/38595/files#diff-459628811d7786c705fbb2b7a381ecd2b88f183f44ab607d43b3d33ea48d390fR3805 anymore
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #38595: [SPARK-41090][SQL] Throw Exception for `db_name.view_name` when creating temp view by Dataset API
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38595:
URL: https://github.com/apache/spark/pull/38595#discussion_r1021101689
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -933,6 +933,11 @@
],
"sqlState" : "42000"
},
+ "TEMP_VIEW_DOES_NOT_BELONG_TO_A_DATABASE" : {
Review Comment:
but it's still better to have a single error for 2 part name and 3 part name.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #38595: [SPARK-41090][SQL] Throw Exception for `db_name.view_name` when creating temp view by Dataset API
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38595:
URL: https://github.com/apache/spark/pull/38595#discussion_r1022521452
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala:
##########
@@ -542,11 +542,11 @@ private[sql] object QueryParsingErrors extends QueryErrorsBase {
}
def notAllowedToAddDBPrefixForTempViewError(
- database: String,
+ viewName: String,
Review Comment:
We should pass the view name as `Seq[String]`, and call `toSQLId` in this method to generate a string
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan closed pull request #38595: [SPARK-41090][SQL] Throw Exception for `db_name.view_name` when creating temp view by Dataset API
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #38595: [SPARK-41090][SQL] Throw Exception for `db_name.view_name` when creating temp view by Dataset API
URL: https://github.com/apache/spark/pull/38595
--
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] amaliujia commented on a diff in pull request #38595: [SPARK-41090][SQL] Fix view not found issue for `db_name.view_name`
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38595:
URL: https://github.com/apache/spark/pull/38595#discussion_r1020520053
##########
sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala:
##########
@@ -3804,6 +3804,13 @@ class Dataset[T] private[sql](
} catch {
case _: ParseException => throw QueryCompilationErrors.invalidViewNameError(viewName)
}
+
+ if (tableIdentifier.database.isDefined) {
+ throw new AnalysisException(
+ errorClass = "TEMP_VIEW_DOES_NOT_BELONG_TO_A_DATABASE",
Review Comment:
Ok I basically
1. add a config to control the behavior. By default we throw exception.
2. reuse `_LEGACY_ERROR_TEMP_0054` but rename to a meaningful name.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org