Skip to content

Commit e980b20

Browse files
TheNeuralBitwesm
authored andcommitted
ARROW-5714: [JS] Inconsistent behavior in Int64Builder with/without BigNum
- Use stride in `DataBufferBuilder.set` to ensure that Int64Builder is consistent with and without BigNum. - Use `WideBufferBuilder` for `Int64` and `Uint64` builders, even when `BigInt` is not available. Moves check for `BigInt` availability into `WideBufferBuilder`. (Thanks to Paul Taylor) Author: Brian Hulette <[email protected]> Author: ptaylor <[email protected]> Closes apache#4691 from TheNeuralBit/js-data-buffer-builder-stride and squashes the following commits: 7862612 <Brian Hulette> Add clarifying comment for int64 generation in builder tests b08ac1b <Brian Hulette> Merge pull request apache#3 from trxcllnt/js/data-buffer-builder-64bit-stride 2e4ce7a <Brian Hulette> Use stride in DataBufferBuilder.set 4567d07 <ptaylor> use WideBufferBuilder for Int64 and Uint64 builders dc9ed3a <ptaylor> update package-lock.json
1 parent eb57fff commit e980b20

File tree

4 files changed

+43
-28
lines changed

4 files changed

+43
-28
lines changed

js/package-lock.json

+4-4
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

js/src/builder/buffer.ts

+10-12
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,12 @@
1616
// under the License.
1717

1818
import { memcpy } from '../util/buffer';
19-
import { BigInt64Array, BigUint64Array } from '../util/compat';
19+
import { BigIntAvailable, BigInt64Array, BigUint64Array } from '../util/compat';
2020
import {
2121
TypedArray, TypedArrayConstructor,
2222
BigIntArray, BigIntArrayConstructor
2323
} from '../interfaces';
2424

25-
/** @ignore */ type WideArray<T extends BigIntArray> = T extends BigIntArray ? Int32Array : Uint32Array;
2625
/** @ignore */ type DataValue<T> = T extends TypedArray ? number : T extends BigIntArray ? WideValue<T> : T;
2726
/** @ignore */ type WideValue<T extends BigIntArray> = T extends BigIntArray ? bigint | Int32Array | Uint32Array : never;
2827
/** @ignore */ type ArrayCtor<T extends TypedArray | BigIntArray> =
@@ -105,7 +104,7 @@ export class DataBufferBuilder<T extends TypedArray> extends BufferBuilder<T, nu
105104
public get(index: number) { return this.buffer[index]; }
106105
public set(index: number, value: number) {
107106
this.reserve(index - this.length + 1);
108-
this.buffer[index] = value;
107+
this.buffer[index * this.stride] = value;
109108
return this;
110109
}
111110
}
@@ -157,16 +156,13 @@ export class OffsetsBufferBuilder extends DataBufferBuilder<Int32Array> {
157156
}
158157

