-
Notifications
You must be signed in to change notification settings - Fork 3.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
GH-37072: [C++] GetArrayView: allow non-nullable fields to be null if parent is null #37202
Conversation
|
I'm honestly not sure this is the right solution. If a field is not nullable, then ideally it shouldn't have any nulls even if it is shadowed by an all-nulls parent array. Other implementations may also not like receiving such data over IPC or the C Data Interface. @alippai @lidavidm @paleolimbot What do you think? Also, in any case, this should come with unit tests! |
But what value should a field of a struct have when the entire struct is a logical null? Isn't that completely undefined and a waste of time to be populated? |
It's undefined but it needs to be populated anyway :-) (we can't leave memory uninitialized as it can be serialized through IPC, for example)
|
But what you are asking for here requires a validity buffer set to |
Right, it would add a bit of complexity, hopefully not too much. Most types would probably be ok with |
Practically I think implementations are not great about using the nullability flag, instead relying mostly on the physical presence of nulls, but this is a chicken-and-egg issue.
This sounds reasonable, but we should clarify this more generally? It sounds like we aren't precise enough with that field nullability means for the physical data. |
Hmm, it seems like this could be discussed on the mailing-list. Perhaps we don't want to impose undue constraints here. |
If we were to got that route we would have to define the "default value" for each type similar to how protobuf 3 is allowed to omit data and default values are assumed.
I vote in favor of not imposing constraints. This is why I don't think the way to go here is "fixing" |
We don't need to officially define "default values", just make sure we don't produce invalid data (such as non-monotonic offsets). This is exactly the idea behind arrow/cpp/src/arrow/array/builder_base.h Lines 156 to 161 in d330d1b
But this discussion can also regard other implementations, I think it would be useful to discuss on the ML. |
My only insight here is that nullability in nested types and the need of fast paths are causing non-trivial performance degradation in parquet serde. So if restricting some edge cases helps here, likely it’ll help in other code paths as well. |
I'm not sure I understand all of this; however, would this mean that there could be an |
The discussion is needed if we decide to put the definition of empty value (a better terminology than "default value" I used above) on the spec. My current position is that validation should be relaxed and not assume other implementations are producing non-null fields inside null structs. For instance, I believe that a compute kernel, in the official implementation or other implementations, should be allowed to skip producing empty values inside of null structs. This can be a very meaningful improvement for structs with a large number of fields. |
No, this is entirely orthogonal.
If we follow your position, other implementations will be exposed to such data. So they will be affected anyway.
What do you mean with "skip producing empty values"? We need to initialize memory anyway. Whether it's by a single |
All I'm saying is that we be liberal in data that we receive (relaxed checks). We can still be strict in Starting a whole specification refinement discussion takes much more time than I thought would be required from me to fix this issue. If the relaxed checks is not an acceptable solution, I can close the PR and move on. |
It's not only "data we receive" here, but "data we produce" ( |
@alippai on the Arrow->Parquet or on the Parquet->Arrow path? |
This was list specific and for the parquet->arrow path: #34510 I’ve read this issue again and as I understand both of the solutions would serialize into exactly the same parquet? Not sure which version the parsing will yield |
The issue there seems to be the complexity of the Parquet layout and the overhead of zero-initializing in-memory arrays, although non-zero, is not yet highlighted as an issue. |
Closing this in favor of #38252 |
Rationale for this change
The layout constraint checks in
GetArrayView
are too strict when they don't allow non-nullable STRUCT fields to be null since STRUCT fields can legally have any value when the parentArray
describes the entry as not valid.Reproduction script
What changes are included in this PR?
Allow the execution of
GetArrayView
with nulls in non-nullable fields iff the parentArray
is 100% null. To cover all cases, the checks would have to be done value by value, but that would be expensive. This PR covers the cases where STRUCT arrays are created byMakeArrayOfNull
. Kernels producing STRUCTs with a few nulls might be wiser to set validity bits to 1 on non-nullable fields even when the parent is null.Are these changes tested?
By existing tests and manual reproduction script.