-
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
Conversation
@rangadi Because some random numbers do not get converted to catalyst type, a null check for the data generator is required. |
69be828
to
bcc333f
Compare
Can one of the admins verify this patch? |
data != null && | ||
(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 comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't ByteString.empty().toByteArray
at line 122 already empty value?
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 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.
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 with val data = generator().asInstanceOf[Row]
.
Also could replace
data.asInstanceOf[Row].get(0).isInstanceOf[Array[Byte]]
with
dt == BinaryType
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
@MaxGekk fixing this one #38344 (comment) here
|
||
checkError( | ||
exception = e, | ||
errorClass = "CANNOT_PARSE_PROTOBUF_DESCRIPTOR", |
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.
@SandishKumarHN Thank you for the added test.
@@ -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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
@MaxGekk added
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 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 with val data = generator().asInstanceOf[Row]
.
Also could replace
data.asInstanceOf[Row].get(0).isInstanceOf[Array[Byte]]
with
dt == BinaryType
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the test.
Do we need to invoke this? Any query would raise this, right?
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, 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 comment
The 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 comment
The 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.
@@ -0,0 +1,18 @@ | |||
|
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.
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 comment
The 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 df
.
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.
correct, fixed.
@MaxGekk, please merge once this this looks good to you. |
@SandishKumarHN @rangadi I will review and merge it (if everything is ok, and test passed) tomorrow. |
+1, LGTM. Merging to master. |
### What changes were proposed in this pull request? null check for data generator after type conversion. ### Why are the changes needed? To fix a test failure. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? I have tested all the random numbers manually, current unit tests. Closes apache#38515 from SandishKumarHN/SPARK-41015-UTests. Authored-by: SandishKumarHN <sanysandish@gmail.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>
What changes were proposed in this pull request?
null check for data generator after type conversion.
Why are the changes needed?
To fix a test failure.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
I have tested all the random numbers manually, current unit tests.