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/14 01:45:55 UTC

[GitHub] [spark] panbingkun opened a new pull request, #42917: [SPARK-45085][SQL][FOLLOWUP] Merge UNSUPPORTED_TEMP_VIEW_OPERATION into UNSUPPORTED_VIEW_OPERATION and refactor some logic

panbingkun opened a new pull request, #42917:
URL: https://github.com/apache/spark/pull/42917

   ### What changes were proposed in this pull request?
   The pr aims to fix some naming issues based on the suggestions of [PR](https://github.com/apache/spark/pull/42824) reviewers.
   The pr is follow up https://github.com/apache/spark/pull/42824.
   
   ### Why are the changes needed?
   Better code readability.
   
   ### Does this PR introduce _any_ user-facing change?
   No.
   
   
   ### How was this patch tested?
   Pass GA.
   
   ### 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] panbingkun commented on pull request #42917: [SPARK-45085][SQL][FOLLOWUP] 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 #42917:
URL: https://github.com/apache/spark/pull/42917#issuecomment-1718621689

   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 #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1328914900


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -1142,7 +1142,7 @@ class Analyzer(override val catalogManager: CatalogManager) extends RuleExecutor
             throw QueryCompilationErrors.unsupportedViewOperationError(identifier, cmd, false, u)
           case t: ResolvedTable =>
             val nameParts = t.catalog.name() +: t.identifier.asMultipartIdentifier
-            throw QueryCompilationErrors.expectViewNotTableError(
+            throw QueryCompilationErrors.unsupportedTableOperationError(

Review Comment:
   The name should still be `expectViewNotTableError`



-- 
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 #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1328910944


##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -860,6 +860,35 @@
       "Exceeds char/varchar type length limitation: <limit>."
     ]
   },
+  "EXPECT_TABLE_NOT_VIEW" : {
+    "message" : [
+      "The view <viewName> does not support <operation>."

Review Comment:
   Can we revert to the old message? https://github.com/apache/spark/commit/295c615b16b8a77f242ffa99006b4fb95f8f3487#diff-74b78a86e87e47c520c1183be7ec8b5220378f123e47daad7f9e4cdd1a66336cL3727



-- 
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 #42917: [SPARK-45085][SQL][FOLLOWUP] 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 #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1325324132


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -498,13 +498,13 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase with Compilat
       origin = t.origin)
   }
 
-  def expectViewNotTableError(
+  def unsupportedTableOperationError(

Review Comment:
   I think a good reason to move a sub error class to top-level error class is itself needs sub error classes.



-- 
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 #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1329515855


##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -860,6 +860,50 @@
       "Exceeds char/varchar type length limitation: <limit>."
     ]
   },
+  "EXPECT_PERMANENT_VIEW_NOT_TEMP" : {
+    "message" : [
+      "'<operation>' expects a permanent view but <viewName> is a temp view."
+    ]
+  },
+  "EXPECT_TABLE_NOT_VIEW" : {
+    "message" : [
+      "'<operation>' expects a table but <viewName> is a view."
+    ],
+    "subClass" : {
+      "NO_ALTERNATIVE" : {
+        "message" : [
+          ""
+        ]
+      },
+      "USE_ALTER_VIEW" : {
+        "message" : [
+          "Please use ALTER VIEW instead."
+        ]
+      }
+    }
+  },
+  "EXPECT_TABLE_OR_PERMANENT_VIEW_NOT_TEMP" : {

Review Comment:
   ```suggestion
     "EXPECT_PERMANENT_TABLE_OR_VIEW_NOT_TEMP_VIEW" : {
   ```



##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -860,6 +860,50 @@
       "Exceeds char/varchar type length limitation: <limit>."
     ]
   },
+  "EXPECT_PERMANENT_VIEW_NOT_TEMP" : {
+    "message" : [
+      "'<operation>' expects a permanent view but <viewName> is a temp view."
+    ]
+  },
+  "EXPECT_TABLE_NOT_VIEW" : {
+    "message" : [
+      "'<operation>' expects a table but <viewName> is a view."
+    ],
+    "subClass" : {
+      "NO_ALTERNATIVE" : {
+        "message" : [
+          ""
+        ]
+      },
+      "USE_ALTER_VIEW" : {
+        "message" : [
+          "Please use ALTER VIEW instead."
+        ]
+      }
+    }
+  },
+  "EXPECT_TABLE_OR_PERMANENT_VIEW_NOT_TEMP" : {

Review Comment:
   in case we add temp table in the future.



-- 
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 #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1329599785


##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -860,6 +860,50 @@
       "Exceeds char/varchar type length limitation: <limit>."
     ]
   },
