You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "panbingkun (via GitHub)" <gi...@apache.org> on 2023/09/06 02:54:35 UTC
[GitHub] [spark] panbingkun opened a new pull request, #42824: [SPARK-45085][SQL] Merge UNSUPPORTED_TEMP_VIEW_OPERATION into UNSUPPORTED_VIEW_OPERATION and refactor some logic
panbingkun opened a new pull request, #42824:
URL: https://github.com/apache/spark/pull/42824
### What changes were proposed in this pull request?
The pr aims to:
- Merge UNSUPPORTED_TEMP_VIEW_OPERATION into UNSUPPORTED_VIEW_OPERATION in error classes.
- Refactor some code.
- Fix some issue about error prompt.
### Why are the changes needed?
- The changes improve the error framework.
- Make code clear.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
- Pass GA.
- Manual test.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
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 #42824: [SPARK-45085][SQL] Merge UNSUPPORTED_TEMP_VIEW_OPERATION into UNSUPPORTED_VIEW_OPERATION and refactor some logic
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #42824:
URL: https://github.com/apache/spark/pull/42824#issuecomment-1717759119
late LGTM
--
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 #42824: [SPARK-45085][SQL] Merge UNSUPPORTED_TEMP_VIEW_OPERATION into UNSUPPORTED_VIEW_OPERATION and refactor some logic
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42824:
URL: https://github.com/apache/spark/pull/42824#discussion_r1324627920
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/v2ResolutionPlans.scala:
##########
@@ -54,7 +54,7 @@ case class UnresolvedView(
multipartIdentifier: Seq[String],
commandName: String,
allowTemp: Boolean,
- hint: Boolean) extends UnresolvedLeafNode
+ suggestAlternative: Boolean) extends UnresolvedLeafNode
Review Comment:
to be consistent with `UnresolvedTable`, shall we give it a default value (false) 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 a diff in pull request #42824: [SPARK-45085][SQL] Merge UNSUPPORTED_TEMP_VIEW_OPERATION into UNSUPPORTED_VIEW_OPERATION and refactor some logic
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42824:
URL: https://github.com/apache/spark/pull/42824#discussion_r1317411164
##########
sql/core/src/test/resources/sql-tests/analyzer-results/change-column.sql.out:
##########
@@ -206,10 +206,10 @@ ALTER TABLE temp_view CHANGE a TYPE INT
-- !query analysis
org.apache.spark.sql.AnalysisException
{
- "errorClass" : "UNSUPPORTED_TEMP_VIEW_OPERATION.WITH_SUGGESTION",
+ "errorClass" : "UNSUPPORTED_VIEW_OPERATION.WITHOUT_SUGGESTION",
Review Comment:
to double-check if we are symmetrical: what happens if we use ALTER VIEW to alter a table?
--
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] panbingkun commented on a diff in pull request #42824: [SPARK-45085][SQL] Merge UNSUPPORTED_TEMP_VIEW_OPERATION into UNSUPPORTED_VIEW_OPERATION and refactor some logic
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #42824:
URL: https://github.com/apache/spark/pull/42824#discussion_r1325165692
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/v2ResolutionPlans.scala:
##########
@@ -54,7 +54,7 @@ case class UnresolvedView(
multipartIdentifier: Seq[String],
commandName: String,
allowTemp: Boolean,
- hint: Boolean) extends UnresolvedLeafNode
+ suggestAlternative: Boolean) extends UnresolvedLeafNode
Review Comment:
The above modifications are submitted as following PR, and I will do it today.
--
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] panbingkun commented on a diff in pull request #42824: [SPARK-45085][SQL] Merge UNSUPPORTED_TEMP_VIEW_OPERATION into UNSUPPORTED_VIEW_OPERATION and refactor some logic
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #42824:
URL: https://github.com/apache/spark/pull/42824#discussion_r1319256211
##########
sql/core/src/test/resources/sql-tests/analyzer-results/change-column.sql.out:
##########
@@ -206,10 +206,10 @@ ALTER TABLE temp_view CHANGE a TYPE INT
-- !query analysis
org.apache.spark.sql.AnalysisException
{
- "errorClass" : "UNSUPPORTED_TEMP_VIEW_OPERATION.WITH_SUGGESTION",
+ "errorClass" : "UNSUPPORTED_VIEW_OPERATION.WITHOUT_SUGGESTION",
Review Comment:
Let me to check it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #42824: [SPARK-45085][SQL] Merge UNSUPPORTED_TEMP_VIEW_OPERATION into UNSUPPORTED_VIEW_OPERATION and refactor some logic
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42824:
URL: https://github.com/apache/spark/pull/42824#discussion_r1324616945
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/v2ResolutionPlans.scala:
##########
@@ -44,7 +44,7 @@ case class UnresolvedNamespace(multipartIdentifier: Seq[String]) extends Unresol
case class UnresolvedTable(
multipartIdentifier: Seq[String],
commandName: String,
- hint: Boolean = false) extends UnresolvedLeafNode
+ suggestAlternative: Boolean = false) extends UnresolvedLeafNode
Review Comment:
shall we also update the parameter name in `AstBuilder#createUnresolvedTable`?
--
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] panbingkun commented on a diff in pull request #42824: [SPARK-45085][SQL] Merge UNSUPPORTED_TEMP_VIEW_OPERATION into UNSUPPORTED_VIEW_OPERATION and refactor some logic
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #42824:
URL: https://github.com/apache/spark/pull/42824#discussion_r1319267739
##########
sql/core/src/test/resources/sql-tests/analyzer-results/change-column.sql.out:
##########
@@ -206,10 +206,10 @@ ALTER TABLE temp_view CHANGE a TYPE INT
-- !query analysis
org.apache.spark.sql.AnalysisException
{
- "errorClass" : "UNSUPPORTED_TEMP_VIEW_OPERATION.WITH_SUGGESTION",
+ "errorClass" : "UNSUPPORTED_VIEW_OPERATION.WITHOUT_SUGGESTION",
Review Comment:
<img width="663" alt="image" src="https://github.com/apache/spark/assets/15246973/5761e465-8161-4e5d-872b-cdf44e6cc0fc">
<img width="639" alt="image" src="https://github.com/apache/spark/assets/15246973/3ec76158-3aba-438b-a57e-7387c57a6e2c">
--
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] panbingkun commented on pull request #42824: [SPARK-45085][SQL] Merge UNSUPPORTED_TEMP_VIEW_OPERATION into UNSUPPORTED_VIEW_OPERATION and refactor some logic
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #42824:
URL: https://github.com/apache/spark/pull/42824#issuecomment-1707841772
Waiting: https://github.com/apache/spark/pull/42830
--
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] panbingkun commented on a diff in pull request #42824: [SPARK-45085][SQL] Merge UNSUPPORTED_TEMP_VIEW_OPERATION into UNSUPPORTED_VIEW_OPERATION and refactor some logic
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #42824:
URL: https://github.com/apache/spark/pull/42824#discussion_r1325249108
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/v2ResolutionPlans.scala:
##########
@@ -54,7 +54,7 @@ case class UnresolvedView(
multipartIdentifier: Seq[String],
commandName: String,
allowTemp: Boolean,
- hint: Boolean) extends UnresolvedLeafNode
+ suggestAlternative: Boolean) extends UnresolvedLeafNode
Review Comment:
A new follow up pr: https://github.com/apache/spark/pull/42917.
--
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] panbingkun commented on pull request #42824: [SPARK-45085][SQL] Merge UNSUPPORTED_TEMP_VIEW_OPERATION into UNSUPPORTED_VIEW_OPERATION and refactor some logic
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #42824:
URL: https://github.com/apache/spark/pull/42824#issuecomment-1708456765
cc @cloud-fan @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 #42824: [SPARK-45085][SQL] Merge UNSUPPORTED_TEMP_VIEW_OPERATION into UNSUPPORTED_VIEW_OPERATION and refactor some logic
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42824:
URL: https://github.com/apache/spark/pull/42824#discussion_r1324614861
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -1124,34 +1124,33 @@ class Analyzer(override val catalogManager: CatalogManager) extends RuleExecutor
if timestamp.forall(ts => ts.resolved && !SubqueryExpression.hasSubquery(ts)) =>
resolveRelation(u, TimeTravelSpec.create(timestamp, version, conf)).getOrElse(r)
- case u @ UnresolvedTable(identifier, cmd, relationTypeMismatchHint) =>
+ case u @ UnresolvedTable(identifier, cmd, suggestAlternative) =>
lookupTableOrView(identifier).map {
case v: ResolvedPersistentView =>
val nameParts = v.catalog.name() +: v.identifier.asMultipartIdentifier
- throw QueryCompilationErrors.expectTableNotViewError(
- nameParts, isTemp = false, cmd, true, u)
+ throw QueryCompilationErrors.unsupportedViewOperationError(
+ nameParts, cmd, suggestAlternative, u)
case _: ResolvedTempView =>
- throw QueryCompilationErrors.expectTableNotViewError(
- identifier, isTemp = true, cmd, true, u)
+ throw QueryCompilationErrors.unsupportedViewOperationError(
+ identifier, cmd, suggestAlternative, u)
case table => table
}.getOrElse(u)
- case u @ UnresolvedView(identifier, cmd, allowTemp, hint) =>
+ case u @ UnresolvedView(identifier, cmd, allowTemp, suggestAlternative) =>
lookupTableOrView(identifier, viewOnly = true).map {
case _: ResolvedTempView if !allowTemp =>
- throw QueryCompilationErrors.expectViewNotTempViewError(identifier, cmd, u)
+ throw QueryCompilationErrors.unsupportedViewOperationError(identifier, cmd, false, u)
case t: ResolvedTable =>
val nameParts = t.catalog.name() +: t.identifier.asMultipartIdentifier
throw QueryCompilationErrors.expectViewNotTableError(
Review Comment:
nit: for consistency, shall we name this method `unsupportedTableOperationError`?
--
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 pull request #42824: [SPARK-45085][SQL] Merge UNSUPPORTED_TEMP_VIEW_OPERATION into UNSUPPORTED_VIEW_OPERATION and refactor some logic
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #42824:
URL: https://github.com/apache/spark/pull/42824#issuecomment-1715162527
+1, LGTM. Merging to master.
Thank you, @panbingkun.
--
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] panbingkun commented on a diff in pull request #42824: [SPARK-45085][SQL] Merge UNSUPPORTED_TEMP_VIEW_OPERATION into UNSUPPORTED_VIEW_OPERATION and refactor some logic
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #42824:
URL: https://github.com/apache/spark/pull/42824#discussion_r1319282063
##########
sql/core/src/test/resources/sql-tests/analyzer-results/change-column.sql.out:
##########
@@ -206,10 +206,10 @@ ALTER TABLE temp_view CHANGE a TYPE INT
-- !query analysis
org.apache.spark.sql.AnalysisException
{
- "errorClass" : "UNSUPPORTED_TEMP_VIEW_OPERATION.WITH_SUGGESTION",
+ "errorClass" : "UNSUPPORTED_VIEW_OPERATION.WITHOUT_SUGGESTION",
Review Comment:
From the test results, it is reasonable and symmetrical.
--
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 pull request #42824: [SPARK-45085][SQL] Merge UNSUPPORTED_TEMP_VIEW_OPERATION into UNSUPPORTED_VIEW_OPERATION and refactor some logic
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #42824:
URL: https://github.com/apache/spark/pull/42824#issuecomment-1713485296
@cloud-fan Are you ok with the changes?
--
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 closed pull request #42824: [SPARK-45085][SQL] Merge UNSUPPORTED_TEMP_VIEW_OPERATION into UNSUPPORTED_VIEW_OPERATION and refactor some logic
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #42824: [SPARK-45085][SQL] Merge UNSUPPORTED_TEMP_VIEW_OPERATION into UNSUPPORTED_VIEW_OPERATION and refactor some logic
URL: https://github.com/apache/spark/pull/42824
--
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