From fb7a0f041542ab462d9df5f14e07c66ea2e2bc79 Mon Sep 17 00:00:00 2001 From: Brian Hulette Date: Sun, 10 Feb 2019 11:09:57 -0800 Subject: [PATCH 1/6] add row proxy benchmark --- js/perf/index.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/js/perf/index.js b/js/perf/index.js index 0e9c2bd689a..2129b9d983d 100644 --- a/js/perf/index.js +++ b/js/perf/index.js @@ -44,10 +44,12 @@ for (let { name, buffers } of require('./table_config')) { for (let {name, buffers, countBys, counts} of require('./table_config')) { const table = Table.from(buffers); + const tableIterateSuiteName = `Table Iterate "${name}"`; const dfCountBySuiteName = `DataFrame Count By "${name}"`; const dfFilterCountSuiteName = `DataFrame Filter-Scan Count "${name}"`; const dfDirectCountSuiteName = `DataFrame Direct Count "${name}"`; + suites.push(createTestSuite(tableIterateSuiteName, createTableIterateTest(table))); suites.push(...countBys.map((countBy) => createTestSuite(dfCountBySuiteName, createDataFrameCountByTest(table, countBy)))); suites.push(...counts.map(({ col, test, value }) => createTestSuite(dfFilterCountSuiteName, createDataFrameFilterCountTest(table, col, test, value)))); suites.push(...counts.map(({ col, test, value }) => createTestSuite(dfDirectCountSuiteName, createDataFrameDirectCountTest(table, col, test, value)))); @@ -135,6 +137,15 @@ function createGetByIndexTest(vector, name) { }; } +function createTableIterateTest(table) { + let value; + return { + async: true, + name: `length: ${table.length}\n`, + fn() { for (value of table) {} } + }; +} + function createDataFrameDirectCountTest(table, column, test, value) { let sum, colidx = table.schema.fields.findIndex((c)=>c.name === column); From 8a5d16293b653ed5a5bf54001960211a56c47771 Mon Sep 17 00:00:00 2001 From: Brian Hulette Date: Sun, 10 Feb 2019 11:10:29 -0800 Subject: [PATCH 2/6] Improve Row proxy performance new a prototype rather than using \`Object.create\` --- js/src/util/vector.ts | 19 ++++++- js/src/vector/map.ts | 8 +-- js/src/vector/row.ts | 106 ++++++++++++++++++++++------------------ js/src/vector/struct.ts | 8 +-- 4 files changed, 84 insertions(+), 57 deletions(-) diff --git a/js/src/util/vector.ts b/js/src/util/vector.ts index 92f348dd3f0..747ba8edafe 100644 --- a/js/src/util/vector.ts +++ b/js/src/util/vector.ts @@ -96,8 +96,8 @@ export function createElementComparator(search: any) { return true; }; } - // Compare Rows and Vectors - if ((search instanceof Row) || (search instanceof Vector)) { + // Compare Vectors + if (search instanceof Vector) { const n = search.length; const C = search.constructor as any; const fns = [] as ((x: any) => boolean)[]; @@ -113,6 +113,21 @@ export function createElementComparator(search: any) { return true; }; } + // Compare Rows + if (search instanceof Row) { + const n = search.length; + const fns = [] as ((x: any) => boolean)[]; + for (let i = -1; ++i < n;) { + fns[i] = createElementComparator((search as any).get(i)); + } + return (value: any) => { + if (!(value.length === n)) { return false; } + for (let i = -1; ++i < n;) { + if (!(fns[i](value.get(i)))) { return false; } + } + return true; + }; + } // Compare non-empty Objects const keys = Object.keys(search); if (keys.length > 0) { diff --git a/js/src/vector/map.ts b/js/src/vector/map.ts index 27c51a62270..55a195be900 100644 --- a/js/src/vector/map.ts +++ b/js/src/vector/map.ts @@ -15,7 +15,7 @@ // specific language governing permissions and limitations // under the License. -import { Row } from './row'; +import { RowProxyGenerator } from './row'; import { Vector } from '../vector'; import { BaseVector } from './base'; import { DataType, Map_, Struct } from '../type'; @@ -25,8 +25,8 @@ export class MapVector extends Base return Vector.new(this.data.clone(new Struct(this.type.children))); } // @ts-ignore - private _rowProxy: Row; - public get rowProxy(): Row { - return this._rowProxy || (this._rowProxy = Row.new(this.type.children || [], true)); + private _rowProxy: RowProxyGenerator; + public get rowProxy(): RowProxyGenerator { + return this._rowProxy || (this._rowProxy = RowProxyGenerator.new(this.type.children || [], true)); } } diff --git a/js/src/vector/row.ts b/js/src/vector/row.ts index 62fb3b608c2..5c12c77b844 100644 --- a/js/src/vector/row.ts +++ b/js/src/vector/row.ts @@ -17,21 +17,56 @@ import { Field } from '../schema'; import { MapVector } from '../vector/map'; -import { DataType, RowLike } from '../type'; +import { DataType } from '../type'; import { valueToString } from '../util/pretty'; import { StructVector } from '../vector/struct'; /** @ignore */ const columnDescriptor = { enumerable: true, configurable: false, get: () => {} }; /** @ignore */ const lengthDescriptor = { writable: false, enumerable: false, configurable: false, value: -1 }; -/** @ignore */ const rowIndexDescriptor = { writable: false, enumerable: false, configurable: true, value: null as any }; -/** @ignore */ const rowParentDescriptor = { writable: false, enumerable: false, configurable: false, value: null as any }; -/** @ignore */ const row = { parent: rowParentDescriptor, rowIndex: rowIndexDescriptor }; -/** @ignore */ export class Row implements Iterable { [key: string]: T[keyof T]['TValue']; + // @ts-ignore + public parent: MapVector | StructVector; + // @ts-ignore + public rowIndex: number; + // @ts-ignore + public readonly length: number; + constructor(parent: MapVector | StructVector, rowIndex: number) { + this.parent = parent; + this.rowIndex = rowIndex; + } + *[Symbol.iterator]() { + for (let i = -1, n = this.length; ++i < n;) { + yield this[i]; + } + } + public get(key: K) { return (this as any)[key] as T[K]['TValue']; } + public toJSON(): any { + return DataType.isStruct(this.parent.type) ? [...this] : + Object.getOwnPropertyNames(this).reduce((props: any, prop: string) => { + return (props[prop] = (this as any)[prop]) && props || props; + }, {}); + } + public toString() { + return DataType.isStruct(this.parent.type) ? + [...this].map((x) => valueToString(x)).join(', ') : + Object.getOwnPropertyNames(this).reduce((props: any, prop: string) => { + return (props[prop] = valueToString((this as any)[prop])) && props || props; + }, {}); + } +} + +interface RowConstructor { + readonly prototype: Row; + new(parent: MapVector | StructVector, rowIndex: number): T & Row +} + + +/** @ignore */ +export class RowProxyGenerator { /** @nocollapse */ - public static new(schemaOrFields: T | Field[], fieldsAreEnumerable = false): RowLike & Row { + public static new(schemaOrFields: T | Field[], fieldsAreEnumerable = false): RowProxyGenerator { let schema: T, fields: Field[]; if (Array.isArray(schemaOrFields)) { fields = schemaOrFields; @@ -40,61 +75,38 @@ export class Row implements Iterable new Field(x, schema[x])); } - return new Row(fields, fieldsAreEnumerable) as RowLike & Row; + return new RowProxyGenerator(fields, fieldsAreEnumerable); } - // @ts-ignore - private parent: TParent; - // @ts-ignore - private rowIndex: number; - // @ts-ignore - public readonly length: number; + + private RowProxy: RowConstructor; + private constructor(fields: Field[], fieldsAreEnumerable: boolean) { + class BoundRow extends Row {} + + const proto = BoundRow.prototype; + lengthDescriptor.value = fields.length; - Object.defineProperty(this, 'length', lengthDescriptor); + Object.defineProperty(proto, 'length', lengthDescriptor); fields.forEach((field, columnIndex) => { - columnDescriptor.get = this._bindGetter(columnIndex); + columnDescriptor.get = function() { + const child = (this as any as Row).parent.getChildAt(columnIndex); + return child ? child.get((this as any as Row).rowIndex) : null; + } // set configurable to true to ensure Object.defineProperty // doesn't throw in the case of duplicate column names columnDescriptor.configurable = true; columnDescriptor.enumerable = fieldsAreEnumerable; - Object.defineProperty(this, field.name, columnDescriptor); + Object.defineProperty(proto, field.name, columnDescriptor); columnDescriptor.configurable = false; columnDescriptor.enumerable = !fieldsAreEnumerable; - Object.defineProperty(this, columnIndex, columnDescriptor); + Object.defineProperty(proto, columnIndex, columnDescriptor); columnDescriptor.get = null as any; }); - } - *[Symbol.iterator](this: RowLike) { - for (let i = -1, n = this.length; ++i < n;) { - yield this[i]; - } - } - private _bindGetter(colIndex: number) { - return function (this: Row) { - let child = this.parent.getChildAt(colIndex); - return child ? child.get(this.rowIndex) : null; - }; + + this.RowProxy = (BoundRow as any) } public get(key: K) { return (this as any)[key] as T[K]['TValue']; } public bind | StructVector>(parent: TParent, rowIndex: number) { - rowIndexDescriptor.value = rowIndex; - rowParentDescriptor.value = parent; - const bound = Object.create(this, row); - rowIndexDescriptor.value = null; - rowParentDescriptor.value = null; - return bound as RowLike; - } - public toJSON(): any { - return DataType.isStruct(this.parent.type) ? [...this] : - Object.getOwnPropertyNames(this).reduce((props: any, prop: string) => { - return (props[prop] = (this as any)[prop]) && props || props; - }, {}); - } - public toString() { - return DataType.isStruct(this.parent.type) ? - [...this].map((x) => valueToString(x)).join(', ') : - Object.getOwnPropertyNames(this).reduce((props: any, prop: string) => { - return (props[prop] = valueToString((this as any)[prop])) && props || props; - }, {}); + return new this.RowProxy(parent, rowIndex); } } diff --git a/js/src/vector/struct.ts b/js/src/vector/struct.ts index 4ad57ff5113..1d4b73f7c0e 100644 --- a/js/src/vector/struct.ts +++ b/js/src/vector/struct.ts @@ -15,7 +15,7 @@ // specific language governing permissions and limitations // under the License. -import { Row } from './row'; +import { RowProxyGenerator } from './row'; import { Vector } from '../vector'; import { BaseVector } from './base'; import { DataType, Map_, Struct } from '../type'; @@ -25,8 +25,8 @@ export class StructVector extends B return Vector.new(this.data.clone(new Map_(this.type.children, keysSorted))); } // @ts-ignore - private _rowProxy: Row; - public get rowProxy(): Row { - return this._rowProxy || (this._rowProxy = Row.new(this.type.children || [], false)); + private _rowProxy: RowProxyGenerator; + public get rowProxy(): RowProxyGenerator { + return this._rowProxy || (this._rowProxy = RowProxyGenerator.new(this.type.children || [], false)); } } From da4d97c2d1ec39900e5ab6428535df734f36c33a Mon Sep 17 00:00:00 2001 From: Brian Hulette Date: Mon, 11 Feb 2019 07:56:36 -0800 Subject: [PATCH 3/6] Switch back to Object.create with no extra property descriptor --- js/src/vector/map.ts | 2 +- js/src/vector/row.ts | 22 +++++++++++++--------- js/src/vector/struct.ts | 2 +- js/src/visitor/get.ts | 2 +- 4 files changed, 16 insertions(+), 12 deletions(-) diff --git a/js/src/vector/map.ts b/js/src/vector/map.ts index 55a195be900..54956d7ba41 100644 --- a/js/src/vector/map.ts +++ b/js/src/vector/map.ts @@ -27,6 +27,6 @@ export class MapVector extends Base // @ts-ignore private _rowProxy: RowProxyGenerator; public get rowProxy(): RowProxyGenerator { - return this._rowProxy || (this._rowProxy = RowProxyGenerator.new(this.type.children || [], true)); + return this._rowProxy || (this._rowProxy = RowProxyGenerator.new(this, this.type.children || [], true)); } } diff --git a/js/src/vector/row.ts b/js/src/vector/row.ts index 5c12c77b844..e8c21a1c306 100644 --- a/js/src/vector/row.ts +++ b/js/src/vector/row.ts @@ -23,6 +23,7 @@ import { StructVector } from '../vector/struct'; /** @ignore */ const columnDescriptor = { enumerable: true, configurable: false, get: () => {} }; /** @ignore */ const lengthDescriptor = { writable: false, enumerable: false, configurable: false, value: -1 }; +/** @ignore */ const rowParentDescriptor = { writable: false, enumerable: false, configurable: false, value: null as any }; export class Row implements Iterable { [key: string]: T[keyof T]['TValue']; @@ -32,8 +33,7 @@ export class Row implements Iterable | StructVector, rowIndex: number) { - this.parent = parent; + constructor(rowIndex: number) { this.rowIndex = rowIndex; } *[Symbol.iterator]() { @@ -59,14 +59,14 @@ export class Row implements Iterable { readonly prototype: Row; - new(parent: MapVector | StructVector, rowIndex: number): T & Row + new(rowIndex: number): T & Row } /** @ignore */ export class RowProxyGenerator { /** @nocollapse */ - public static new(schemaOrFields: T | Field[], fieldsAreEnumerable = false): RowProxyGenerator { + public static new(parent: MapVector | StructVector, schemaOrFields: T | Field[], fieldsAreEnumerable = false): RowProxyGenerator { let schema: T, fields: Field[]; if (Array.isArray(schemaOrFields)) { fields = schemaOrFields; @@ -75,17 +75,19 @@ export class RowProxyGenerator { fieldsAreEnumerable = true; fields = Object.keys(schema).map((x) => new Field(x, schema[x])); } - return new RowProxyGenerator(fields, fieldsAreEnumerable); + return new RowProxyGenerator(parent, fields, fieldsAreEnumerable); } private RowProxy: RowConstructor; - private constructor(fields: Field[], fieldsAreEnumerable: boolean) { + private constructor(parent: MapVector | StructVector, fields: Field[], fieldsAreEnumerable: boolean) { class BoundRow extends Row {} const proto = BoundRow.prototype; + rowParentDescriptor.value = parent; lengthDescriptor.value = fields.length; + Object.defineProperty(proto, 'parent', rowParentDescriptor); Object.defineProperty(proto, 'length', lengthDescriptor); fields.forEach((field, columnIndex) => { columnDescriptor.get = function() { @@ -105,8 +107,10 @@ export class RowProxyGenerator { this.RowProxy = (BoundRow as any) } - public get(key: K) { return (this as any)[key] as T[K]['TValue']; } - public bind | StructVector>(parent: TParent, rowIndex: number) { - return new this.RowProxy(parent, rowIndex); + public bind(rowIndex: number) { + const bound = Object.create(this.RowProxy.prototype) + bound.rowIndex = rowIndex; + return bound; + //return new this.RowProxy(rowIndex); } } diff --git a/js/src/vector/struct.ts b/js/src/vector/struct.ts index 1d4b73f7c0e..e1596d6e1df 100644 --- a/js/src/vector/struct.ts +++ b/js/src/vector/struct.ts @@ -27,6 +27,6 @@ export class StructVector extends B // @ts-ignore private _rowProxy: RowProxyGenerator; public get rowProxy(): RowProxyGenerator { - return this._rowProxy || (this._rowProxy = RowProxyGenerator.new(this.type.children || [], false)); + return this._rowProxy || (this._rowProxy = RowProxyGenerator.new(this, this.type.children || [], false)); } } diff --git a/js/src/visitor/get.ts b/js/src/visitor/get.ts index 67909eacfce..4aa134b8fb9 100644 --- a/js/src/visitor/get.ts +++ b/js/src/visitor/get.ts @@ -211,7 +211,7 @@ const getNested = < S extends { [key: string]: DataType }, V extends Vector> | Vector> >(vector: V, index: number): V['TValue'] => { - return vector.rowProxy.bind(vector, index); + return vector.rowProxy.bind(index); }; /* istanbul ignore next */ From 4079439e1598c81581de35ea6468809212a5c96d Mon Sep 17 00:00:00 2001 From: Brian Hulette Date: Mon, 11 Feb 2019 08:40:12 -0800 Subject: [PATCH 4/6] linter fixes --- js/src/vector/row.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/js/src/vector/row.ts b/js/src/vector/row.ts index e8c21a1c306..256705a68e3 100644 --- a/js/src/vector/row.ts +++ b/js/src/vector/row.ts @@ -59,10 +59,9 @@ export class Row implements Iterable { readonly prototype: Row; - new(rowIndex: number): T & Row + new(rowIndex: number): T & Row; } - /** @ignore */ export class RowProxyGenerator { /** @nocollapse */ @@ -93,7 +92,7 @@ export class RowProxyGenerator { columnDescriptor.get = function() { const child = (this as any as Row).parent.getChildAt(columnIndex); return child ? child.get((this as any as Row).rowIndex) : null; - } + }; // set configurable to true to ensure Object.defineProperty // doesn't throw in the case of duplicate column names columnDescriptor.configurable = true; @@ -105,10 +104,10 @@ export class RowProxyGenerator { columnDescriptor.get = null as any; }); - this.RowProxy = (BoundRow as any) + this.RowProxy = (BoundRow as any); } public bind(rowIndex: number) { - const bound = Object.create(this.RowProxy.prototype) + const bound = Object.create(this.RowProxy.prototype); bound.rowIndex = rowIndex; return bound; //return new this.RowProxy(rowIndex); From 7bb6e4f215512f9f221a46a50fed29fef67f7dd3 Mon Sep 17 00:00:00 2001 From: Brian Hulette Date: Mon, 11 Feb 2019 18:53:47 -0800 Subject: [PATCH 5/6] Update comparator, switch to Symbols for internal variables in Row --- js/src/util/vector.ts | 142 +++++++++++++++++++++++++----------------- js/src/vector/row.ts | 34 +++++----- 2 files changed, 104 insertions(+), 72 deletions(-) diff --git a/js/src/util/vector.ts b/js/src/util/vector.ts index 747ba8edafe..8acf42e1222 100644 --- a/js/src/util/vector.ts +++ b/js/src/util/vector.ts @@ -16,7 +16,7 @@ // under the License. import { Vector } from '../vector'; -import { Row } from '../vector/row'; +import { Row, kLength } from '../vector/row'; import { compareArrayLike } from '../util/buffer'; /** @ignore */ @@ -75,75 +75,105 @@ export function createElementComparator(search: any) { } // Compare Array-likes if (Array.isArray(search)) { - const n = (search as any).length; - const fns = [] as ((x: any) => boolean)[]; - for (let i = -1; ++i < n;) { - fns[i] = createElementComparator((search as any)[i]); - } - return (value: any) => { - if (!value || value.length !== n) { return false; } - // Handle the case where the search element is an Array, but the - // values are Rows or Vectors, e.g. list.indexOf(['foo', 'bar']) - if ((value instanceof Row) || (value instanceof Vector)) { - for (let i = -1, n = value.length; ++i < n;) { - if (!(fns[i]((value as any).get(i)))) { return false; } - } - return true; - } - for (let i = -1, n = value.length; ++i < n;) { - if (!(fns[i](value[i]))) { return false; } - } - return true; - }; + return createArrayLikeComparator(search); + } + // Compare Rows + if (search instanceof Row) { + return createRowComparator(search); } // Compare Vectors if (search instanceof Vector) { - const n = search.length; - const C = search.constructor as any; - const fns = [] as ((x: any) => boolean)[]; - for (let i = -1; ++i < n;) { - fns[i] = createElementComparator((search as any).get(i)); - } - return (value: any) => { - if (!(value instanceof C)) { return false; } - if (!(value.length === n)) { return false; } + return createVectorComparator(search); + } + // Compare non-empty Objects + const keys = Object.keys(search); + if (keys.length > 0) { + return createObjectKeysComparator(search, keys); + } + // No valid comparator + return () => false; +} + +/** @ignore */ +function createArrayLikeComparator(search: ArrayLike) { + const n = search.length; + const fns = [] as ((x: any) => boolean)[]; + for (let i = -1; ++i < n;) { + fns[i] = createElementComparator((search as any)[i]); + } + return (value: any) => { + if (!value) { return false; } + // Handle the case where the search element is an Array, but the + // values are Rows or Vectors, e.g. list.indexOf(['foo', 'bar']) + if (value instanceof Row) { + if (value[kLength] !== n) { return false; } for (let i = -1; ++i < n;) { if (!(fns[i](value.get(i)))) { return false; } } return true; - }; - } - // Compare Rows - if (search instanceof Row) { - const n = search.length; - const fns = [] as ((x: any) => boolean)[]; - for (let i = -1; ++i < n;) { - fns[i] = createElementComparator((search as any).get(i)); } - return (value: any) => { - if (!(value.length === n)) { return false; } + if (value.length !== n) { return false; } + if (value instanceof Vector) { for (let i = -1; ++i < n;) { if (!(fns[i](value.get(i)))) { return false; } } return true; - }; + } + for (let i = -1; ++i < n;) { + if (!(fns[i](value[i]))) { return false; } + } + return true; + }; +} + +/** @ignore */ +function createRowComparator(search: Row) { + const n = search[kLength]; + const C = search.constructor as any; + const fns = [] as ((x: any) => boolean)[]; + for (let i = -1; ++i < n;) { + fns[i] = createElementComparator(search.get(i)); } - // Compare non-empty Objects - const keys = Object.keys(search); - if (keys.length > 0) { - const n = keys.length; - const fns = [] as ((x: any) => boolean)[]; + return (value: any) => { + if (!(value instanceof C)) { return false; } + if (!(value[kLength] === n)) { return false; } for (let i = -1; ++i < n;) { - fns[i] = createElementComparator(search[keys[i]]); + if (!(fns[i](value.get(i)))) { return false; } } - return (value: any) => { - if (!value || typeof value !== 'object') { return false; } - for (let i = -1; ++i < n;) { - if (!(fns[i](value[keys[i]]))) { return false; } - } - return true; - }; + return true; + }; +} + +/** @ignore */ +function createVectorComparator(search: Vector) { + const n = search.length; + const C = search.constructor as any; + const fns = [] as ((x: any) => boolean)[]; + for (let i = -1; ++i < n;) { + fns[i] = createElementComparator((search as any).get(i)); } - // No valid comparator - return () => false; + return (value: any) => { + if (!(value instanceof C)) { return false; } + if (!(value.length === n)) { return false; } + for (let i = -1; ++i < n;) { + if (!(fns[i](value.get(i)))) { return false; } + } + return true; + }; +} + +/** @ignore */ +function createObjectKeysComparator(search: any, keys: string[]) { + const n = keys.length; + const fns = [] as ((x: any) => boolean)[]; + for (let i = -1; ++i < n;) { + fns[i] = createElementComparator(search[keys[i]]); + } + return (value: any) => { + if (!value || typeof value !== 'object') { return false; } + for (let i = -1; ++i < n;) { + if (!(fns[i](value[keys[i]]))) { return false; } + } + return true; + }; } diff --git a/js/src/vector/row.ts b/js/src/vector/row.ts index 256705a68e3..e441b1b8442 100644 --- a/js/src/vector/row.ts +++ b/js/src/vector/row.ts @@ -21,35 +21,38 @@ import { DataType } from '../type'; import { valueToString } from '../util/pretty'; import { StructVector } from '../vector/struct'; +/** @ignore */ export const kLength = Symbol.for('length'); +/** @ignore */ export const kParent = Symbol.for('parent'); +/** @ignore */ export const kRowIndex = Symbol.for('rowIndex'); /** @ignore */ const columnDescriptor = { enumerable: true, configurable: false, get: () => {} }; -/** @ignore */ const lengthDescriptor = { writable: false, enumerable: false, configurable: false, value: -1 }; +/** @ignore */ const rowLengthDescriptor = { writable: false, enumerable: false, configurable: false, value: -1 }; /** @ignore */ const rowParentDescriptor = { writable: false, enumerable: false, configurable: false, value: null as any }; export class Row implements Iterable { [key: string]: T[keyof T]['TValue']; // @ts-ignore - public parent: MapVector | StructVector; + public [kParent]: MapVector | StructVector; // @ts-ignore - public rowIndex: number; + public [kRowIndex]: number; // @ts-ignore - public readonly length: number; + public readonly [kLength]: number; constructor(rowIndex: number) { - this.rowIndex = rowIndex; + this[kRowIndex] = rowIndex; } *[Symbol.iterator]() { - for (let i = -1, n = this.length; ++i < n;) { + for (let i = -1, n = this[kLength]; ++i < n;) { yield this[i]; } } public get(key: K) { return (this as any)[key] as T[K]['TValue']; } public toJSON(): any { - return DataType.isStruct(this.parent.type) ? [...this] : + return DataType.isStruct(this[kParent].type) ? [...this] : Object.getOwnPropertyNames(this).reduce((props: any, prop: string) => { return (props[prop] = (this as any)[prop]) && props || props; }, {}); } public toString() { - return DataType.isStruct(this.parent.type) ? + return DataType.isStruct(this[kParent].type) ? [...this].map((x) => valueToString(x)).join(', ') : Object.getOwnPropertyNames(this).reduce((props: any, prop: string) => { return (props[prop] = valueToString((this as any)[prop])) && props || props; @@ -81,17 +84,16 @@ export class RowProxyGenerator { private constructor(parent: MapVector | StructVector, fields: Field[], fieldsAreEnumerable: boolean) { class BoundRow extends Row {} - - const proto = BoundRow.prototype; + const proto = BoundRow.prototype rowParentDescriptor.value = parent; - lengthDescriptor.value = fields.length; - Object.defineProperty(proto, 'parent', rowParentDescriptor); - Object.defineProperty(proto, 'length', lengthDescriptor); + rowLengthDescriptor.value = fields.length; + Object.defineProperty(proto, kParent, rowParentDescriptor); + Object.defineProperty(proto, kLength, rowLengthDescriptor); fields.forEach((field, columnIndex) => { columnDescriptor.get = function() { - const child = (this as any as Row).parent.getChildAt(columnIndex); - return child ? child.get((this as any as Row).rowIndex) : null; + const child = (this as any as BoundRow)[kParent].getChildAt(columnIndex); + return child ? child.get((this as any as BoundRow)[kRowIndex]) : null; }; // set configurable to true to ensure Object.defineProperty // doesn't throw in the case of duplicate column names @@ -108,7 +110,7 @@ export class RowProxyGenerator { } public bind(rowIndex: number) { const bound = Object.create(this.RowProxy.prototype); - bound.rowIndex = rowIndex; + bound[kRowIndex] = rowIndex; return bound; //return new this.RowProxy(rowIndex); } From 24425452d7bcc4199cb41404ef18aa4c0f523a50 Mon Sep 17 00:00:00 2001 From: Brian Hulette Date: Mon, 11 Feb 2019 20:13:20 -0800 Subject: [PATCH 6/6] Remove inner class, don't re-define columns --- js/src/vector/row.ts | 48 ++++++++++++++++++++------------------------ 1 file changed, 22 insertions(+), 26 deletions(-) diff --git a/js/src/vector/row.ts b/js/src/vector/row.ts index e441b1b8442..54dcd7f61ee 100644 --- a/js/src/vector/row.ts +++ b/js/src/vector/row.ts @@ -24,7 +24,7 @@ import { StructVector } from '../vector/struct'; /** @ignore */ export const kLength = Symbol.for('length'); /** @ignore */ export const kParent = Symbol.for('parent'); /** @ignore */ export const kRowIndex = Symbol.for('rowIndex'); -/** @ignore */ const columnDescriptor = { enumerable: true, configurable: false, get: () => {} }; +/** @ignore */ const columnDescriptor = { enumerable: true, configurable: false, get: null as any }; /** @ignore */ const rowLengthDescriptor = { writable: false, enumerable: false, configurable: false, value: -1 }; /** @ignore */ const rowParentDescriptor = { writable: false, enumerable: false, configurable: false, value: null as any }; @@ -36,9 +36,6 @@ export class Row implements Iterable implements Iterable { - readonly prototype: Row; - new(rowIndex: number): T & Row; -} - /** @ignore */ export class RowProxyGenerator { /** @nocollapse */ @@ -80,36 +72,40 @@ export class RowProxyGenerator { return new RowProxyGenerator(parent, fields, fieldsAreEnumerable); } - private RowProxy: RowConstructor; + private rowPrototype: Row; private constructor(parent: MapVector | StructVector, fields: Field[], fieldsAreEnumerable: boolean) { - class BoundRow extends Row {} - const proto = BoundRow.prototype + const proto = Object.create(Row.prototype); rowParentDescriptor.value = parent; rowLengthDescriptor.value = fields.length; Object.defineProperty(proto, kParent, rowParentDescriptor); Object.defineProperty(proto, kLength, rowLengthDescriptor); fields.forEach((field, columnIndex) => { - columnDescriptor.get = function() { - const child = (this as any as BoundRow)[kParent].getChildAt(columnIndex); - return child ? child.get((this as any as BoundRow)[kRowIndex]) : null; - }; - // set configurable to true to ensure Object.defineProperty - // doesn't throw in the case of duplicate column names - columnDescriptor.configurable = true; - columnDescriptor.enumerable = fieldsAreEnumerable; - Object.defineProperty(proto, field.name, columnDescriptor); - columnDescriptor.configurable = false; - columnDescriptor.enumerable = !fieldsAreEnumerable; - Object.defineProperty(proto, columnIndex, columnDescriptor); + if (!proto.hasOwnProperty(field.name)) { + columnDescriptor.enumerable = fieldsAreEnumerable; + columnDescriptor.get || (columnDescriptor.get = this._bindGetter(columnIndex)); + Object.defineProperty(proto, field.name, columnDescriptor); + } + if (!proto.hasOwnProperty(columnIndex)) { + columnDescriptor.enumerable = !fieldsAreEnumerable; + columnDescriptor.get || (columnDescriptor.get = this._bindGetter(columnIndex)); + Object.defineProperty(proto, columnIndex, columnDescriptor); + } columnDescriptor.get = null as any; }); - this.RowProxy = (BoundRow as any); + this.rowPrototype = proto; + } + + private _bindGetter(columnIndex: number) { + return function(this: Row) { + const child = this[kParent].getChildAt(columnIndex); + return child ? child.get(this[kRowIndex]) : null; + }; } public bind(rowIndex: number) { - const bound = Object.create(this.RowProxy.prototype); + const bound = Object.create(this.rowPrototype); bound[kRowIndex] = rowIndex; return bound; //return new this.RowProxy(rowIndex);