ARROW-271: Update Field structure to be more explicit

[julienledem]

This is a proposal. I have not updated the code depending on this yet.

[wesm]

This makes sense for explicitness -- for the name of this, BufferType?

[julienledem]

BufferType sounds good too.

[wesm]

This seems reasonable to me, and avoids issues around the flattening. It also means in very large schemas that you will not have to reconstruct the tree structure if you wish to examine the metadata in a particular node, which may have nice performance benefits.

I think when we originally drafted the flatbuffers IDL, it seemed like using the struct flatbuffers type would be more efficient / faster, but having the table indirection doesn't seem so bad for the improved flexibility.

Note also that tables permit more flexible schema evolution

[julienledem]

@wesm Yes. I'm a bit concerned of using structs since they can not be changed ever.

[jacques-n]

I'm -1 on the change from struct -> table.

I am the one who pushed hard that the core DataHeader structures should be structs with predictable layouts. One of the targets from my perspective held for the data header structure is that I can always directly locate a buffer length and offset without indirection via the data header. This is why they are in a struct. The schema negotiation gives me an ability to set up all my code with this knowledge and without having to deal with rewriting those pointers.

I'd suggest a level of indirection in the apis if you're finding the structures difficult to use rather than moving to a table based structure. The use of structs in data headers is parallel to the use of direct memory layout in the vectors themselves. I agree that this is unchangeable in a particular format version and think that is okay.

[julienledem]

@jacques-n: could you point to an example of this code?

The main thing I'm trying to achieve is a more explicit metadata for interoperability.
It is easy to produce the original flat record batch structure from the tree structure I'm proposing above.
Going back to the tree structure from the flat buffer list requires implicit knowledge of how many buffers we have for a type and in what order (possibly depending on the value count as well). Bugs in this interpretation will be hard to figure out since buffers are not labelled.

Another option:

• FieldNode and Buffer are structs. nodes and buffers fields in RecordBatch are unchanged
• add childrenCount and bufferCount to FieldNode
• add type to Buffer

[wesm]

I would be in favor of structs for both FieldNode and Buffer, but putting the buffers in the FieldNode. Structs cannot be used recursively. Thoughts?

[julienledem]

@wesm yes. Using structures requires to have flattened lists for nodes and buffers. adding childrenCount and bufferCount in FieldNode makes it explicit which buffers belong to which fieldNode. the type in Buffer makes it explicit what each buffer is for.

[jacques-n]

A RecordBatch relies on the Schema structure explicitly. It cannot be consumed without one. The children/bufferCount values are all already implicitly defined by the nature of the schema structure. I don't think it necessary to store this explicitly. I also think it is unnecessary to duplicate that data in the RecordBatch structure. These values would be exactly the same for every RecordBatch corresponding to a particular Schema. If you concerned about validity and interoperability, it seems that this a simple validation utility could be written in each of C++ and Java. Given a particular schema, this utility could tell you the associated children count and buffer count expected. Perhaps I'm not fully understanding the technical problem?

[jacques-n]

@julienledem: I'm not exactly sure what code you're asking me to point you to.

[julienledem]

@jacques-n I was referring to: "The schema negotiation gives me an ability to set up all my code with this knowledge and without having to deal with rewriting those pointers."

Yes the children/bufferCount is already known implicitly. However it is not possible to validate against all errors (buffers in wrong order for example). Adding a little more information would go a long way to pinpointing exactly what's wrong when validation fails (instead of "wrong buffer count"). It is not necessary but it is very useful.

[jacques-n]

Just to clarify, the reason we chose to use flatbuf was to avoid extra indirection in the RecordBatch structure (and specifically fieldnode and buffer vectors). Without this, we would have chosen protobuf or another less memory-structured (but more common) standard. I continue to think that is important to maintain each of those structures as a struct rather than a table.

Making things less error-prone and more explicit is always a good goal. Your proposal comes at the cost of additional memory overhead. In high performance computing, I think we have to weight the value of that redundancy against the types of errors we'll be able to detect. I also continue to think that a good set of construction validation tools and APIs in C++ and Java will lead to the same thing without overhead. From my perspective, safety and redundancy is the realm of the schema, not the RecordBatch.

Staying with structs, can you make your proposal more specific? It sounds like you're wanting to add an additional 12 bytes to every fieldnode (type, childcount, buffercount)? Is that correct? Are you also proposing an additional 4 byte value per buffer of type? If so, what values would this select from? What specific errors do you think this will help improve?

[julienledem]

@jacques-n: I have updated the PR to something that provides the same information but in the schema only. Each Field provides the list of buffers to be expected in each RecordBatch (and all RecordBatches look the same).
The RecordBatch is unchanged.
That should address your concerns.

[wesm]

In theory, the set of buffers associated with each field should be deterministic (with deterministic types), but having this additional metadata adds a small extra layer of validation / flexibility (for example: if the null count is 0, the validity / nullness buffer metadata could be omitted). I am okay with either solution.

[julienledem]

@wesm With the vector info in the schema, the number of vectors has to be the same for every RowBatch. Since null count is in the RowBatch, we still need to have a buffer if it is all null (or at least metadata for it). However this Buffer can be empty (size = 0). Technically it is the same but just represented differently in the metadata.

[jacques-n]

Looks good to me +1

[wesm]

Will merge after rebase

[julienledem]

As I was using this in the File format I realized that there's one thing missing which is the bit_width of a slot in a buffer.
I updated accordingly. With that the physical layout is fully explicit.

[julienledem]

rebased

[wesm]

Could you explain why the bit_width is needed (the buffer lengths are indicated already in bytes, but this doesn't help you with the size of the slots)? I suppose otherwise you need to use the logical types specification to know the physical size of the slots.

[jacques-n]

What does "does not include children" mean? I would expect this to list all buffers for a batch.

[julienledem]

This is Field which is part of the Schema (FieldNode and not Field will contain all the Buffers for a RecordBatch).
Each Field has a vector layout and children. Each child define its own layout.
For example:

• a Tuple only has a validity vector (children define their own).
• a List has a validity vector and an offset vector (and a single child)
• an int has a validity vector and a data vector (and no children)

[julienledem]

@wesm yes. In some case the bit_width is obvious (ints), in others it is not (dictionary id width. Decimal width?)

[wesm]

I see. In theory the physical size would be a deterministic function of the metadata, but this may not be guaranteed in the implementation (for example, if the dictionary ids were int8_t -- though I wonder whether we should make the id storage more explicit in the metadata).

For decimals, is it true that the physical representation may not be consistent across systems for a given precision and scale?

In any case, I'm okay with having the bit width in the buffer metadata

[wesm]

I'm satisfied with the solution of putting the vector/buffer layout in the schema. @jacques-n?

[wesm]

Where are we at now that ARROW-264 is merged?

[julienledem]

@wesm Here is a rebased PR (since part of this was used in ARROW-264).

[wesm]

+1