nchammas commented on code in PR #56174:
URL: https://github.com/apache/spark/pull/56174#discussion_r4142684190
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -6919,6 +6919,12 @@
],
"sqlState" : "42K0G"
},
+ "PROTOBUF_DESCRIPTOR_FILE_READ_FAILED" : {
Review Comment:
@HyukjinKwon - I think in a future PR it _would_ make sense to add this to
the Scala side and replace the (IMO incorrect) uses of
`CANNOT_PARSE_PROTOBUF_DESCRIPTOR`: [case 1], [case 2]
In both these cases no attempt is made to parse the bytes. The failures are
before any parsing, so the current error condition name is misleading.
`PROTOBUF_DESCRIPTOR_FILE_READ_FAILED` is more appropriate.
This is in contrast to other cases where there is a genuine problem
specifically with parsing: [case 3]
Not a huge deal, and maybe we don't want to change existing behavior. But
something to consider, especially if it becomes more important for clients to
distinguish these types of failures.
[case 1]:
https://github.com/apache/spark/blob/578105289d82158f7d04d7976d547038d1a270c2/sql/api/src/main/scala/org/apache/spark/sql/util/ProtobufUtils.scala#L36
[case 2]:
https://github.com/apache/spark/blob/578105289d82158f7d04d7976d547038d1a270c2/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/ProtobufDescriptorFileReader.scala#L89-L95
[case 3]:
https://github.com/apache/spark/blob/578105289d82158f7d04d7976d547038d1a270c2/connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/ProtobufUtils.scala#L310-L315
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
