Adding null fix for rows and col stats information.#14617
Adding null fix for rows and col stats information.#14617cryptoe merged 2 commits intoapache:masterfrom
Conversation
cryptoe
added
Area - MSQ
LakshSingla
left a comment
LakshSingla
left a comment
There was a problem hiding this comment.
Nitty comment, PR LGTM 💯
| for (PageInformation pageInformation : pageList.get()) { | ||
| rows += pageInformation.getNumRows(); | ||
| size += pageInformation.getSizeInBytes(); | ||
| rows += pageInformation.getNumRows() != null ? pageInformation.getNumRows() : 0L; |
There was a problem hiding this comment.
nit:
From the code, it seems like numRows can't be null, so can we modify the PageInformation instead to remove the Nullable tag for numRows, convert it to a long, and remove the check here?
Similarly, with this change, we aren't using the nullity of PageInformation::sizeOfBytes() anywhere apart from this check, therefore can we convert it to a primitive long, populate it as 0 instead of null, remove the Nullable and clean up the special handling here?
Since we don't require any special ability of the boxed types, it seems better to clean them up. WDYT?
There was a problem hiding this comment.
Since page information is serialized as json in the SqlStatementResult, I prefer PageInformation#numRows and sizeOfBytes to be Long. That way if they are not populated for whatever reason, they are not sent across to the end users.
There was a problem hiding this comment.
In that case, please add @JsonInclude(JsonInclude.Include.NON_NULL) to the Jackson annotations of the members of PageInformation. By default, we do serialize and present null values to the user. So per the current behavior, page information would get serialized as {"numRows":null,"sizeInBytes":null,"id":5}.
Approving the PR since the fix is not related to the current change.
There was a problem hiding this comment.
Good catch. Will do that in the next PR.
|
Thanks @LakshSingla for the review. |
* Adding null fix for rows and col stats information. * Null handling test case fix (cherry picked from commit ae168c4)
* Adding null fix for rows and col stats information. * Null handling test case fix
2 participants
Null pointer fix for sql statement endpoint for "replace/Insert" q's. Since we donot populate the size counters for segment generation stage, the value is always null. Adjusted the code to be more defensive.
This PR has: