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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
