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/12/09 08:17:29 UTC

[GitHub] [spark] bozhang2820 opened a new pull request, #38998: [SPARK-41463][SQL][TESTS] Ensure error class names contain only capital letters, numbers and underscores

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

   ### What changes were proposed in this pull request?
   This change adds a unit test to verify that error class and subclass names contain only capital letters, numbers and underscores.
   
   ### Why are the changes needed?
   This is to ensure that the standard holds for error classes added in the future.
   
   ### Does this PR introduce _any_ user-facing change?
   No
   
   ### How was this patch tested?
   Test only change. Also tried adding an invalid error class name and verified that the test failed locally.
   


-- 
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] bozhang2820 commented on a diff in pull request #38998: [SPARK-41463][SQL][TESTS] Ensure error class names contain only capital letters, numbers and underscores

Posted by GitBox <gi...@apache.org>.
bozhang2820 commented on code in PR #38998:
URL: https://github.com/apache/spark/pull/38998#discussion_r1045389644


##########
core/src/test/scala/org/apache/spark/SparkThrowableSuite.scala:
##########
@@ -147,6 +147,18 @@ class SparkThrowableSuite extends SparkFunSuite {
     assert(rereadErrorClassToInfoMap == errorReader.errorInfoMap)
   }
 
+  test("Error class names should contain only capital letters, numbers and underscores") {
+    val allowedChars = "[A-Z0-9_]*"
+    errorReader.errorInfoMap.foreach { e =>
+      assert(e._1.matches(allowedChars))

Review Comment:
   Thanks for the suggestion. 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] MaxGekk commented on pull request #38998: [SPARK-41463][SQL][TESTS] Ensure error class names contain only capital letters, numbers and underscores

Posted by GitBox <gi...@apache.org>.
MaxGekk commented on PR #38998:
URL: https://github.com/apache/spark/pull/38998#issuecomment-1345970916

   +1, LGTM. Merging to master.
   Thank you, @bozhang2820 and @srielau for review.


-- 
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] bozhang2820 commented on pull request #38998: [SPARK-41463][SQL][TESTS] Ensure error class names contain only capital letters, numbers and underscores

Posted by GitBox <gi...@apache.org>.
bozhang2820 commented on PR #38998:
URL: https://github.com/apache/spark/pull/38998#issuecomment-1344364945

   @srielau, @cloud-fan, could you 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] MaxGekk commented on a diff in pull request #38998: [SPARK-41463][SQL][TESTS] Ensure error class names contain only capital letters, numbers and underscores

Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #38998:
URL: https://github.com/apache/spark/pull/38998#discussion_r1045013373


##########
core/src/test/scala/org/apache/spark/SparkThrowableSuite.scala:
##########
@@ -147,6 +147,18 @@ class SparkThrowableSuite extends SparkFunSuite {
     assert(rereadErrorClassToInfoMap == errorReader.errorInfoMap)
   }
 
+  test("Error class names should contain only capital letters, numbers and underscores") {
+    val allowedChars = "[A-Z0-9_]*"
+    errorReader.errorInfoMap.foreach { e =>
+      assert(e._1.matches(allowedChars))

Review Comment:
   This one doesn't show which error class among 1500 is invalid, see:
   ```
   e._1.matches(allowedChars) was false
   ScalaTestFailureLocation: org.apache.spark.SparkThrowableSuite at (SparkThrowableSuite.scala:153)
   org.scalatest.exceptions.TestFailedException: e._1.matches(allowedChars) was false
   ```
   Could you output the error class name:
   ```suggestion
         assert(e._1.matches(allowedChars), s"The error class is invalid: ${e._1}")
   ```



##########
core/src/test/scala/org/apache/spark/SparkThrowableSuite.scala:
##########
@@ -147,6 +147,18 @@ class SparkThrowableSuite extends SparkFunSuite {
     assert(rereadErrorClassToInfoMap == errorReader.errorInfoMap)
   }
 
+  test("Error class names should contain only capital letters, numbers and underscores") {
+    val allowedChars = "[A-Z0-9_]*"
+    errorReader.errorInfoMap.foreach { e =>
+      assert(e._1.matches(allowedChars))
+      e._2.subClass.map { s =>
+        s.keys.foreach { k =>
+          assert(k.matches(allowedChars))

Review Comment:
   ```suggestion
             assert(k.matches(allowedChars), s"The error sub-class is invalid: $k")
   ```



-- 
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 #38998: [SPARK-41463][SQL][TESTS] Ensure error class names contain only capital letters, numbers and underscores

Posted by GitBox <gi...@apache.org>.
MaxGekk closed pull request #38998: [SPARK-41463][SQL][TESTS] Ensure error class names contain only capital letters, numbers and underscores
URL: https://github.com/apache/spark/pull/38998


-- 
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