[SPARK-40777][SQL][PROTOBUF] Protobuf import support and move error-classes. #38344

SandishKumarHN · 2022-10-22T03:08:48Z

This is the follow-up PR to #37972 and #38212

What changes were proposed in this pull request?

Move spark-protobuf error classes to the spark error-classes framework(core/src/main/resources/error/error-classes.json).
Support protobuf imports
validate protobuf timestamp and duration types.

Why are the changes needed?

N/A

Does this PR introduce any user-facing change?

None

How was this patch tested?

Existing tests should cover the validation of this PR.

CC: @rangadi @mposdev21 @gengliangwang

…mport support

# Conflicts: # connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/ProtobufUtils.scala # connector/protobuf/src/test/scala/org/apache/spark/sql/protobuf/ProtobufFunctionsSuite.scala

…mport support

SandishKumarHN · 2022-10-22T03:13:47Z

connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/SchemaConverters.scala

@@ -62,14 +63,16 @@ object SchemaConverters {
      case STRING => Some(StringType)
      case BYTE_STRING => Some(BinaryType)
      case ENUM => Some(StringType)
-      case MESSAGE if fd.getMessageType.getName == "Duration" =>
+      case MESSAGE if fd.getMessageType.getName == "Duration" &&


@rangadi My assumption is that users should be able to use the Timestamp and Duration message types with different fields.

SandishKumarHN · 2022-10-22T03:13:55Z

connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/ProtobufUtils.scala

    }

-    val descriptorProto: DescriptorProtos.FileDescriptorProto = fileDescriptorSet.getFile(0)
+    val descriptorProto: DescriptorProtos.FileDescriptorProto =


@rangadi This handles the protobuf import; please let me know if you know of a better approach.

AmplabJenkins · 2022-10-22T04:16:40Z

Can one of the admins verify this patch?

…uration validation

HyukjinKwon · 2022-10-24T02:25:53Z

cc @MaxGekk FYI

MaxGekk · 2022-10-24T04:50:56Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala

