-
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 1 commit
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 |
---|---|---|
|
@@ -677,4 +677,34 @@ class ProtobufFunctionsSuite extends QueryTest with SharedSparkSession with Seri | |
=== inputDf.select("durationMsg.duration").take(1).toSeq(0).get(0)) | ||
} | ||
} | ||
|
||
test("raise cannot construct protobuf descriptor error") { | ||
val basicMessageDesc = ProtobufUtils.buildDescriptor(testFileDesc, "BasicMessage") | ||
|
||
val basicMessage = DynamicMessage | ||
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. Btw, you don't need the message. Could use empty byte array while initializing 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. correct, fixed. |
||
.newBuilder(basicMessageDesc) | ||
.setField(basicMessageDesc.findFieldByName("id"), 1111L) | ||
.setField(basicMessageDesc.findFieldByName("string_value"), "slam") | ||
.setField(basicMessageDesc.findFieldByName("int32_value"), 12345) | ||
.setField(basicMessageDesc.findFieldByName("int64_value"), 0x90000000000L) | ||
.setField(basicMessageDesc.findFieldByName("double_value"), 10000000000.0d) | ||
.setField(basicMessageDesc.findFieldByName("float_value"), 10902.0f) | ||
.setField(basicMessageDesc.findFieldByName("bool_value"), true) | ||
.setField( | ||
basicMessageDesc.findFieldByName("bytes_value"), | ||
ByteString.copyFromUtf8("ProtobufDeserializer")) | ||
.build() | ||
|
||
val df = Seq(basicMessage.toByteArray).toDF("value") | ||
val testFileDescriptor = testFile("basicmessage_noimports.desc").replace("file:/", "/") | ||
|
||
val e = intercept[AnalysisException] { | ||
df.select(functions.from_protobuf($"value", "BasicMessage", testFileDescriptor) as 'sample) | ||
.where("sample.string_value == \"slam\"").show() | ||
} | ||
checkError( | ||
exception = e, | ||
errorClass = "CANNOT_CONSTRUCT_PROTOBUF_DESCRIPTOR", | ||
parameters = Map("descFilePath" -> testFileDescriptor)) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -177,27 +177,25 @@ class ProtobufSerdeSuite extends SharedSparkSession { | |
withFieldMatchType(Deserializer.create(CATALYST_STRUCT, protoNestedFile, _)) | ||
} | ||
|
||
test("raise cannot parse protobuf descriptor error") { | ||
test("raise cannot parse and construct protobuf descriptor error") { | ||
// passing serde_suite.proto instead serde_suite.desc | ||
val testFileDesc = testFile("serde_suite.proto").replace("file:/", "/") | ||
val e = intercept[AnalysisException] { | ||
var testFileDesc = testFile("serde_suite.proto").replace("file:/", "/") | ||
val e1 = intercept[AnalysisException] { | ||
ProtobufUtils.buildDescriptor(testFileDesc, "FieldMissingInSQLRoot") | ||
} | ||
|
||
checkError( | ||
exception = e, | ||
exception = e1, | ||
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) | ||
testFileDesc = testFile("basicmessage_noimports.desc").replace("file:/", "/") | ||
val e2 = intercept[AnalysisException] { | ||
ProtobufUtils.buildDescriptor(testFileDesc, "FieldMissingInSQLRoot") | ||
} | ||
|
||
checkError( | ||
exception = e, | ||
exception = e2, | ||
errorClass = "CANNOT_CONSTRUCT_PROTOBUF_DESCRIPTOR", | ||
parameters = Map("descFilePath" -> testFileDesc)) | ||
} | ||
|
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.
This should just be
data == null || data.asInstanceOf[Row].get(0) == defaultValue
?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.
I meant, we don't need the array check.
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
data.asInstanceOf[Row].get(0) == ByteString.empty().toByteArray
data.asInstanceOf[Row].get(0) == Array.emptyByteArray
data.asInstanceOf[Row].get(0) == ByteString.EMPTY.toByteArray
data.asInstanceOf[Row].get(0) == "".getBytes
data.asInstanceOf[Row].get(0).isInstanceOf[Array[Byte]] && data.asInstanceOf[Row].get(0).asInstanceOf[Array[Byte]].isEmpty
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 comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Equals does not check the content on array.
Optional:
We could recduce
data.asInstanceOf
calls withval data = generator().asInstanceOf[Row]
.Also could replace