[GitHub] [spark] SandishKumarHN commented on a diff in pull request #38384: [SPARK-40657][PROTOBUF] Require shading for Java class jar, improve error handling

[GitBox <gi...@apache.org>]

SandishKumarHN commented on code in PR #38384:
URL: https://github.com/apache/spark/pull/38384#discussion_r1021864869


##########
connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/ProtobufUtils.scala:
##########
@@ -155,21 +155,52 @@ private[sql] object ProtobufUtils extends Logging {
    *  Loads the given protobuf class and returns Protobuf descriptor for it.
    */
   def buildDescriptorFromJavaClass(protobufClassName: String): Descriptor = {
+
+    // Default 'Message' class here is shaded while using the package (as in production).
+    // The incoming classes might not be shaded. Check both.
+    val shadedMessageClass = classOf[Message] // Shaded in prod, not in unit tests.
+    val missingShadingErrorMessage = "The jar with Protobuf classes needs to be shaded " +
+      s"(com.google.protobuf.* --> ${shadedMessageClass.getPackage.getName}.*)"
+
     val protobufClass = try {
       Utils.classForName(protobufClassName)
     } catch {
       case e: ClassNotFoundException =>
-        throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, e)
+        val explanation =
+          if (protobufClassName.contains(".")) "Ensure the class include in the jar"
+          else "Ensure the class name includes package prefix"
+        throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, explanation, e)
+
+      case e: NoClassDefFoundError if e.getMessage.matches("com/google/proto.*Generated.*") =>
+        // This indicates the the the Java classes are not shaded.
+        throw QueryCompilationErrors.protobufClassLoadError(
+          protobufClassName, missingShadingErrorMessage, e)
     }
 
-    if (!classOf[Message].isAssignableFrom(protobufClass)) {
-      throw QueryCompilationErrors.protobufMessageTypeError(protobufClassName)
-      // TODO: Need to support V2. This might work with V2 classes too.
+    if (!shadedMessageClass.isAssignableFrom(protobufClass)) {
+      // Check if this extends 2.x Message class included in spark, that does not work.
+      val unshadedMessageClass = Utils.classForName(
+        "com.escape-shading.google.protobuf.Message".replace("escape-shading.", "")
+      )
+      val explanation =
+        if (unshadedMessageClass.isAssignableFrom(protobufClass)) {
+          s"$protobufClassName does not extend shaded Protobuf Message class " +
+          s"${shadedMessageClass.getName}. $missingShadingErrorMessage"
+        } else s"$protobufClassName is not a Protobuf Message type"
+      throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, explanation)
     }
 
     // Extract the descriptor from Protobuf message.
-    protobufClass
-      .getDeclaredMethod("getDescriptor")
+    val getDescriptorMethod = try {
+      protobufClass
+        .getDeclaredMethod("getDescriptor")
+    } catch {
+      case _: NoSuchMethodError => // This is usually not expected.
+        throw new IllegalArgumentException(

Review Comment:
   can we move this to a new error class framework? 



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