Skip to content

Conversation

@scovich
Copy link
Contributor

@scovich scovich commented Sep 20, 2025

Which issue does this PR close?

Rationale for this change

Somehow, #8392 exposes a latent bug in #8366, which has improper NULL handling for shredded object fields. The shredding PR originally attempted to handle this case, but somehow the test did not trigger the bug and so the (admittedly incomplete) code was removed. See #8366 (comment). To be honest, I have no idea how the original ever worked correctly, nor why the new PR is able to expose the problem.

What changes are included in this PR?

When used as a top-level builder, VariantToShreddedVariantRowBuilder::append_null must append NULL to its own NullBufferBuilder; but when used as a shredded object field builder, it must append non-NULL. Plumb a new top_level parameter through the various functions and into the two sub-builders it relies on, so they can implement the correct semantics.

Are these changes tested?

In theory, yes (I don't know how the object shredding test ever passed). And it fixes the breakage in #8392.

Are there any user-facing changes?

No

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @scovich

@alamb alamb merged commit 8394659 into apache:main Sep 20, 2025
18 checks passed
@scovich
Copy link
Contributor Author

scovich commented Sep 20, 2025

@alamb -- we should track a follow-on item somewhere: Once we support array element shredding, we will have three different semantics to deal with, and this fix will need to expand from a bool to an enum:

When appending NULL:

struct value typed_value
Variant NULL NULL NULL
Object field valid NULL NULL
Array element valid Variant::Null NULL

@alamb
Copy link
Contributor

alamb commented Sep 21, 2025

@alamb -- we should track a follow-on item somewhere: Once we support array element shredding, we will have three different semantics to deal with, and this fix will need to expand from a bool to an enum:

Filed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

parquet-variant parquet-variant* crates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants