GeorgeLeePatterson commented on code in PR #320:
URL: https://github.com/apache/arrow-js/pull/320#discussion_r2494421850
##########
src/data.ts:
##########
@@ -117,7 +123,16 @@ export class Data<T extends DataType = DataType> {
return nullCount;
}
- constructor(type: T, offset: number, length: number, nullCount?: number,
buffers?: Partial<Buffers<T>> | Data<T>, children: Data[] = [], dictionary?:
Vector) {
+ constructor(
+ type: T,
+ offset: number,
+ length: number,
+ nullCount?: number,
+ buffers?: Partial<Buffers<T>> | Data<T>,
+ children: Data[] = [],
+ dictionary?: Vector,
+ variadicBuffers: ReadonlyArray<Uint8Array> = []
Review Comment:
Agreed. After the dust has settled I can revisit this. Your point that
Buffers<T> doesn't map from [BufferType] => Buffer is accurate. In the case of
variadic buffers, they get spread into the wrapping structures after being
flattened, even the canonical cpp implementation does this. So the "Data"
construct itself is really just a collection of conventions and algorithms to
interpret. These underlying buffers really are first class citizens in the
"container" that carries this data. Even the VALIDITY buffer itself seems to
break an otherwise nice consistent interface over the index and a consistent
value. VariadicBuffers are even more of an outlier.
I'll make a note to revisit this and compare different implementations of
this structure and see if we can find something a bit more ergonomic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
