-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[HUDI-6072] Fix NPE when upsert merger and null map or array #8432
Conversation
hudi-client/hudi-spark-client/src/main/scala/org/apache/spark/sql/HoodieInternalRowUtils.scala
Outdated
Show resolved
Hide resolved
Tests based on the spark datasource api (similar to the eg in the OP) or do you have a better idea?
…On April 13, 2023 6:35:17 AM UTC, Danny Chan ***@***.***> wrote:
@danny0405 commented on this pull request.
> - val newArrayData = createArrayData(newElementType, prevArrayData.numElements())
- val elementUpdater = new ArrayDataUpdater(newArrayData)
-
- var i = 0
- while (i < prevArray.length) {
- val element = prevArray(i)
- if (element == null) {
- if (!containsNull) {
- throw new HoodieException(
- s"Array value at path '${fieldNameStack.asScala.mkString(".")}' is not allowed to be null")
+ if (value == null) {
+ fieldUpdater.setNullAt(ordinal)
+ } else {
+ val prevArrayData = value.asInstanceOf[ArrayData]
+ val prevArray = prevArrayData.toObjectArray(prevElementType)
+
Thanks for the contribution, can we add some test cases?
--
Reply to this email directly or view it on GitHub:
#8432 (review)
You are receiving this because you authored the thread.
Message ID: ***@***.***>
|
Either data sorurce api or sql should be fine. |
@danny0405 added stuff for tests, bug cannot setup intellij to debug/run tests locally (pain following the https://hudi.apache.org/contribute/developer-setup) |
Could we fix line 191 instead, that could fix all the data type with null values, covering also the atomic data types. You can also use the |
Thanks for the fix, I have reviewed and created a patch: |
@hudi-bot run azure |
@danny0405 integrated your patch. Now I need to:
|
Yeah, that makes sense, I run the 2 tests locally and they can pass. |
also make sure the exception is raised when the fix is not applied
@danny0405 added a commit to apply the exact same context as the issue. The previous tests did not fail w/o the patch. |
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.
+1
…8432) Co-authored-by: Danny Chan <yuzhao.cyz@gmail.com>
…8432) Co-authored-by: Danny Chan <yuzhao.cyz@gmail.com>
…8432) Co-authored-by: Danny Chan <yuzhao.cyz@gmail.com>
Change Logs
Fixes #8431
Impact
Describe any public API or user-facing feature change or any performance impact.
Risk level (write none, low medium or high below)
none
Documentation Update
Describe any necessary documentation update if there is any new feature, config, or user-facing change
ticket number here and follow the instruction to make
changes to the website.
Contributor's checklist