+  "EXPECT_PERMANENT_VIEW_NOT_TEMP" : {
+    "message" : [
+      "'<operation>' expects a permanent view but <viewName> is a temp view."
+    ]
+  },
+  "EXPECT_TABLE_NOT_VIEW" : {
+    "message" : [
+      "'<operation>' expects a table but <viewName> is a view."
+    ],
+    "subClass" : {
+      "NO_ALTERNATIVE" : {
+        "message" : [
+          ""
+        ]
+      },
+      "USE_ALTER_VIEW" : {
+        "message" : [
+          "Please use ALTER VIEW instead."
+        ]
+      }
+    }
+  },
+  "EXPECT_TABLE_OR_PERMANENT_VIEW_NOT_TEMP" : {

Review Comment:
   That seems quite reasonable. Let me give it a try, and at the same time, let me to double check the rationality of similar cases in 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] cloud-fan commented on a diff in pull request #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1328917452


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -1142,7 +1142,7 @@ class Analyzer(override val catalogManager: CatalogManager) extends RuleExecutor
             throw QueryCompilationErrors.unsupportedViewOperationError(identifier, cmd, false, u)

Review Comment:
   We probably need a new error class for it: `EXPECT_PERMANENT_VIEW_NOT_TEMP`



-- 
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 #42917: [SPARK-45163][SQL] Merge TABLE_OPERATION & _LEGACY_ERROR_TEMP_1113 into UNSUPPORTED_TABLE_OPERATION and refactor some logic

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1325993468


##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -3215,11 +3225,6 @@
           "<variableName> is a VARIABLE and cannot be updated using the SET statement. Use SET VARIABLE <variableName> = ... instead."
         ]
       },
-      "TABLE_OPERATION" : {
-        "message" : [
-          "Table <tableName> does not support <operation>. Please check the current catalog and namespace to make sure the qualified table name is expected, and also check the catalog implementation which is configured by \"spark.sql.catalog\"."

Review Comment:
   Let's not degrade the error message. I think there are two kinds if unsupported table operations: 1) the table implementation does not support certain DS v2 features. 2) it's a view not table.
   
   Now my preference is to keep this sub error class here, but add two new error class `EXPECT_VIEW_GOT_TABLE` and `EXPECT_TABLE_GOT_VIEW` to replace the `UNSUPPORTED_TABLE_OPERATION` and `UNSUPPORTED_VIEW_OPERATION`



-- 
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 #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1327119760


##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -3215,11 +3225,6 @@
           "<variableName> is a VARIABLE and cannot be updated using the SET statement. Use SET VARIABLE <variableName> = ... instead."
         ]
       },
-      "TABLE_OPERATION" : {
-        "message" : [
-          "Table <tableName> does not support <operation>. Please check the current catalog and namespace to make sure the qualified table name is expected, and also check the catalog implementation which is configured by \"spark.sql.catalog\"."

Review Comment:
   Done.
   PS: After the pr, I will open a new seperate pr to merge `_LEGACY_ERROR_TEMP_1113` into `UNSUPPORTED_FEATURE.TABLE_OPERATION`.



-- 
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 #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1327124355


##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala:
##########
@@ -976,7 +976,7 @@ class HiveDDLSuite
           exception = intercept[AnalysisException] {
             sql(s"ALTER TABLE $oldViewName RECOVER PARTITIONS")
           },
-          errorClass = "UNSUPPORTED_VIEW_OPERATION.WITH_SUGGESTION",

Review Comment:
   The old logic will prompt `Please use ALTER VIEW instead.` here, 
   Because the syntax of `ALTER VIEW ... RECOVER PARTITIONS` is not supported on view, prompts can actually cause misunderstandings.
   which is incorrect. 
   I have checked the rationality of the error prompts returned by all 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 pull request #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #42917:
URL: https://github.com/apache/spark/pull/42917#issuecomment-1730624466

   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] cloud-fan closed pull request #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue
URL: 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 #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #42917:
URL: https://github.com/apache/spark/pull/42917#issuecomment-1730670776

   Thank you very much!
   After the pr, I will open a new pr to merge _LEGACY_ERROR_TEMP_1113 into UNSUPPORTED_FEATURE.TABLE_OPERATION.
   https://github.com/apache/spark/blob/39d6fdabb48c700ab5d9fe33437341f5dbf3d1d7/common/utils/src/main/resources/error/error-classes.json#L4100-L4104


-- 
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 #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #42917:
URL: https://github.com/apache/spark/pull/42917#issuecomment-1721070239

   I have checked all UT to display prompts for what should be prompted and not for what should not be prompted.


-- 
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 #42917: [SPARK-45085][SQL][FOLLOWUP] 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 #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1325314370


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -498,13 +498,13 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase with Compilat
       origin = t.origin)
   }
 
