Copilot commented on code in PR #334:
URL: https://github.com/apache/arrow-js/pull/334#discussion_r2853570286
##########
src/schema.ts:
##########
@@ -143,6 +154,18 @@ export class Field<T extends DataType = any> {
: ({ name = this.name, type = this.type, nullable = this.nullable,
metadata = this.metadata } = args[0]);
return Field.new<R>(name, type, nullable, metadata);
}
+
+ /**
+ * Create a new Field with replaced metadata. Accepts either a Map or a
plain object.
+ * Pass `null` to clear existing metadata.
+ *
+ * @param metadata Replacement metadata entries.
+ */
+ public withMetadata(metadata?: Map<string, string> | Record<string,
string> | null): Field<T> {
+ if (metadata === undefined) { return this; }
+ const next = metadata === null ? new Map<string, string>() :
toMetadataMap(metadata);
Review Comment:
Same as `Schema.withMetadata`: this returns `this` when `metadata ===
undefined` even though the docstring says it creates a new `Field`. If the
intended contract is “Map/object/null only”, make the parameter non-optional
and always return a new instance (or update docs/tests to explicitly document
the `undefined` no-op behavior).
```suggestion
* @param metadata Replacement metadata entries. If omitted, the
existing metadata is preserved on a new Field.
*/
public withMetadata(metadata?: Map<string, string> | Record<string,
string> | null): Field<T> {
const next =
metadata === undefined ? this.metadata
: metadata === null ? new Map<string, string>()
: toMetadataMap(metadata);
```
##########
src/schema.ts:
##########
@@ -69,6 +69,17 @@ export class Schema<T extends TypeMap = any> {
return new Schema<K>(fields, this.metadata);
}
+ /**
+ * Create a new Schema with replaced metadata.
+ *
+ * @param metadata Replacement metadata entries. Pass `null` to clear.
+ */
+ public withMetadata(metadata?: Map<string, string> | Record<string,
string> | null): Schema<T> {
+ if (metadata === undefined) { return this; }
Review Comment:
`withMetadata` is typed as an optional parameter and returns `this` when
`metadata === undefined`. That contradicts the docstring (“Create a new
Schema…”) and the intended API from the issue/PR description (accept
Map/object/null). Consider making the parameter required (no `?`) and always
returning a new `Schema`, or treating `undefined` the same as `null` if you
want a no-arg convenience.
```suggestion
public withMetadata(metadata: Map<string, string> | Record<string,
string> | null): Schema<T> {
```
##########
src/schema.ts:
##########
@@ -69,6 +69,17 @@ export class Schema<T extends TypeMap = any> {
return new Schema<K>(fields, this.metadata);
}
+ /**
+ * Create a new Schema with replaced metadata.
+ *
+ * @param metadata Replacement metadata entries. Pass `null` to clear.
+ */
+ public withMetadata(metadata?: Map<string, string> | Record<string,
string> | null): Schema<T> {
+ if (metadata === undefined) { return this; }
+ const next = metadata === null ? new Map<string, string>() :
toMetadataMap(metadata);
+ return new Schema<T>(this.fields, next, this.dictionaries,
this.metadataVersion);
Review Comment:
`Schema.withMetadata` passes `this.dictionaries` through to the new
`Schema`, which means the returned instance shares the same mutable `Map` as
the original. Other constructors in this file typically let the constructor
regenerate dictionaries (by omitting the arg) or create a new `Map` (see
`assign`). It’d be safer to avoid sharing by omitting the `dictionaries`
argument (or cloning it) since callers can mutate the map despite the
`readonly` property.
```suggestion
return new Schema<T>(this.fields, next, undefined,
this.metadataVersion);
```
##########
test/unit/schema-tests.ts:
##########
@@ -0,0 +1,66 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+import '../jest-extensions.js';
+import { Field, Int32, Schema } from 'apache-arrow';
+
+describe('Field.withMetadata', () => {
+ test('replaces metadata from plain objects', () => {
+ const field = new Field('col', new Int32(), true, new Map([['foo',
'bar']]));
+ const updated = field.withMetadata({ baz: 'qux' });
+ expect(updated).not.toBe(field);
+ expect(updated.metadata.get('foo')).toBeUndefined();
+ expect(updated.metadata.get('baz')).toBe('qux');
+ });
+
+ test('replaces metadata from Maps', () => {
+ const field = new Field('col', new Int32(), true, new Map([['foo',
'bar']]));
+ const updated = field.withMetadata(new Map([['foo', 'baz']]));
+ expect(updated.metadata.get('foo')).toBe('baz');
+ expect(updated.metadata.size).toBe(1);
+ });
+
+ test('clears metadata when null is passed', () => {
+ const field = new Field('col', new Int32(), true, new Map([['foo',
'bar']]));
+ const updated = field.withMetadata(null);
+ expect(updated.metadata.size).toBe(0);
+ });
+});
+
+describe('Schema.withMetadata', () => {
+ test('replaces metadata from plain objects', () => {
+ const schema = new Schema([new Field('col', new Int32())], new
Map([['foo', 'bar']]));
+ const updated = schema.withMetadata({ baz: 'qux' });
+ expect(updated).not.toBe(schema);
+ expect(schema.metadata.get('baz')).toBeUndefined();
+ expect(updated.metadata.get('foo')).toBeUndefined();
+ expect(updated.metadata.get('baz')).toBe('qux');
+ });
+
+ test('replaces metadata from Maps', () => {
+ const schema = new Schema([new Field('col', new Int32())], new
Map([['foo', 'bar']]));
+ const updated = schema.withMetadata(new Map([['foo', 'baz']]));
+ expect(updated.metadata.get('foo')).toBe('baz');
+ expect(updated.metadata.size).toBe(1);
+ });
+
+ test('clears metadata when null is passed', () => {
+ const schema = new Schema([new Field('col', new Int32())], new
Map([['foo', 'bar']]));
+ const updated = schema.withMetadata(null);
+ expect(updated.metadata.size).toBe(0);
Review Comment:
The new tests validate replacement/clearing, but they don’t assert the input
metadata isn’t aliased. Since `withMetadata` clones Maps/objects via
`toMetadataMap`, add an assertion that mutating the input `Map` after calling
`withMetadata` does not change `updated.metadata` (and similarly that
`field.metadata`/`schema.metadata` remain unchanged). This guards against
regressions where metadata could be shared accidentally.
```suggestion
const metadata = { baz: 'qux' };
const updated = field.withMetadata(metadata);
expect(updated).not.toBe(field);
expect(updated.metadata.get('foo')).toBeUndefined();
expect(updated.metadata.get('baz')).toBe('qux');
// Original field metadata should remain unchanged
expect(field.metadata.get('foo')).toBe('bar');
expect(field.metadata.get('baz')).toBeUndefined();
// Mutating the input object after withMetadata should not affect
updated.metadata
metadata.baz = 'changed';
(metadata as any).newKey = 'newValue';
expect(updated.metadata.get('baz')).toBe('qux');
expect(updated.metadata.get('newKey')).toBeUndefined();
});
test('replaces metadata from Maps', () => {
const field = new Field('col', new Int32(), true, new Map([['foo',
'bar']]));
const metadata = new Map([['foo', 'baz']]);
const updated = field.withMetadata(metadata);
expect(updated.metadata.get('foo')).toBe('baz');
expect(updated.metadata.size).toBe(1);
// Original field metadata should remain unchanged
expect(field.metadata.get('foo')).toBe('bar');
expect(field.metadata.size).toBe(1);
// Mutating the input Map after withMetadata should not affect
updated.metadata
metadata.set('foo', 'qux');
metadata.set('newKey', 'newValue');
expect(updated.metadata.get('foo')).toBe('baz');
expect(updated.metadata.get('newKey')).toBeUndefined();
expect(updated.metadata.size).toBe(1);
});
test('clears metadata when null is passed', () => {
const field = new Field('col', new Int32(), true, new Map([['foo',
'bar']]));
const updated = field.withMetadata(null);
expect(updated.metadata.size).toBe(0);
// Original field metadata should remain unchanged
expect(field.metadata.get('foo')).toBe('bar');
expect(field.metadata.size).toBe(1);
});
});
describe('Schema.withMetadata', () => {
test('replaces metadata from plain objects', () => {
const schema = new Schema([new Field('col', new Int32())], new
Map([['foo', 'bar']]));
const metadata = { baz: 'qux' };
const updated = schema.withMetadata(metadata);
expect(updated).not.toBe(schema);
expect(schema.metadata.get('baz')).toBeUndefined();
expect(schema.metadata.get('foo')).toBe('bar');
expect(updated.metadata.get('foo')).toBeUndefined();
expect(updated.metadata.get('baz')).toBe('qux');
// Mutating the input object after withMetadata should not affect
updated.metadata
metadata.baz = 'changed';
(metadata as any).newKey = 'newValue';
expect(updated.metadata.get('baz')).toBe('qux');
expect(updated.metadata.get('newKey')).toBeUndefined();
});
test('replaces metadata from Maps', () => {
const schema = new Schema([new Field('col', new Int32())], new
Map([['foo', 'bar']]));
const metadata = new Map([['foo', 'baz']]);
const updated = schema.withMetadata(metadata);
expect(updated.metadata.get('foo')).toBe('baz');
expect(updated.metadata.size).toBe(1);
// Original schema metadata should remain unchanged
expect(schema.metadata.get('foo')).toBe('bar');
expect(schema.metadata.size).toBe(1);
// Mutating the input Map after withMetadata should not affect
updated.metadata
metadata.set('foo', 'qux');
metadata.set('newKey', 'newValue');
expect(updated.metadata.get('foo')).toBe('baz');
expect(updated.metadata.get('newKey')).toBeUndefined();
expect(updated.metadata.size).toBe(1);
});
test('clears metadata when null is passed', () => {
const schema = new Schema([new Field('col', new Int32())], new
Map([['foo', 'bar']]));
const updated = schema.withMetadata(null);
expect(updated.metadata.size).toBe(0);
// Original schema metadata should remain unchanged
expect(schema.metadata.get('foo')).toBe('bar');
expect(schema.metadata.size).toBe(1);
```
--
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]
