Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

I love this idea, we should totally do something like this. However, I wonder if we shouldn't accept an IterableBuilderOptions instead of Schema, or accept both and convert the schema into an IterableBuilderOptions.

The reason I ask is Unions.

If someone has a JS Array of a distinct number of types, they might want to encode that more efficiently as a DenseUnion. But to do that, they need to give us a function to map each value to its typeId, which is what the valueToChildTypeId function in UnionBuilderOptions enables:

let num = new Field("num", new Float64);
let str = new Field("str", new Utf8);
let struct = new Field("struct", new Struct([str]));
let nullValues = [null, undefined];
vectorFromArray(
 [123, "a", "b", "c", { str: "hello" }, { str: "goodbye" }],
 {
 type: new DenseUnion([0, 1, 2], [num, str, struct]),
 children: {
 "num": { type: num.type, nullValues, },
 "str": { type: str.type, nullValues, },
 "struct": { type: struct.type, nullValues, },
 },
 valueToChildTypeId(_, value, _) {
 switch(typeof value) {
 case "number": return 0;
 case "string": return 1;
 case "object": return 2;
 }
 },
 nullValues,
 }
)

vectorFromArray uses the vector Builder machinery under the hood, so this seems like something worth enabling. As a bonus, users can pass in their own queueingStrategy and highWaterMark to control chunking, or use a custom hash function (i.e. node-metrohash) when dictionary encoding.

What're your thoughts?