+    protobufType: String,
+    sqlType: String): Throwable = {
+    new AnalysisException(
+      errorClass = "_LEGACY_ERROR_TEMP_2251",


We use the prefix _LEGACY_ERROR_TEMP_ to convert existing exception to error classes but for new exception, please, assign appropriate names to error classes.

makes sense made changes.

MaxGekk · 2022-10-24T04:52:10Z

connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/ProtobufDeserializer.scala

+          toFieldStr(catalystPath),
+          s"${protoType} ${protoType.toProto.getLabel} ${protoType.getJavaType}" +
+            s" ${protoType.getType}",
+          catalystType.sql)


Pass catalystType into cannotConvertProtobufTypeToSqlTypeError(), and use toSQLType to quote the type.

rangadi · 2022-10-24T05:30:30Z

connector/protobuf/pom.xml

@@ -123,6 +123,7 @@
            <configuration>
              <protocArtifact>com.google.protobuf:protoc:${protobuf.version}</protocArtifact>
              <protocVersion>${protobuf.version}</protocVersion>
+              <includeMavenTypes>direct</includeMavenTypes>


Could you add comment here?
Is this for protos like `Timestamp' ? Looks like our code handles any Timestamp defined similar com.google.protobuf.Timestamp. Ok to not to include these.

@rangadi this protoc maven option is equal to CLI "protoc --include_imports" and I have added validations to protobuf Timestamp and Duration types. and currently, maven protoc does not generate classes for google protobuf types. sbt is enabled by default.

I see. What would fail if you remove this?

@rangadi just "messageClassName" param feature unit tests(Timestamp and Duration). The protoc plugin is only used for the test right?

That's because this removes existing 'Timestamp' message : https://github.com/apache/spark/pull/38344/files#diff-97aac63266f3c60eef9bd8dd1b76be3a5bd77fe4d17fa6fa370f5e0d9428a0a9L172

We could undo that change.

@rangadi how about supporting imports? I made this change of "Timestamp" and "Duration" to show a unit test for imports. even if we undo the above change, a unit test for import case needs plugin change

I think it is better to have import of our own proto files. Currently we don't have any. We can split some of them. I think I left a TODO about that in my PR. Many production proto files will have imports.

@rangadi how about splitting Timestamp and Duration into our own .proto files and then importing them in the functions_suite.proto file?

Yep. That would be nice. That will mimic normal proto files organization.

@rangadi made changes same.

rangadi · 2022-10-24T20:18:29Z

connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/ProtobufUtils.scala

    }

-    val descriptorProto: DescriptorProtos.FileDescriptorProto = fileDescriptorSet.getFile(0)
+    val descriptorProto: DescriptorProtos.FileDescriptorProto =


could you some brief comments here?
Is the last file the right file?

rangadi · 2022-10-24T20:20:05Z

connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/ProtobufUtils.scala

+    val descriptorProto: DescriptorProtos.FileDescriptorProto =
+      fileDescriptorSet.getFileList.asScala.last
+
+    var fileDescriptorList = List[Descriptors.FileDescriptor]()


Is this the import file list? What happens when the imported file imports other files? i.e. A imports B and B imports C.

@rangadi some changes were made to support nested imports. Reading protobuf descriptors from the bottom up, the last element in FileDescriptorSet is the initial FileDescriptorProto, from which we will continue to find more FileDescriptors recursively.

rangadi · 2022-10-24T20:23:45Z

connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/SchemaConverters.scala

-        //        google.protobuf package should default to corresponding Catalylist types.
+      case MESSAGE
+        if fd.getMessageType.getName == "Timestamp" &&
+        fd.getMessageType.getFields.size() == 2 &&


Could move all the conditions into braces (a & b & ...)?

rangadi · 2022-10-24T20:26:36Z

connector/protobuf/src/test/resources/protobuf/timestamp.proto

+message Timestamp {
+  int64 seconds = 1;
+  int32 nanos = 2;
+}


nit: Fix end-of-line.

rangadi · 2022-10-24T20:27:33Z

connector/protobuf/src/test/scala/org/apache/spark/sql/protobuf/ProtobufSerdeSuite.scala

    }

    assert(e.getMessage === expectMsg)
-    assert(e.getCause.getMessage === expectedCauseMessage)
+    if (e.getCause != null) {
+      assert(e.getCause.getMessage === expectedCauseMessage)


What does it mean if e.getCause is null and expectedCauseMessage is not?

made changes to support new error class framework.

…rotoErrorCls

MaxGekk · 2022-10-28T12:52:42Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala

+    new AnalysisException(
+      errorClass = "PROTOBUF_CLASS_LOAD_ERROR",
+      messageParameters = Map("protobufClassName" -> protobufClassName, "message" -> message),
+      cause = Some(cause.getCause))


getCause() can return null, correct. If so, could you wrap it by Option(cause.getCause)

MaxGekk · 2022-10-28T12:55:27Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala

+    new AnalysisException(errorClass = "INVALID_BYTE_STRING_ERROR", messageParameters = Map.empty)
+  }
+
+  def malformedRecordsDetectedInRecordParsingError(cause: Throwable): Throwable = {


Is it a compilation error not runtime (execution) one?

it is runtime, moved to ExecutionError class.

MaxGekk · 2022-10-28T12:56:24Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala

+    new AnalysisException(
+      errorClass = "PROTOBUF_DESCRIPTOR_PARSING_ERROR",
+      messageParameters = Map.empty(),
+      cause = Some(cause.getCause))


Some -> Option?

MaxGekk · 2022-10-28T12:56:41Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala

+    new AnalysisException(
+      errorClass = "CANNOT_FIND_PROTOBUF_DESCRIPTOR_FILE_ERROR",
+      messageParameters = Map("filePath" -> filePath),
+      cause = Some(cause.getCause))


Some -> Option

MaxGekk · 2022-10-28T12:56:56Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala

+    new AnalysisException(
+      errorClass = "PROTOBUF_DESCRIPTOR_ERROR",
+      messageParameters = Map.empty(),
+      cause = Some(cause.getCause))


Some -> Option

MaxGekk · 2022-10-28T12:57:25Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala

+      messageParameters = Map(
+        "protobufType" -> protobufType,
+        "toType" -> toSQLType(sqlType)),
+      cause = Some(cause.getCause))


Some -> Option

MaxGekk · 2022-10-28T12:57:35Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala

+      messageParameters = Map(
+        "protobufType" -> protobufType,
+        "toType" -> toSQLType(sqlType)),
+      cause = Some(cause.getCause))


Some -> Option

MaxGekk · 2022-10-28T13:00:12Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala

+    new AnalysisException(
+      errorClass = "CATALYST_TYPE_TO_PROTOBUF_ENUM_TYPE_ERROR",
+      messageParameters = Map(
+        "sqlColumn" -> sqlColumn,


If it is a column id, wrap it by toSQLId()

MaxGekk · 2022-10-28T13:02:23Z

connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/ProtobufSerializer.scala

-            s"schema is incompatible (sqlType = ${catalystType.sql}, " +
-            s"protoType = ${fieldDescriptor.getJavaType})")
+        throw QueryCompilationErrors.cannotConvertCatalystTypeToProtobufTypeError(
+          toFieldStr(catalystPath),


Please, don't quote it twice if you do that inside of cannotConvertCatalystTypeToProtobufTypeError() by toSQLId

okay, will call toSQLType inside the error class, avoid using toFieldStr for sqlType

srielau · 2022-10-28T18:12:54Z

core/src/main/resources/error/error-classes.json

@@ -23,6 +23,11 @@
    ],
    "sqlState" : "42000"
  },
+  "CANNOT_FIND_PROTOBUF_DESCRIPTOR_FILE_ERROR" : {


Definitely remove _ERROR.
Would be nice if we could gravitate towards *_NOT_FOUND and *_ALREADY_EXIST style naming convention:
PROTOBUF_DESCRIPTOR_FILE_NOT_FOUND

srielau · 2022-10-28T18:14:44Z

core/src/main/resources/error/error-classes.json

@@ -65,6 +70,11 @@
    ],
    "sqlState" : "22005"
  },
+  "CATALYST_TYPE_TO_PROTOBUF_ENUM_TYPE_ERROR" : {


I don't think we call out Catalyst anywhere else. How is that relevant? Can we be generic?

srielau · 2022-10-28T18:17:13Z

core/src/main/resources/error/error-classes.json

@@ -65,6 +70,11 @@
    ],
    "sqlState" : "22005"
  },
+  "CATALYST_TYPE_TO_PROTOBUF_ENUM_TYPE_ERROR" : {
+    "message" : [
+      "Cannot convert SQL <sqlColumn> to Protobuf <protobufColumn> because <data> cannot be written since it's not defined in ENUM <enumString>"


That's a because "squared"... I don't have enough context to propose a rephrase..
Also If we have SQL here, maybe we can do CATALYST -> SQL for the name?

@srielau there is no ENUM type in spark, so we convert the protobuf enum to spark string. so we are trying to throw an enum-specific error here.

srielau · 2022-10-28T18:17:34Z

core/src/main/resources/error/error-classes.json

@@ -517,6 +527,11 @@
      "Invalid bucket file: <path>"
    ]
  },
+  "INVALID_BYTE_STRING_ERROR" : {


Remove _ERROR

srielau · 2022-10-28T18:18:42Z

core/src/main/resources/error/error-classes.json

@@ -517,6 +527,11 @@
      "Invalid bucket file: <path>"
    ]
  },
+  "INVALID_BYTE_STRING_ERROR" : {
+    "message" : [
+      "Invalid ByteString format"


Can we spell out the invalid format?
For extra points: Can we spell out a valid format?

srielau · 2022-10-28T18:29:08Z

core/src/main/resources/error/error-classes.json

+      "Protobuf type not yet supported: <protobufType>."
+    ]
+  },
+  "PROTOBUF_TYPE_TO_CATALYST_TYPE_ERROR" : {


Suggested change

"PROTOBUF_TYPE_TO_CATALYST_TYPE_ERROR" : {

"CANNOT_CONVERT_PROTOBUF_TYPE_TO_SQL_TYPE" : {

this gives which protobuf message had failed to convert, it would be the protobuf root name since the protobuf message might have a long list of fields, just keeping the root name.

srielau · 2022-10-28T18:30:19Z

core/src/main/resources/error/error-classes.json

+      "Unable to convert <protobufType> of Protobuf to SQL type <toType>."
+    ]
+  },
+  "PROTOBUF_TYPE_TO_SQL_TYPE_ERROR" : {


How is this different from eh one above? Just more info available?

this gives which protobuf field had failed to convert, this clearly describes the field name and field type.

srielau · 2022-10-28T18:31:10Z

core/src/main/resources/error/error-classes.json

@@ -760,6 +850,11 @@
    ],
    "sqlState" : "22023"
  },
+  "SQL_TYPE_TO_PROTOBUF_TYPE_ERROR" : {


Suggested change

"SQL_TYPE_TO_PROTOBUF_TYPE_ERROR" : {

"CANNOT_CONVERT_SQL_TYPE_TO_PROTOBUF_TYPE" : {

srielau · 2022-10-28T19:02:06Z

core/src/main/resources/error/error-classes.json

@@ -792,6 +887,21 @@
      "Unable to acquire <requestedBytes> bytes of memory, got <receivedBytes>"
    ]
  },
+  "UNABLE_TO_CONVERT_TO_PROTOBUF_TYPE" : {


Same as above?

srielau · 2022-10-28T19:02:41Z

core/src/main/resources/error/error-classes.json

+      "Unable to convert SQL type <toType> to Protobuf type <protobufType>."
+    ]
+  },
+  "UNABLE_TO_LOCATE_PROTOBUF_MESSAGE_ERROR" : {


Suggested change

"UNABLE_TO_LOCATE_PROTOBUF_MESSAGE_ERROR" : {

"PROTOBUF_MESSAGE_NOT_FOUND" : {

@srielau fixed all the error class names

error class name changes, more details to error message

…rotoErrorCls

rangadi · 2022-10-27T01:38:01Z

connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/ProtobufUtils.scala

@@ -175,19 +175,26 @@ private[sql] object ProtobufUtils extends Logging {
      .asInstanceOf[Descriptor]
  }

+  // TODO: Revisit to ensure that messageName is searched through all imports


What is missing? Looks fairly complete to me.
Better to state the problem here.

rangadi · 2022-10-27T01:40:50Z

connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/ProtobufUtils.scala

    }

-    descriptor match {
+    descriptorList.last match {


Could you add a comment on why we are picking the last one? Will be useful for future readers as well.

rangadi · 2022-11-01T04:18:53Z

connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/ProtobufUtils.scala

    }
  }

-  private def parseFileDescriptor(descFilePath: String): Descriptors.FileDescriptor = {
+  private def parseFileDescriptor(descFilePath: String): List[Descriptors.FileDescriptor] = {


Rename to parseFileDescriptorSet (otherwise it sounds like it is parsing just one file descriptor).

rangadi · 2022-11-01T04:21:52Z

connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/ProtobufUtils.scala

  def buildDescriptor(descFilePath: String, messageName: String): Descriptor = {
-    val descriptor = parseFileDescriptor(descFilePath).getMessageTypes.asScala.find { desc =>
-      desc.getName == messageName || desc.getFullName == messageName
+    val descriptorList = parseFileDescriptor(descFilePath).map(fileDescriptor => {


Style: use find() rather than map().filter().

(you can use findLast() if there is a reason to use the last match).

@rangadi makes sense to use find and return, fixed.

…rotoErrorCls

rangadi

LGTM on import functionality changes. Will let other reviewers to approve the error message changes.

rangadi · 2022-11-01T17:01:50Z

connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/ProtobufUtils.scala

-      desc.getName == messageName || desc.getFullName == messageName
-    }
+    val fileDescriptor = parseFileDescriptorSet(descFilePath)
+      .find(!_.getMessageTypes.asScala.find(desc =>


Oh, we need to find() invocations and check is made twice. How about this? [Optional to do, only a suggestion]

// Find the first message descriptor that matches the name. val descriptorOpt = parseFileDescriptorSet(descFilePath) .flatMap { fileDesc => fileDesc.getMessageTypes.asScala.find { desc => desc.getName == messageName || desc.getFullName == messageName } }.headOption descriptoOpt match { case Some(d) => d case None => throw QueryCompilationErrors.unableToLocateProtobufMessageError(messageName) }

srielau · 2022-11-02T23:39:35Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala

+    val message = if (hasDots) "" else ". Ensure the class name includes package prefix."
+    new AnalysisException(
+      errorClass = "CANNOT_LOAD_PROTOBUF_CLASS",
+      messageParameters = Map("protobufClassName" -> protobufClassName, "message" -> message),


We don't want text as a parameter. Aside to make this text depends on whether there is any dot seems to be quite subtle. Maybe it simply doesn't have enough dots?
I propose to move this text into the error message (unconditionally).

srielau · 2022-11-02T23:41:14Z

core/src/main/resources/error/error-classes.json

@@ -742,6 +832,11 @@
    ],
    "sqlState" : "22023"
  },
+  "SQL_TYPE_TO_PROTOBUF_ENUM_TYPE_ERROR" : {


Rename without _ERROR

srielau · 2022-11-02T23:43:13Z

core/src/main/resources/error/error-classes.json

+      "Type mismatch encountered for field: <field>"
+    ]
+  },
+  "PROTOBUF_FIELD_TYPE_TO_SQL_TYPE_ERROR" : {


How about:

Suggested change

"PROTOBUF_FIELD_TYPE_TO_SQL_TYPE_ERROR" : {

"CANNOT_CONVERT_PROTOBUF_FIELD_TYPE_TO_SQL_TYPE" : {

srielau · 2022-11-02T23:44:14Z

core/src/main/resources/error/error-classes.json

@@ -29,12 +44,22 @@
    ],
    "sqlState" : "22007"
  },
+  "CANNOT_LOAD_PROTOBUF_CLASS" : {
+    "message" : [
+      "Could not load Protobuf class with name <protobufClassName><message>"


Suggested change

"Could not load Protobuf class with name <protobufClassName><message>"

"Could not load Protobuf class with name <protobufClassName>. Ensure the class name includes package prefix."

@srielau fixed the suggested changes.

srielau

LGTM

rangadi · 2022-11-04T03:23:30Z

@MaxGekk are you still reviewing this?
@SandishKumarHN is there any more review to be addressed? If we are ready, I can ask @HeartSaVioR to merge this (before his weekend starts in Seoul :) ).

SandishKumarHN · 2022-11-04T04:41:43Z

@MaxGekk are you still reviewing this? @SandishKumarHN is there any more review to be addressed? If we are ready, I can ask @HeartSaVioR to merge this (before his weekend starts in Seoul :) ).

@rangadi I have addressed all of the review comments.

MaxGekk

Error classes LGTM

HeartSaVioR

+1

HeartSaVioR · 2022-11-04T05:49:46Z

Thanks! Merging to master.

MaxGekk · 2022-11-05T14:43:00Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala

+  def descrioptorParseError(descFilePath: String, cause: Throwable): Throwable = {
+    new AnalysisException(
+      errorClass = "CANNOT_PARSE_PROTOBUF_DESCRIPTOR",
+      messageParameters = Map.empty("descFilePath" -> descFilePath),


Accidentally found this. Is messageParameters set to an empty map, if so, this will fail in runtime w/ an internal error. Could you write a test which trigger the error and fix the bug.

MaxGekk · 2022-11-05T14:43:43Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala

+  def failedParsingDescriptorError(descFilePath: String, cause: Throwable): Throwable = {
+    new AnalysisException(
+      errorClass = "CANNOT_CONSTRUCT_PROTOBUF_DESCRIPTOR",
+      messageParameters = Map.empty("descFilePath" -> descFilePath),


@SandishKumarHN Please, fix this. Map.empty -> Map

…lasses This is the follow-up PR to apache#37972 and apache#38212 ### What changes were proposed in this pull request? 1. Move spark-protobuf error classes to the spark error-classes framework(core/src/main/resources/error/error-classes.json). 2. Support protobuf imports 3. validate protobuf timestamp and duration types. ### Why are the changes needed? N/A ### Does this PR introduce _any_ user-facing change? None ### How was this patch tested? Existing tests should cover the validation of this PR. CC: rangadi mposdev21 gengliangwang Closes apache#38344 from SandishKumarHN/SPARK-40777-ProtoErrorCls. Authored-by: SandishKumarHN <[email protected]> Signed-off-by: Jungtaek Lim <[email protected]>

SandishKumarHN added 7 commits October 17, 2022 21:21

review changes nit

c6177a3

adding spark-protobuf error class into spark error-class framework

8fad018

adding spark-protobuf error class into spark error-class framework

dac2fa8

adding spark-protobuf error class into spark error-class framework, i…

d1c9b1e

…mport support

Merge remote-tracking branch 'remote-spark/master' into SPARK-40777

03b918f

# Conflicts: # connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/ProtobufUtils.scala # connector/protobuf/src/test/scala/org/apache/spark/sql/protobuf/ProtobufFunctionsSuite.scala

adding spark-protobuf error class into spark error-class framework, i…

60c2122

…mport support

Merge branch 'SPARK-40777' into SPARK-40777-ProtoErrorCls

7c866de

github-actions bot added CORE PROTOBUF SQL labels Oct 22, 2022

SandishKumarHN commented Oct 22, 2022

View reviewed changes

SandishKumarHN force-pushed the SPARK-40777-ProtoErrorCls branch from 90c61cc to c78ddca Compare October 22, 2022 04:47

github-actions bot added the BUILD label Oct 22, 2022

spark-protobuf error clss frameworks, import support, timestamp and d…

e6f3cab

…uration validation

SandishKumarHN force-pushed the SPARK-40777-ProtoErrorCls branch from c78ddca to e6f3cab Compare October 22, 2022 04:50

HyukjinKwon changed the title ~~[SPARK-40777][SQL][SPARK-PROTOBUF] : Protobuf import support and move error-classes.~~ [SPARK-40777][SQL][PROTOBUF] Protobuf import support and move error-classes. Oct 24, 2022

fixing typos

70a5983

MaxGekk requested changes Oct 24, 2022

View reviewed changes

rangadi reviewed Oct 24, 2022

View reviewed changes

SandishKumarHN requested a review from MaxGekk October 24, 2022 06:20

SandishKumarHN force-pushed the SPARK-40777-ProtoErrorCls branch from 7cea436 to a40c748 Compare October 24, 2022 06:24

review changes, name protobuf error class and comment in pom.xml

26e471b

SandishKumarHN force-pushed the SPARK-40777-ProtoErrorCls branch from a40c748 to 26e471b Compare October 24, 2022 17:52

review chages rm includeMavenTypes and import own protos

68f87c1

rangadi reviewed Oct 24, 2022

View reviewed changes

SandishKumarHN force-pushed the SPARK-40777-ProtoErrorCls branch from 34ac86a to 38f43b0 Compare October 25, 2022 04:10

review changes: nested import support, nit, build fix

9cdf4d5

SandishKumarHN added 2 commits October 27, 2022 08:32

review changes: use prettyName

3fc59b5

Merge remote-tracking branch 'remote-spark/master' into SPARK-40777-P…

e5482a5

…rotoErrorCls

MaxGekk reviewed Oct 28, 2022

View reviewed changes

review changes: nit, option, runtime error

dd63be8

SandishKumarHN force-pushed the SPARK-40777-ProtoErrorCls branch from 39b0981 to dd63be8 Compare October 28, 2022 16:32

srielau suggested changes Oct 28, 2022

View reviewed changes

SandishKumarHN force-pushed the SPARK-40777-ProtoErrorCls branch 2 times, most recently from 0229009 to 80b17a1 Compare October 28, 2022 20:52

error class name changes, more details to error message

e5140b0

error class name changes, more details to error message

SandishKumarHN force-pushed the SPARK-40777-ProtoErrorCls branch from 80b17a1 to e5140b0 Compare October 29, 2022 00:49

SandishKumarHN added 2 commits October 28, 2022 22:55

Merge remote-tracking branch 'remote-spark/master' into SPARK-40777-P…

3037415

…rotoErrorCls

NO_UDF_INTERFACE_ERROR to NO_UDF_INTERFACE

be02c9e

rangadi reviewed Nov 1, 2022

View reviewed changes

SandishKumarHN added 2 commits October 31, 2022 22:49

review changes scala style, find, parseFileDescriptorSet

d8cce82

Merge remote-tracking branch 'remote-spark/master' into SPARK-40777-P…

ad1f7e1

…rotoErrorCls

rangadi approved these changes Nov 1, 2022

View reviewed changes

review changes buildDescriptor suggested changes

48bcb5c

SandishKumarHN requested a review from srielau November 1, 2022 17:44

srielau suggested changes Nov 2, 2022

View reviewed changes

review changes, error classes

87918a1

srielau approved these changes Nov 3, 2022

View reviewed changes

MaxGekk reviewed Nov 4, 2022

View reviewed changes

HeartSaVioR approved these changes Nov 4, 2022

View reviewed changes

HeartSaVioR closed this in 5741d38 Nov 4, 2022

MaxGekk reviewed Nov 5, 2022

View reviewed changes

SandishKumarHN mentioned this pull request Nov 5, 2022

[SPARK-41015][SQL][PROTOBUF] UnitTest null check for data generator #38515

Closed

	"PROTOBUF_TYPE_TO_CATALYST_TYPE_ERROR" : {
	"CANNOT_CONVERT_PROTOBUF_TYPE_TO_SQL_TYPE" : {

	"SQL_TYPE_TO_PROTOBUF_TYPE_ERROR" : {
	"CANNOT_CONVERT_SQL_TYPE_TO_PROTOBUF_TYPE" : {

	"UNABLE_TO_LOCATE_PROTOBUF_MESSAGE_ERROR" : {
	"PROTOBUF_MESSAGE_NOT_FOUND" : {

	"PROTOBUF_FIELD_TYPE_TO_SQL_TYPE_ERROR" : {
	"CANNOT_CONVERT_PROTOBUF_FIELD_TYPE_TO_SQL_TYPE" : {

	"Could not load Protobuf class with name <protobufClassName><message>"
	"Could not load Protobuf class with name <protobufClassName>. Ensure the class name includes package prefix."

[SPARK-40777][SQL][PROTOBUF] Protobuf import support and move error-classes. #38344

[SPARK-40777][SQL][PROTOBUF] Protobuf import support and move error-classes. #38344

Conversation

SandishKumarHN commented Oct 22, 2022 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented Oct 22, 2022

HyukjinKwon commented Oct 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SandishKumarHN Oct 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SandishKumarHN Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SandishKumarHN Oct 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SandishKumarHN Oct 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rangadi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srielau left a comment

SandishKumarHN commented Oct 22, 2022 •

edited

Loading

SandishKumarHN Oct 24, 2022 •

edited

Loading

SandishKumarHN Oct 25, 2022 •

edited

Loading

SandishKumarHN Oct 28, 2022 •

edited

Loading

SandishKumarHN Oct 28, 2022 •

edited

Loading