-
Notifications
You must be signed in to change notification settings - Fork 28.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-41015][SQL][PROTOBUF] UnitTest null check for data generator #38515
Changes from 3 commits
bcc333f
d4e26b9
f11b04d
c9abb2d
44e9209
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
|
||
� | ||
basicmessage.proto$org.apache.spark.sql.protobuf.protosnestedenum.proto"� | ||
BasicMessage | ||
id (Rid! | ||
string_value ( RstringValue | ||
int32_value (R | ||
int32Value | ||
int64_value (R | ||
int64Value! | ||
double_value (RdoubleValue | ||
float_value (R | ||
floatValue | ||
|
||
bool_value (R boolValue | ||
bytes_value (R | ||
bytesValueS | ||
rnested_enum (20.org.apache.spark.sql.protobuf.protos.NestedEnumRrnestedEnumBBBasicMessageProtobproto3 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -123,16 +123,21 @@ class ProtobufCatalystDataConversionSuite | |
StringType -> ("StringMsg", "")) | ||
|
||
testingTypes.foreach { dt => | ||
val seed = 1 + scala.util.Random.nextInt((1024 - 1) + 1) | ||
val seed = scala.util.Random.nextInt(RandomDataGenerator.MAX_STR_LEN) | ||
test(s"single $dt with seed $seed") { | ||
|
||
val (messageName, defaultValue) = catalystTypesToProtoMessages(dt.fields(0).dataType) | ||
|
||
val rand = new scala.util.Random(seed) | ||
val generator = RandomDataGenerator.forType(dt, rand = rand).get | ||
var data = generator() | ||
while (data.asInstanceOf[Row].get(0) == defaultValue) // Do not use default values, since | ||
data = generator() // from_protobuf() returns null in v3. | ||
// Do not use default values, since from_protobuf() returns null in v3. | ||
while ( | ||
data != null && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should just be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant, we don't need the array check. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Except for (5), none of them worked. I'm printing under the conditions listed above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. Equals does not check the content on array.
|
||
(data.asInstanceOf[Row].get(0) == defaultValue || | ||
(data.asInstanceOf[Row].get(0).isInstanceOf[Array[Byte]] && | ||
data.asInstanceOf[Row].get(0).asInstanceOf[Array[Byte]].isEmpty))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't |
||
data = generator() | ||
|
||
val converter = CatalystTypeConverters.createToCatalystConverter(dt) | ||
val input = Literal.create(converter(data), dt) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -177,6 +177,31 @@ class ProtobufSerdeSuite extends SharedSparkSession { | |
withFieldMatchType(Deserializer.create(CATALYST_STRUCT, protoNestedFile, _)) | ||
} | ||
|
||
test("raise cannot parse protobuf descriptor error") { | ||
// passing serde_suite.proto instead serde_suite.desc | ||
val testFileDesc = testFile("serde_suite.proto").replace("file:/", "/") | ||
val e = intercept[AnalysisException] { | ||
ProtobufUtils.buildDescriptor(testFileDesc, "FieldMissingInSQLRoot") | ||
} | ||
|
||
checkError( | ||
exception = e, | ||
errorClass = "CANNOT_PARSE_PROTOBUF_DESCRIPTOR", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @SandishKumarHN Thank you for the added test. |
||
parameters = Map("descFilePath" -> testFileDesc)) | ||
} | ||
|
||
test("raise cannot construct protobuf descriptor error") { | ||
val testFileDesc = testFile("basicmessage_noimports.desc").replace("file:/", "/") | ||
val e = intercept[AnalysisException] { | ||
ProtobufUtils.parseFileDescriptorSet(testFileDesc) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for adding the test. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rangadi Yes, but the query would catch this error and throw a different error right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Users don't call this. Analysis exception thrown to the user if they try to use this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rangadi Check it out once again I've made a few minor changes. |
||
} | ||
|
||
checkError( | ||
exception = e, | ||
errorClass = "CANNOT_CONSTRUCT_PROTOBUF_DESCRIPTOR", | ||
parameters = Map("descFilePath" -> testFileDesc)) | ||
} | ||
|
||
/** | ||
* Attempt to convert `catalystSchema` to `protoSchema` (or vice-versa if `deserialize` is | ||
* true), assert that it fails, and assert that the _cause_ of the thrown exception has a | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3330,7 +3330,7 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase { | |
def descrioptorParseError(descFilePath: String, cause: Throwable): Throwable = { | ||
new AnalysisException( | ||
errorClass = "CANNOT_PARSE_PROTOBUF_DESCRIPTOR", | ||
messageParameters = Map.empty("descFilePath" -> descFilePath), | ||
messageParameters = Map("descFilePath" -> descFilePath), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @MaxGekk fixing this one #38344 (comment) here |
||
cause = Option(cause.getCause)) | ||
} | ||
|
||
|
@@ -3344,7 +3344,7 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase { | |
def failedParsingDescriptorError(descFilePath: String, cause: Throwable): Throwable = { | ||
new AnalysisException( | ||
errorClass = "CANNOT_CONSTRUCT_PROTOBUF_DESCRIPTOR", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add a test for this error class. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @MaxGekk added |
||
messageParameters = Map.empty("descFilePath" -> descFilePath), | ||
messageParameters = Map("descFilePath" -> descFilePath), | ||
cause = Option(cause.getCause)) | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this used?
Did you mean to another test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rangadi yes, it is used in the unit test above, how else can we raise the CANNOT_CONSTRUCT_PROTOBUF_DESCRIPTOR exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I didn't see the second test carefully.