From 8ddce0a1dc8a945744dc0e9b65e36f88845b3cb6 Mon Sep 17 00:00:00 2001 From: Paul Taylor Date: Wed, 24 Jan 2018 20:44:09 -0800 Subject: [PATCH] check bounds in getChildAt(i) to avoid NPEs --- js/src/predicate.ts | 2 +- js/src/recordbatch.ts | 21 ++++++++---------- js/src/table.ts | 15 +++++++------ js/src/vector.ts | 4 ++-- js/src/vector/nested.ts | 48 ++++++++++++++++++++++++++++------------- 5 files changed, 53 insertions(+), 37 deletions(-) diff --git a/js/src/predicate.ts b/js/src/predicate.ts index 00aa639663619..ab327ea9d72b8 100644 --- a/js/src/predicate.ts +++ b/js/src/predicate.ts @@ -61,7 +61,7 @@ export class Col extends Value { } if (this.colidx < 0) { throw new Error(`Failed to bind Col "${this.name}"`); } } - this.vector = batch.getChildAt(this.colidx); + this.vector = batch.getChildAt(this.colidx)!; return this.vector.get.bind(this.vector); } diff --git a/js/src/recordbatch.ts b/js/src/recordbatch.ts index 19bd145b15b4d..07d94a9d49629 100644 --- a/js/src/recordbatch.ts +++ b/js/src/recordbatch.ts @@ -15,7 +15,7 @@ // specific language governing permissions and limitations // under the License. -import { Schema, Struct } from './type'; +import { Schema, Struct, DataType } from './type'; import { flatbuffers } from 'flatbuffers'; import { View, Vector, StructVector } from './vector'; import { Data, NestedData } from './data'; @@ -40,34 +40,31 @@ export class RecordBatch extends StructVector { super(data, args[2]); this.schema = args[0]; this.length = data.length; - this.numCols = this.schema.fields.length; } else { const [schema, numRows, cols] = args; - const columns: Vector[] = new Array(cols.length); - const columnsData: Data[] = new Array(cols.length); + const childData: Data[] = new Array(cols.length); for (let index = -1, length = cols.length; ++index < length;) { const col: Data | Vector = cols[index]; - if (col instanceof Vector) { - columnsData[index] = (columns[index] = col as Vector).data; - } else { - columns[index] = Vector.create(columnsData[index] = col); - } + childData[index] = col instanceof Vector ? col.data : col; } - super(new NestedData(new Struct(schema.fields), numRows, null, columnsData)); + super(new NestedData(new Struct(schema.fields), numRows, null, childData)); this.schema = schema; this.length = numRows; - this.numCols = schema.fields.length; } + this.numCols = this.schema.fields.length; } public clone(data: Data, view: View = this.view.clone(data)): this { return new RecordBatch(this.schema, data as any, view) as any; } + public getChildAt(index: number): Vector | null { + return index < 0 || index >= this.numCols ? null : super.getChildAt(index); + } public select(...columnNames: string[]) { const fields = this.schema.fields; const namesToKeep = columnNames.reduce((xs, x) => (xs[x] = true) && xs, Object.create(null)); return new RecordBatch( this.schema.select(...columnNames), this.length, - this.childData.filter((_, index) => namesToKeep[fields[index].name]) + this.childData.filter((_, i) => namesToKeep[fields[i].name]) ); } } diff --git a/js/src/table.ts b/js/src/table.ts index 33899c21665b7..1c874ee26ef82 100644 --- a/js/src/table.ts +++ b/js/src/table.ts @@ -65,9 +65,9 @@ export class Table implements DataFrame { static fromStruct(struct: StructVector) { const schema = new Schema(struct.type.children); const chunks = struct.view instanceof ChunkedView ? - (struct.view.childVectors as StructVector[]) : + (struct.view.chunkVectors as StructVector[]) : [struct]; - return new Table(chunks.map((chunk)=>new RecordBatch(schema, chunk.length, chunk.view.childData))); + return new Table(chunks.map((chunk) => new RecordBatch(schema, chunk.length, chunk.view.childData))); } public readonly schema: Schema; @@ -102,7 +102,6 @@ export class Table implements DataFrame { this.schema = schema; this.batches = batches; this.batchesUnion = batches.reduce((union, batch) => union.concat(batch)); - // this.columns = schema.fields.map((_, i) => this.batchesUnion.getChildAt(i)); this.length = this.batchesUnion.length; this.numCols = this.batchesUnion.numCols; } @@ -113,8 +112,10 @@ export class Table implements DataFrame { return this.getColumnAt(this.getColumnIndex(name)); } public getColumnAt(index: number) { - return this._columns[index] || ( - this._columns[index] = this.batchesUnion.getChildAt(index)); + return index < 0 || index >= this.numCols + ? null + : this._columns[index] || ( + this._columns[index] = this.batchesUnion.getChildAt(index)!); } public getColumnIndex(name: string) { return this.schema.fields.findIndex((f) => f.name === name); @@ -271,8 +272,8 @@ export class CountByResult extends Table implements DataFrame { )); } public toJSON(): Object { - const values = this.getColumnAt(0); - const counts = this.getColumnAt(1); + const values = this.getColumnAt(0)!; + const counts = this.getColumnAt(1)!; const result = {} as { [k: string]: number | null }; for (let i = -1; ++i < this.length;) { result[values.get(i)] = counts.get(i); diff --git a/js/src/vector.ts b/js/src/vector.ts index 34d1fb0023b4f..d9ca97b5fd120 100644 --- a/js/src/vector.ts +++ b/js/src/vector.ts @@ -35,8 +35,8 @@ export class Vector implements VectorLike, View, Vi public static create(data: Data): Vector { return createVector(data); } - public static concat(...sources: Vector[]): Vector { - return sources.length === 1 ? sources[0] : sources.reduce((a, b) => a.concat(b)); + public static concat(source?: Vector | null, ...others: Vector[]): Vector { + return others.reduce((a, b) => a ? a.concat(b) : b, source!); } public type: T; public length: number; diff --git a/js/src/vector/nested.ts b/js/src/vector/nested.ts index 78e2a988e6d5d..d0fb24ca99682 100644 --- a/js/src/vector/nested.ts +++ b/js/src/vector/nested.ts @@ -24,15 +24,15 @@ export abstract class NestedView implements View { public length: number; public numChildren: number; public childData: Data[]; - protected _childColumns: Vector[]; + protected _children: Vector[]; constructor(data: Data, children?: Vector[]) { this.length = data.length; this.childData = data.childData; this.numChildren = data.childData.length; - this._childColumns = children || new Array(this.numChildren); + this._children = children || new Array(this.numChildren); } public clone(data: Data): this { - return new ( this.constructor)(data, this._childColumns) as this; + return new ( this.constructor)(data, this._children) as this; } public isValid(): boolean { return true; @@ -52,9 +52,11 @@ export abstract class NestedView implements View { } protected abstract getNested(self: NestedView, index: number): T['TValue']; protected abstract setNested(self: NestedView, index: number, value: T['TValue']): void; - public getChildAt(index: number) { - return this._childColumns[index] || ( - this._childColumns[index] = Vector.create(this.childData[index])); + public getChildAt(index: number): Vector | null { + return index < 0 || index >= this.numChildren + ? null + : (this._children[index] as Vector) || + (this._children[index] = Vector.create(this.childData[index])); } public *[Symbol.iterator](): IterableIterator { const get = this.getNested; @@ -120,14 +122,22 @@ export class DenseUnionView extends UnionView { export class StructView extends NestedView { protected getNested(self: StructView, index: number) { - return new RowView(self as any, self._childColumns, index); + return new RowView(self as any, self._children, index); } protected setNested(self: StructView, index: number, value: any): void { - let idx = -1, len = self.numChildren; + let idx = -1, len = self.numChildren, child: Vector | null; if (!(value instanceof NestedView || value instanceof Vector)) { - while (++idx < len) { self.getChildAt(idx).set(index, value[idx]); } + while (++idx < len) { + if (child = self.getChildAt(idx)) { + child.set(index, value[idx]); + } + } } else { - while (++idx < len) { self.getChildAt(idx).set(index, value.get(idx)); } + while (++idx < len) { + if (child = self.getChildAt(idx)) { + child.set(index, value.get(idx)); + } + } } } } @@ -140,14 +150,22 @@ export class MapView extends NestedView { (xs[x.name] = i) && xs || xs, Object.create(null)); } protected getNested(self: MapView, index: number) { - return new MapRowView(self as any, self._childColumns, index); + return new MapRowView(self as any, self._children, index); } protected setNested(self: MapView, index: number, value: { [k: string]: any }): void { - const typeIds = self.typeIds as any; + let typeIds = self.typeIds as any, child: Vector | null; if (!(value instanceof NestedView || value instanceof Vector)) { - for (const key in typeIds) { self.getChildAt(typeIds[key]).set(index, value[key]); } + for (const key in typeIds) { + if (child = self.getChildAt(typeIds[key])) { + child.set(index, value[key]); + } + } } else { - for (const key in typeIds) { self.getChildAt(typeIds[key]).set(index, value.get(key as any)); } + for (const key in typeIds) { + if (child = self.getChildAt(typeIds[key])) { + child.set(index, value.get(key as any)); + } + } } } } @@ -160,7 +178,7 @@ export class RowView extends UnionView { this.length = data.numChildren; } public clone(data: Data & NestedView): this { - return new ( this.constructor)(data, this._childColumns, this.rowIndex) as this; + return new ( this.constructor)(data, this._children, this.rowIndex) as this; } protected getChildValue(self: RowView, index: number, _typeIds: any, _valueOffsets?: any): any | null { const child = self.getChildAt(index);