You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "swallez (via GitHub)" <gi...@apache.org> on 2024/09/27 19:00:28 UTC
[PR] GH-44246: [JS] Add a lightweight array view to Table [arrow]
swallez opened a new pull request, #44247:
URL: https://github.com/apache/arrow/pull/44247
### Rationale for this change
`Table.toArray()` provides the convenience of using Arrow tables as common object arrays. However it does allocate an array of the size of the table that hold proxies to the table rows. This array can consume a significant amount of memory with large tables.
Providing an array proxy that wraps the table and creates struct row proxies on demand would avoid this and provide a object array view on top of a table at almost zero memory cost.
### What changes are included in this PR?
This PR adds a new `Table.toArrayView()` method that returns an array proxy to the table rows.
It complements `Table.toArray()` and doesn't replace it for the following reasons:
* this is a read-only view, while `toArray()` returns a plain mutable array. Replacing it would be a breaking change.
* the proxy adds some overhead to direct array access. Tests on iterator loops show that access via the array proxy takes 5 more time than direct array access. This can be a concern for some applications, even if it should generally be negligible, and if applications seeking maximum performance should avoid using these convenience wrappers.
Also fixes #30863 by using a singleton proxy handler for `StructRowProxyHandler`, which further reduces memory usage.
### Are these changes tested?
Yes. New tests are added to `js/test/unit/table/table-test.ts` for both `toArrayView()` and `toArray()` that was not tested.
### Are there any user-facing changes?
This adds a new `Table.toArrayView()` method.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44246: [JS] Add a lightweight array view to Table [arrow]
Posted by "swallez (via GitHub)" <gi...@apache.org>.
swallez commented on code in PR #44247:
URL: https://github.com/apache/arrow/pull/44247#discussion_r1779667945
##########
js/src/table.ts:
##########
@@ -238,17 +238,30 @@ export class Table<T extends TypeMap = any> {
*
* @returns An Array of Table rows.
*/
- public toArray() {
+ public toArray(): Array<Struct<T>['TValue']> {
Review Comment:
It just makes it explicit. I added it to know precisely what `toArrayView()` was supposed to return. And also because I like explicit types 😉
##########
js/test/unit/table/table-test.ts:
##########
@@ -79,3 +79,61 @@ describe('tableFromJSON()', () => {
expect(table.getChild('c')!.type).toBeInstanceOf(Dictionary);
});
});
+
+describe('table views', () => {
+ const table = tableFromJSON([{
+ a: 42,
+ b: true,
+ c: 'foo',
+ }, {
+ a: 12,
+ b: false,
+ c: 'bar',
+ }]);
+
+ function checkArrayValues(arr: Array<any>) {
+ expect(arr).toHaveLength(2);
+ expect(arr[0].a).toBe(42);
+ expect(arr[0].b).toBe(true);
+ expect(arr[0].c).toBe('foo');
+ expect(arr[1].a).toBe(12);
+ expect(arr[1].b).toBe(false);
+ expect(arr[1].c).toBe('bar');
+ }
+
+ function checkArray(arr: Array<any>) {
+ test('Wrapper', () => checkArrayValues(arr));
+
+ test('Iterator', () => {
+ const arr2 = [];
+ for (let item of arr) {
+ arr2.push(item);
+ }
+ checkArrayValues(arr);
+ });
+
+ test('Array index', () => {
+ const arr2 = new Array(arr.length);
+ for (let i = 0; i < arr2.length; i++) {
+ arr2[i] = arr[i];
+ }
+ checkArrayValues(arr2);
+ });
+
+ test('Keys', () => {
+ const arr2: any[] = new Array(arr.length);
+ let keys = Object.keys(arr);
+ for (let k in keys) {
+ arr2[k] = arr[k];
+ }
+ checkArrayValues(arr2);
+ });
+ }
+
+ describe('table.toArray()', () => {
+ checkArray(table.toArray());
+ });
+ describe('table.toArrayView()', () => {
+ checkArray(table.toArrayView());
Review Comment:
See the discussion [here](https://github.com/apache/arrow/pull/44247#issuecomment-2380722833). We cannot make `Table` behave like an array.
##########
js/src/table.ts:
##########
@@ -238,17 +238,30 @@ export class Table<T extends TypeMap = any> {
*
* @returns An Array of Table rows.
*/
- public toArray() {
+ public toArray(): Array<Struct<T>['TValue']> {
return [...this];
}
+ /**
+ * Return a JavaScript Array view of the Table rows.
+ *
+ * It is a lightweight read-only proxy that delegates to the table. Accessing elements has some
+ * overhead compared to the regular array returned by `toArray()` because of this indirection,
+ * but it avoids potentially large memory allocation.
+ *
+ * @returns An Array proxy to the Table rows.
+ */
+ public toArrayView(): Array<Struct<T>['TValue']> {
+ return new Proxy([] as Array<Struct<T>['TValue']>, new TableArrayProxyHandler(this));
+ }
+
/**
* Returns a string representation of the Table rows.
*
* @returns A string representation of the Table rows.
*/
public toString() {
- return `[\n ${this.toArray().join(',\n ')}\n]`;
+ return `[\n ${this.toArrayView().join(',\n ')}\n]`;
Review Comment:
Got it. I don't think this is an issue here, as `toString()` is generally used for debugging purposes. Actually I'm wondering if it's practically usable for `Table` beyond tests as it can create huge strings.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44246: [JS] Add a lightweight array view to Table [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on PR #44247:
URL: https://github.com/apache/arrow/pull/44247#issuecomment-2380714682
> Also, do you agree that toArrayView() should not replace toArray()? Or would the slight behavior change be acceptable if we replace it?
Good question. I wonder actually whether we need `toArrayView` at all. Why not use make the vector/table behave like an array view?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44246: [JS] Add a lightweight array view to Table [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on code in PR #44247:
URL: https://github.com/apache/arrow/pull/44247#discussion_r1779658502
##########
js/src/table.ts:
##########
@@ -238,17 +238,30 @@ export class Table<T extends TypeMap = any> {
*
* @returns An Array of Table rows.
*/
- public toArray() {
+ public toArray(): Array<Struct<T>['TValue']> {
return [...this];
}
+ /**
+ * Return a JavaScript Array view of the Table rows.
+ *
+ * It is a lightweight read-only proxy that delegates to the table. Accessing elements has some
+ * overhead compared to the regular array returned by `toArray()` because of this indirection,
+ * but it avoids potentially large memory allocation.
+ *
+ * @returns An Array proxy to the Table rows.
+ */
+ public toArrayView(): Array<Struct<T>['TValue']> {
+ return new Proxy([] as Array<Struct<T>['TValue']>, new TableArrayProxyHandler(this));
+ }
+
/**
* Returns a string representation of the Table rows.
*
* @returns A string representation of the Table rows.
*/
public toString() {
- return `[\n ${this.toArray().join(',\n ')}\n]`;
+ return `[\n ${this.toArrayView().join(',\n ')}\n]`;
Review Comment:
I understand the difference but I wonder whether this change introduces a performance regression since "the proxy adds some overhead to direct array access."
I'm not against this change but want to make sure we have thought through the implications.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44246: [JS] Add a lightweight array view to Table [arrow]
Posted by "swallez (via GitHub)" <gi...@apache.org>.
swallez commented on code in PR #44247:
URL: https://github.com/apache/arrow/pull/44247#discussion_r1779657198
##########
js/src/table.ts:
##########
@@ -238,17 +238,30 @@ export class Table<T extends TypeMap = any> {
*
* @returns An Array of Table rows.
*/
- public toArray() {
+ public toArray(): Array<Struct<T>['TValue']> {
return [...this];
}
+ /**
+ * Return a JavaScript Array view of the Table rows.
+ *
+ * It is a lightweight read-only proxy that delegates to the table. Accessing elements has some
+ * overhead compared to the regular array returned by `toArray()` because of this indirection,
+ * but it avoids potentially large memory allocation.
+ *
+ * @returns An Array proxy to the Table rows.
+ */
+ public toArrayView(): Array<Struct<T>['TValue']> {
+ return new Proxy([] as Array<Struct<T>['TValue']>, new TableArrayProxyHandler(this));
+ }
+
/**
* Returns a string representation of the Table rows.
*
* @returns A string representation of the Table rows.
*/
public toString() {
- return `[\n ${this.toArray().join(',\n ')}\n]`;
+ return `[\n ${this.toArrayView().join(',\n ')}\n]`;
Review Comment:
We need an array (or array-like) to use `join()`. This _was_ creating an array, and with `toArrayView()` this now just creates a proxy. Or did I misunderstand your comment?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44246: [JS] Add a lightweight array view to Table [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on PR #44247:
URL: https://github.com/apache/arrow/pull/44247#issuecomment-2380842013
I see. I'll page @trxcllnt to weigh in on this one.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44246: [JS] Add a lightweight array view to Table [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on code in PR #44247:
URL: https://github.com/apache/arrow/pull/44247#discussion_r1779660217
##########
js/src/table.ts:
##########
@@ -238,17 +238,30 @@ export class Table<T extends TypeMap = any> {
*
* @returns An Array of Table rows.
*/
- public toArray() {
+ public toArray(): Array<Struct<T>['TValue']> {
Review Comment:
Does this improve the type returned by this method or just make the type explicit?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44246: [JS] Add a lightweight array view to Table [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on code in PR #44247:
URL: https://github.com/apache/arrow/pull/44247#discussion_r1779658502
##########
js/src/table.ts:
##########
@@ -238,17 +238,30 @@ export class Table<T extends TypeMap = any> {
*
* @returns An Array of Table rows.
*/
- public toArray() {
+ public toArray(): Array<Struct<T>['TValue']> {
return [...this];
}
+ /**
+ * Return a JavaScript Array view of the Table rows.
+ *
+ * It is a lightweight read-only proxy that delegates to the table. Accessing elements has some
+ * overhead compared to the regular array returned by `toArray()` because of this indirection,
+ * but it avoids potentially large memory allocation.
+ *
+ * @returns An Array proxy to the Table rows.
+ */
+ public toArrayView(): Array<Struct<T>['TValue']> {
+ return new Proxy([] as Array<Struct<T>['TValue']>, new TableArrayProxyHandler(this));
+ }
+
/**
* Returns a string representation of the Table rows.
*
* @returns A string representation of the Table rows.
*/
public toString() {
- return `[\n ${this.toArray().join(',\n ')}\n]`;
+ return `[\n ${this.toArrayView().join(',\n ')}\n]`;
Review Comment:
I understand the difference but I wonder whether this change introduces a regression since "the proxy adds some overhead to direct array access."
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44246: [JS] Add a lightweight array view to Table [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on code in PR #44247:
URL: https://github.com/apache/arrow/pull/44247#discussion_r1779660032
##########
js/test/unit/table/table-test.ts:
##########
@@ -79,3 +79,61 @@ describe('tableFromJSON()', () => {
expect(table.getChild('c')!.type).toBeInstanceOf(Dictionary);
});
});
+
+describe('table views', () => {
+ const table = tableFromJSON([{
+ a: 42,
+ b: true,
+ c: 'foo',
+ }, {
+ a: 12,
+ b: false,
+ c: 'bar',
+ }]);
+
+ function checkArrayValues(arr: Array<any>) {
+ expect(arr).toHaveLength(2);
+ expect(arr[0].a).toBe(42);
+ expect(arr[0].b).toBe(true);
+ expect(arr[0].c).toBe('foo');
+ expect(arr[1].a).toBe(12);
+ expect(arr[1].b).toBe(false);
+ expect(arr[1].c).toBe('bar');
+ }
+
+ function checkArray(arr: Array<any>) {
+ test('Wrapper', () => checkArrayValues(arr));
+
+ test('Iterator', () => {
+ const arr2 = [];
+ for (let item of arr) {
+ arr2.push(item);
+ }
+ checkArrayValues(arr);
+ });
+
+ test('Array index', () => {
+ const arr2 = new Array(arr.length);
+ for (let i = 0; i < arr2.length; i++) {
+ arr2[i] = arr[i];
+ }
+ checkArrayValues(arr2);
+ });
+
+ test('Keys', () => {
+ const arr2: any[] = new Array(arr.length);
+ let keys = Object.keys(arr);
+ for (let k in keys) {
+ arr2[k] = arr[k];
+ }
+ checkArrayValues(arr2);
+ });
+ }
+
+ describe('table.toArray()', () => {
+ checkArray(table.toArray());
+ });
+ describe('table.toArrayView()', () => {
+ checkArray(table.toArrayView());
Review Comment:
Why can't we make this work?
```suggestion
checkArray(table);
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44246: [JS] Add a lightweight array view to Table [arrow]
Posted by "swallez (via GitHub)" <gi...@apache.org>.
swallez commented on PR #44247:
URL: https://github.com/apache/arrow/pull/44247#issuecomment-2380722833
> Why not use make the vector/table behave like an array view?
Mimicking an array is a bit tricky: it has a lot of methods that are implemented by iterating it (like `join`, `map`, `find`, etc) and we would have to reimplement all of them. Also, `Array.isArray()` would return `false` even if we reimplemented all array methods.
This is why `toArrayView()` creates a proxy to an empty array and passes the table to the proxy handler:
```js
return new Proxy([] as Array<Struct<T>['TValue']>, new TableArrayProxyHandler(this));
```
That way the proxy handler only has to implement iteration and direct access, and benefits from all other array methods by delegating to the wrapped array.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44246: [JS] Add a lightweight array view to Table [arrow]
Posted by "swallez (via GitHub)" <gi...@apache.org>.
swallez commented on PR #44247:
URL: https://github.com/apache/arrow/pull/44247#issuecomment-2380714034
Thank @domoritz!
> Would you also add this to vectors or if not, why only tables?
That absolutely makes sense. My immediate need was for tables for an application that uses plain objects, and I will add it for vectors.
Also, do you agree that `toArrayView()` should not replace `toArray()`? Or would the slight behavior change be acceptable if we replace it?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44246: [JS] Add a lightweight array view to Table [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on code in PR #44247:
URL: https://github.com/apache/arrow/pull/44247#discussion_r1779651957
##########
js/src/table.ts:
##########
@@ -238,17 +238,30 @@ export class Table<T extends TypeMap = any> {
*
* @returns An Array of Table rows.
*/
- public toArray() {
+ public toArray(): Array<Struct<T>['TValue']> {
return [...this];
}
+ /**
+ * Return a JavaScript Array view of the Table rows.
+ *
+ * It is a lightweight read-only proxy that delegates to the table. Accessing elements has some
+ * overhead compared to the regular array returned by `toArray()` because of this indirection,
+ * but it avoids potentially large memory allocation.
+ *
+ * @returns An Array proxy to the Table rows.
+ */
+ public toArrayView(): Array<Struct<T>['TValue']> {
+ return new Proxy([] as Array<Struct<T>['TValue']>, new TableArrayProxyHandler(this));
+ }
+
/**
* Returns a string representation of the Table rows.
*
* @returns A string representation of the Table rows.
*/
public toString() {
- return `[\n ${this.toArray().join(',\n ')}\n]`;
+ return `[\n ${this.toArrayView().join(',\n ')}\n]`;
Review Comment:
Since we are stringifying the whole table, is it worth using an array view here?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44246: [JS] Add a lightweight array view to Table [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #44247:
URL: https://github.com/apache/arrow/pull/44247#issuecomment-2379885221
:warning: GitHub issue #44246 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44246: [JS] Add a lightweight array view to Table [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #44247:
URL: https://github.com/apache/arrow/pull/44247#issuecomment-2910719340
:warning: GitHub issue #44246 **has no components**, please add labels for components.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org