-  def expectViewNotTableError(
+  def unsupportedTableOperationError(

Review Comment:
   Well, this is the legacy issue, 
   The error class used by the another ·unsupportedTableOperationError` method is `UNSUPPORTED_FEATURE.TABLE_OPERATION`. 
   
   The biggest difference between it and the current `UNSUPPORTED_TABLE_OPERATION.WITH_SUGGESTION` is that the prompt is different. 
   
   - The `UNSUPPORTED_FEATURE.TABLE_OPERATION` is
   https://github.com/apache/spark/blob/b6190a3db974c19b6a0c4fe7af75531d67755074/common/utils/src/main/resources/error/error-classes.json#L3218-L3222
   
   - The `UNSUPPORTED_TABLE_OPERATION.WITH_SUGGESTION` is:
   https://github.com/apache/spark/blob/b6190a3db974c19b6a0c4fe7af75531d67755074/common/utils/src/main/resources/error/error-classes.json#L3440-L3444
   
   ### Let me try to merge them together in this PR.



##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -498,13 +498,13 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase with Compilat
       origin = t.origin)
   }
 
-  def expectViewNotTableError(
+  def unsupportedTableOperationError(

Review Comment:
   Well, this is the legacy issue, 
   The error class used by the another ·unsupportedTableOperationError` method is `UNSUPPORTED_FEATURE.TABLE_OPERATION`. 
   
   The biggest difference between it and the current `UNSUPPORTED_TABLE_OPERATION.WITH_SUGGESTION` is that the prompt is different. 
   
   - The `UNSUPPORTED_FEATURE.TABLE_OPERATION` is
   https://github.com/apache/spark/blob/b6190a3db974c19b6a0c4fe7af75531d67755074/common/utils/src/main/resources/error/error-classes.json#L3218-L3222
   
   - The `UNSUPPORTED_TABLE_OPERATION.WITH_SUGGESTION` is:
   https://github.com/apache/spark/blob/b6190a3db974c19b6a0c4fe7af75531d67755074/common/utils/src/main/resources/error/error-classes.json#L3440-L3444
   
   ### Let me try to merge them together in this PR.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] panbingkun commented on a diff in pull request #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1329502973


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -1142,7 +1142,7 @@ class Analyzer(override val catalogManager: CatalogManager) extends RuleExecutor
             throw QueryCompilationErrors.unsupportedViewOperationError(identifier, cmd, false, u)

Review Comment:
   I also added this new error class: `EXPECT_TABLE_OR_PERMANENT_VIEW_NOT_TEMP`.



-- 
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 #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1328263710


##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -860,6 +860,16 @@
       "Exceeds char/varchar type length limitation: <limit>."
     ]
   },
+  "EXPECT_TABLE_NOT_VIEW" : {
+    "message" : [
+      "The view <viewName> does not support <operation>, Please use ALTER VIEW instead."

Review Comment:
   We can add more sub error clasess when some commands start to have alternative commands for views.



-- 
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 #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #42917:
URL: https://github.com/apache/spark/pull/42917#issuecomment-1724869552

   > looks much better now, thanks for your patience!
   
   Thank you very much for patiently reviewing the 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.

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 #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #42917:
URL: https://github.com/apache/spark/pull/42917#issuecomment-1729110480

   Gentle ping @cloud-fan 


-- 
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 #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1328917452


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -1142,7 +1142,7 @@ class Analyzer(override val catalogManager: CatalogManager) extends RuleExecutor
             throw QueryCompilationErrors.unsupportedViewOperationError(identifier, cmd, false, u)

Review Comment:
   We probably need a new error class for it: `EXPECT_PERSISTENT_VIEW_NOT_TEMP`



-- 
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 #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1329502401


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -1142,7 +1142,7 @@ class Analyzer(override val catalogManager: CatalogManager) extends RuleExecutor
             throw QueryCompilationErrors.unsupportedViewOperationError(identifier, cmd, false, u)

Review Comment:
   Done



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] panbingkun commented on a diff in pull request #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1329502401


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -1142,7 +1142,7 @@ class Analyzer(override val catalogManager: CatalogManager) extends RuleExecutor
             throw QueryCompilationErrors.unsupportedViewOperationError(identifier, cmd, false, u)

Review Comment:
   Done



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] panbingkun commented on a diff in pull request #42917: [SPARK-45163][SQL] Merge TABLE_OPERATION & _LEGACY_ERROR_TEMP_1113 into UNSUPPORTED_TABLE_OPERATION and refactor some logic

Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1326684936