159158
/** @ignore */
160-
export class WideBufferBuilder<T extends BigIntArray> extends BufferBuilder<WideArray<T>, DataValue<T>> {
159+
export class WideBufferBuilder<T extends TypedArray, R extends BigIntArray> extends BufferBuilder<T, DataValue<T>> {
161160
// @ts-ignore
162-
public buffer64: T;
161+
public buffer64: R;
163162
// @ts-ignore
164-
constructor(buffer: T, stride: number) {
165-
const ArrayType = buffer instanceof BigInt64Array ? Int32Array : Uint32Array;
166-
super(new ArrayType(buffer.buffer, buffer.byteOffset, buffer.byteLength / 4) as WideArray<T>, stride);
167-
}
168-
public get ArrayType64(): BigIntArrayConstructor<T> {
169-
return this.buffer instanceof Int32Array ? BigInt64Array : BigUint64Array as any;
163+
protected _ArrayType64: BigIntArrayConstructor<R>;
164+
public get ArrayType64() {
165+
return this._ArrayType64 || (this._ArrayType64 = <BigIntArrayConstructor<R>> (this.buffer instanceof Int32Array ? BigInt64Array : BigUint64Array));
170166
}
171167
public set(index: number, value: DataValue<T>) {
172168
this.reserve(index - this.length + 1);
@@ -180,7 +176,9 @@ export class WideBufferBuilder<T extends BigIntArray> extends BufferBuilder<Wide
180176
protected _resize(newLength: number) {
181177
const data = super._resize(newLength);
182178
const length = data.byteLength / (this.BYTES_PER_ELEMENT * this.stride);
183-
this.buffer64 = new this.ArrayType64(data.buffer, data.byteOffset, length);
179+
if (BigIntAvailable) {
180+
this.buffer64 = new this.ArrayType64(data.buffer, data.byteOffset, length);
181+
}
184182
return data;
185183
}
186184
}

js/src/builder/int.ts

+7-10
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,8 @@
1717

1818
import { bignumToBigInt } from '../util/bn';
1919
import { WideBufferBuilder } from './buffer';
20+
import { BigInt64Array } from '../util/compat';
2021
import { FixedWidthBuilder, BuilderOptions } from '../builder';
21-
import { BigInt64ArrayAvailable, BigInt64Array } from '../util/compat';
22-
import { BigUint64ArrayAvailable, BigUint64Array } from '../util/compat';
2322
import { Int, Int8, Int16, Int32, Int64, Uint8, Uint16, Uint32, Uint64 } from '../type';
2423

2524
/** @ignore */
@@ -37,16 +36,15 @@ export class Int16Builder<TNull = any> extends IntBuilder<Int16, TNull> {}
3736
export class Int32Builder<TNull = any> extends IntBuilder<Int32, TNull> {}
3837
/** @ignore */
3938
export class Int64Builder<TNull = any> extends IntBuilder<Int64, TNull> {
39+
protected _values: WideBufferBuilder<Int32Array, BigInt64Array>;
4040
constructor(options: BuilderOptions<Int64, TNull>) {
4141
if (options['nullValues']) {
4242
options['nullValues'] = (options['nullValues'] as TNull[]).map(toBigInt);
4343
}
4444
super(options);
45-
if (BigInt64ArrayAvailable) {
46-
this._values = <any> new WideBufferBuilder(new BigInt64Array(0), 2);
47-
}
45+
this._values = new WideBufferBuilder(new Int32Array(0), 2);
4846
}
49-
public get values64() { return (this._values as any).buffer64 as BigInt64Array; }
47+
public get values64() { return this._values.buffer64; }
5048
public isValid(value: Int32Array | bigint | TNull) { return super.isValid(toBigInt(value)); }
5149
}
5250

@@ -58,16 +56,15 @@ export class Uint16Builder<TNull = any> extends IntBuilder<Uint16, TNull> {}
5856
export class Uint32Builder<TNull = any> extends IntBuilder<Uint32, TNull> {}
5957
/** @ignore */
6058
export class Uint64Builder<TNull = any> extends IntBuilder<Uint64, TNull> {
59+
protected _values: WideBufferBuilder<Uint32Array, BigUint64Array>;
6160
constructor(options: BuilderOptions<Uint64, TNull>) {
6261
if (options['nullValues']) {
6362
options['nullValues'] = (options['nullValues'] as TNull[]).map(toBigInt);
6463
}
6564
super(options);
66-
if (BigUint64ArrayAvailable) {
67-
this._values = <any> new WideBufferBuilder(new BigUint64Array(0), 2);
68-
}
65+
this._values = new WideBufferBuilder(new Uint32Array(0), 2);
6966
}
70-
public get values64() { return (this._values as any).buffer64 as BigUint64Array; }
67+
public get values64() { return this._values.buffer64; }
7168
public isValid(value: Uint32Array | bigint | TNull) { return super.isValid(toBigInt(value)); }
7269
}
7370

js/test/unit/builders/utils.ts

+22-2
Original file line numberDiff line numberDiff line change
@@ -53,15 +53,35 @@ export const int16sNoNulls = (length = 20) => Array.from(new Int16Array(randomBy
5353
export const int32sNoNulls = (length = 20) => Array.from(new Int32Array(randomBytes(length * Int32Array.BYTES_PER_ELEMENT).buffer));
5454
export const int64sNoNulls = (length = 20) => Array.from({ length }, (_, i) => {
5555
const bn = util.BN.new(new Int32Array(randomBytes(2 * 4).buffer));
56-
return i % 3 === 0 ? bn : i % 2 === 0 ? bn[Symbol.toPrimitive]() : bn[0];
56+
// Evenly distribute the three types of arguments we support in the Int64
57+
// builder
58+
switch (i % 3) {
59+
// Int32Array (util.BN is-a Int32Array)
60+
case 0: return bn;
61+
// BigInt
62+
case 1: return bn[Symbol.toPrimitive]()
63+
// number
64+
case 2:
65+
default: return bn[0];
66+
}
5767
});
5868

5969
export const uint8sNoNulls = (length = 20) => Array.from(new Uint8Array(randomBytes(length * Uint8Array.BYTES_PER_ELEMENT).buffer));
6070
export const uint16sNoNulls = (length = 20) => Array.from(new Uint16Array(randomBytes(length * Uint16Array.BYTES_PER_ELEMENT).buffer));
6171
export const uint32sNoNulls = (length = 20) => Array.from(new Uint32Array(randomBytes(length * Uint32Array.BYTES_PER_ELEMENT).buffer));
6272
export const uint64sNoNulls = (length = 20) => Array.from({ length }, (_, i) => {
6373
const bn = util.BN.new(new Uint32Array(randomBytes(2 * 4).buffer));
64-
return i % 3 === 0 ? bn : i % 2 === 0 ? bn[Symbol.toPrimitive]() : bn[0];
74+
// Evenly distribute the three types of arguments we support in the Uint64
75+
// builder
76+
switch (i % 3) {
77+
// UInt32Array (util.BN is-a Uint32Array)
78+
case 0: return bn;
79+
// BigInt
80+
case 1: return bn[Symbol.toPrimitive]()
81+
// number
82+
case 2:
83+
default: return bn[0];
84+
}
6585
});
6686
export const float16sNoNulls = (length = 20) => Array.from(new Uint16Array(randomBytes(length * Uint16Array.BYTES_PER_ELEMENT).buffer)).map((x) => (x - 32767) / 32767);
6787
export const float32sNoNulls = (length = 20) => Array.from(new Float32Array(randomBytes(length * Float32Array.BYTES_PER_ELEMENT).buffer));

0 commit comments

Comments
 (0)