GeorgeLeePatterson commented on code in PR #320:
URL: https://github.com/apache/arrow-js/pull/320#discussion_r2487215746
##########
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:
I thought similarly. At first I thought introducing some sort of builder
pattern might be a good approach, but that only made sense in the case where I
would absorb other args as well and it would sort of duplicate some of
`makeData`'s semantics, then I thought perhaps a setter for this only, but that
felt unconventional, so I landed on a simple extra arg.
I'm open to changing this, but I would need some direction in terms of what
args to absorb into an options object.
For reference, the Rust implementation sorts of splits it out by introducing
a "DataLayout" structure that works in tandem with the Data structure in
various methods. I'm not suggesting we need to introduce that, but it might be
useful as a consult.
--
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]