##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -3215,11 +3225,6 @@
           "<variableName> is a VARIABLE and cannot be updated using the SET statement. Use SET VARIABLE <variableName> = ... instead."
         ]
       },
-      "TABLE_OPERATION" : {
-        "message" : [
-          "Table <tableName> does not support <operation>. Please check the current catalog and namespace to make sure the qualified table name is expected, and also check the catalog implementation which is configured by \"spark.sql.catalog\"."

Review Comment:
   Let me refactor 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] panbingkun commented on a diff in pull request #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1327125546


##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala:
##########
@@ -976,7 +976,7 @@ class HiveDDLSuite
           exception = intercept[AnalysisException] {
             sql(s"ALTER TABLE $oldViewName RECOVER PARTITIONS")
           },
-          errorClass = "UNSUPPORTED_VIEW_OPERATION.WITH_SUGGESTION",

Review Comment:
   Other modifications are similar.



-- 
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 #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1328263710


##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -860,6 +860,16 @@
       "Exceeds char/varchar type length limitation: <limit>."
     ]
   },
+  "EXPECT_TABLE_NOT_VIEW" : {
+    "message" : [
+      "The view <viewName> does not support <operation>, Please use ALTER VIEW instead."

Review Comment:
   We can add more sub error clasess when some commands start to have alternative commands.



-- 
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 #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1328263476


##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -860,6 +860,16 @@
       "Exceeds char/varchar type length limitation: <limit>."
     ]
   },
+  "EXPECT_TABLE_NOT_VIEW" : {
+    "message" : [
+      "The view <viewName> does not support <operation>, Please use ALTER VIEW instead."

Review Comment:
   We can add sub error classes
   ```
   NO_ALTERNATIVE: ...
   USE_ALTER_VIEW: ...
   ``` 



-- 
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 #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1328262847


##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -860,6 +860,16 @@
       "Exceeds char/varchar type length limitation: <limit>."
     ]
   },
+  "EXPECT_TABLE_NOT_VIEW" : {
+    "message" : [
+      "The view <viewName> does not support <operation>, Please use ALTER VIEW instead."

Review Comment:
   I think `Please use ALTER VIEW instead` only applies to certain commands?



-- 
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 #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1328910944


##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -860,6 +860,35 @@
       "Exceeds char/varchar type length limitation: <limit>."
     ]
   },
+  "EXPECT_TABLE_NOT_VIEW" : {
+    "message" : [
+      "The view <viewName> does not support <operation>."

Review Comment:
   Can we revert to the old message? https://github.com/apache/spark/commit/295c615b16b8a77f242ffa99006b4fb95f8f3487#diff-74b78a86e87e47c520c1183be7ec8b5220378f123e47daad7f9e4cdd1a66336cL3727
   
   I think it's better to say something like: `xxx_command expects 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 #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1329665321


##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -860,6 +860,50 @@
       "Exceeds char/varchar type length limitation: <limit>."
     ]
   },
+  "EXPECT_PERMANENT_VIEW_NOT_TEMP" : {
+    "message" : [
+      "'<operation>' expects a permanent view but <viewName> is a temp view."
+    ]
+  },
+  "EXPECT_TABLE_NOT_VIEW" : {
+    "message" : [
+      "'<operation>' expects a table but <viewName> is a view."
+    ],
+    "subClass" : {
+      "NO_ALTERNATIVE" : {
+        "message" : [
+          ""
+        ]
+      },
+      "USE_ALTER_VIEW" : {
+        "message" : [
+          "Please use ALTER VIEW instead."
+        ]
+      }
+    }
+  },
+  "EXPECT_TABLE_OR_PERMANENT_VIEW_NOT_TEMP" : {

Review Comment:
   I checked two cases in UT，as follows:
   - "SHOW CREATE TABLE<viewName> ..."
   - "ANALYZE TABLE<viewName> ..."
   
   throw ` EXPECT_ PERMANENT_ VIEW_ NOT_ TEMP`, It's reasonable.



##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -860,6 +860,50 @@
       "Exceeds char/varchar type length limitation: <limit>."
     ]
   },
+  "EXPECT_PERMANENT_VIEW_NOT_TEMP" : {
+    "message" : [
+      "'<operation>' expects a permanent view but <viewName> is a temp view."
+    ]
+  },
+  "EXPECT_TABLE_NOT_VIEW" : {
+    "message" : [
+      "'<operation>' expects a table but <viewName> is a view."
+    ],
+    "subClass" : {
+      "NO_ALTERNATIVE" : {
+        "message" : [
+          ""
+        ]
+      },
+      "USE_ALTER_VIEW" : {
+        "message" : [
+          "Please use ALTER VIEW instead."
+        ]
+      }
+    }
+  },
+  "EXPECT_TABLE_OR_PERMANENT_VIEW_NOT_TEMP" : {

Review Comment:
   I checked two cases in UT，as follows:
   - "SHOW CREATE TABLE<viewName> ..."
   - "ANALYZE TABLE<viewName> ..."
   
   throw ` EXPECT_ PERMANENT_ VIEW_ NOT_ TEMP`, It's reasonable.



-- 
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 #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1328913719


##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -860,6 +860,35 @@
       "Exceeds char/varchar type length limitation: <limit>."
     ]
   },
+  "EXPECT_TABLE_NOT_VIEW" : {
+    "message" : [
+      "The view <viewName> does not support <operation>."

Review Comment:
   `'<cmd>' expects a table but <viewName> is a view.`



-- 
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 #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1329519381


##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -860,6 +860,50 @@
       "Exceeds char/varchar type length limitation: <limit>."
     ]
   },
+  "EXPECT_PERMANENT_VIEW_NOT_TEMP" : {
+    "message" : [
+      "'<operation>' expects a permanent view but <viewName> is a temp view."
+    ]
+  },
+  "EXPECT_TABLE_NOT_VIEW" : {
+    "message" : [
+      "'<operation>' expects a table but <viewName> is a view."
+    ],
+    "subClass" : {
+      "NO_ALTERNATIVE" : {
+        "message" : [
+          ""
+        ]
+      },
+      "USE_ALTER_VIEW" : {
+        "message" : [
+          "Please use ALTER VIEW instead."
+        ]
+      }
+    }
+  },
+  "EXPECT_TABLE_OR_PERMANENT_VIEW_NOT_TEMP" : {

Review Comment:
   Actually, seems we can reuse `EXPECT_PERMANENT_VIEW_NOT_TEMP`? If we get a temp view, it's probably good enough to notify users that the operation needs permanent view not temp, no need to mention that the operation accepts table 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 #42917: [SPARK-45085][SQL][FOLLOWUP] 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 #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1325296267


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -498,13 +498,13 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase with Compilat
       origin = t.origin)
   }
 
-  def expectViewNotTableError(
+  def unsupportedTableOperationError(

Review Comment:
   There is already a `unsupportedTableOperationError` in this file, shall we merge these two?



-- 
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 #42917: [SPARK-45163][SQL] Merge TABLE_OPERATION & _LEGACY_ERROR_TEMP_1113 into UNSUPPORTED_TABLE_OPERATION and refactor some logic

Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1325764549


##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala:
##########
@@ -1486,11 +1486,8 @@ class DataSourceV2SQLSuiteV1Filter
             sql("USE dummy")
             sql(s"$statement dummy.$tableDefinition")
           },
-          errorClass = "UNSUPPORTED_FEATURE.TABLE_OPERATION",
-          parameters = Map(
-            "tableName" -> "`dummy`.`my_tab`",
-            "operation" -> "column default value"
-          )
+          errorClass = "UNSUPPORTED_FEATURE.CREATE_OR_REPLACE_TABLE_WITH_DEFAULT_VALUE",

Review Comment:
   The name here seems to better convey the meaning of this error.



-- 
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 #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1328267365


##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -3239,6 +3249,11 @@
         "message" : [
           "TRANSFORM with SERDE is only supported in hive mode."
         ]
+      },
+      "VIEW_OPERATION" : {

Review Comment:
   I'm not sure we need this error class. DS v2 does not support view yet.



-- 
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 #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1327126016


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -4715,8 +4714,7 @@ class AstBuilder extends DataTypeAstBuilder with SQLConfHelper with Logging {
     RecoverPartitions(
       createUnresolvedTable(
         ctx.identifierReference,
-        "ALTER TABLE ... RECOVER PARTITIONS",
-        true))
+        "ALTER TABLE ... RECOVER PARTITIONS"))
   }

Review Comment:
   Because the syntax of ALTER VIEW ... RECOVER PARTITIONS is not supported on view, prompts can actually cause misunderstandings.



-- 
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 #42917: [SPARK-45163][SQL] Merge TABLE_OPERATION & _LEGACY_ERROR_TEMP_1113 into UNSUPPORTED_TABLE_OPERATION and refactor some logic

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1325993468


##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -3215,11 +3225,6 @@
           "<variableName> is a VARIABLE and cannot be updated using the SET statement. Use SET VARIABLE <variableName> = ... instead."
         ]
       },
-      "TABLE_OPERATION" : {
-        "message" : [
-          "Table <tableName> does not support <operation>. Please check the current catalog and namespace to make sure the qualified table name is expected, and also check the catalog implementation which is configured by \"spark.sql.catalog\"."

Review Comment:
   Let's not degrade the error message. I think there are two kinds if unsupported table operations: 1) the table implementation does not support certain DS v2 features. 2) it's a view not table.
   
   Now my preference is to keep this sub error class here, but add two new error class `EXPECT_VIEW_NOT_TABLE` and `EXPECT_TABLE_NOT_VIEW` to replace the `UNSUPPORTED_TABLE_OPERATION` and `UNSUPPORTED_VIEW_OPERATION`



-- 
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 #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1329425447


##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -860,6 +860,35 @@
       "Exceeds char/varchar type length limitation: <limit>."
     ]
   },
+  "EXPECT_TABLE_NOT_VIEW" : {
+    "message" : [
+      "The view <viewName> does not support <operation>."

Review Comment:
   Okay



-- 
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 #42917: [SPARK-45085][SQL][FOLLOWUP] 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 #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1325298998


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -498,13 +498,13 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase with Compilat
       origin = t.origin)
   }
 
-  def expectViewNotTableError(
+  def unsupportedTableOperationError(

Review Comment:
   Let me take a look.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] panbingkun commented on a diff in pull request #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1328399836


##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -3239,6 +3249,11 @@
         "message" : [
           "TRANSFORM with SERDE is only supported in hive mode."
         ]
+      },
+      "VIEW_OPERATION" : {

Review Comment:
   I will replace `UNSUPPORTED_FEATURE.VIEW_OPERATION ` with `EXPECT_TABLE_NOT_VIEW. NO_ALTERNATIVE`



-- 
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 #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1328919556


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -4715,8 +4714,7 @@ class AstBuilder extends DataTypeAstBuilder with SQLConfHelper with Logging {
     RecoverPartitions(
       createUnresolvedTable(
         ctx.identifierReference,
-        "ALTER TABLE ... RECOVER PARTITIONS",
-        true))
+        "ALTER TABLE ... RECOVER PARTITIONS"))
   }

Review Comment:
   good catch!



-- 
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 #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1328929659


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -2854,10 +2854,11 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase with Compilat
         "dataColumns" -> query.output.map(c => toSQLId(c.name)).mkString(", ")))
   }
 
-  def tableIsNotViewError(name: TableIdentifier, replace: Boolean): Throwable = {
+  def unsupportedCreateOrReplaceViewOnTableError(
+      name: TableIdentifier, replace: Boolean): Throwable = {
     val operation = if (replace) "CREATE OR REPLACE VIEW" else "CREATE VIEW"

Review Comment:
   If `replace = false`, it's better to throw `TABLE_OR_VIEW_ALREADY_EXISTS`. Otherwise, `EXPETCT_VIEW_NOT_TABLE` should be good.



-- 
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 #42917: [SPARK-45163][SQL] Merge UNSUPPORTED_VIEW_OPERATION & UNSUPPORTED_TABLE_OPERATION & fix some issue

Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #42917:
URL: https://github.com/apache/spark/pull/42917#discussion_r1328273864


##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -860,6 +860,16 @@
       "Exceeds char/varchar type length limitation: <limit>."
     ]
   },
+  "EXPECT_TABLE_NOT_VIEW" : {
+    "message" : [
+      "The view <viewName> does not support <operation>, Please use ALTER VIEW instead."

Review Comment:
   Okay, I'll do 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