diff --git a/nodejs/__test__/connection.test.ts b/nodejs/__test__/connection.test.ts index 355b86ccb..e32e7e626 100644 --- a/nodejs/__test__/connection.test.ts +++ b/nodejs/__test__/connection.test.ts @@ -36,6 +36,87 @@ describe("when connecting", () => { }); }); +describe.each([ + [ + "URI overload", + (uri: string, options: { storageOptions?: Record }) => + connect(uri, options), + ], + [ + "object overload", + async ( + uri: string, + options: { storageOptions?: Record }, + ) => { + const input = { uri, ...options }; + if (Object.isFrozen(options)) { + Object.freeze(input); + } + const db = await connect(input); + expect(input).toEqual({ uri, ...options }); + expect(input.storageOptions).toBe(options.storageOptions); + return db; + }, + ], + [ + "namespace", + (uri: string, options: { storageOptions?: Record }) => + connectNamespace("dir", { root: uri }, options), + ], +])("%s connection options", (_name, open) => { + let tmpDir: tmp.DirResult; + beforeEach(() => { + tmpDir = tmp.dirSync({ unsafeCleanup: true }); + }); + afterEach(() => tmpDir.removeCallback()); + + it("should accept frozen options without storage options", async () => { + const options = Object.freeze({}); + const db = await open(tmpDir.name, options); + try { + await db.createTable("frozen", [{ id: 1 }]); + await expect(db.tableNames()).resolves.toContain("frozen"); + expect(options).toEqual({}); + } finally { + await db.close(); + } + }); + + it("should accept frozen options with storage options", async () => { + const storageOptions = Object.freeze({ + newTableDataStorageVersion: "stable", + }); + const options = Object.freeze({ storageOptions }); + const db = await open(tmpDir.name, options); + try { + const table = await db.createTable("frozen", [{ id: 1 }]); + await expect(table.countRows()).resolves.toBe(1); + expect(options.storageOptions).toBe(storageOptions); + expect(storageOptions).toEqual({ newTableDataStorageVersion: "stable" }); + } finally { + await db.close(); + } + }); + + it("should preserve reused options and their storage options", async () => { + const storageOptions = { newTableDataStorageVersion: "stable" }; + const options = { storageOptions }; + for (let i = 0; i < 2; i++) { + const db = await open(tmpDir.name, options); + try { + const table = await db.createTable(`reused_${i}`, [{ id: i }]); + await expect(table.countRows()).resolves.toBe(1); + expect(options.storageOptions).toBe(storageOptions); + expect(storageOptions).toEqual({ + newTableDataStorageVersion: "stable", + }); + } finally { + await db.close(); + } + } + }); +}); + describe("given a connection", () => { let tmpDir: tmp.DirResult; let db: Connection; @@ -367,6 +448,69 @@ describe("given a connection", () => { }); }); + describe.each(["createTable", "createEmptyTable"] as const)( + "%s storage options", + (method) => { + const schema = new Schema([new Field("id", new Float64(), true)]); + + it("should accept frozen creation options without storage options", async () => { + const options = Object.freeze({ + dataStorageVersion: "stable", + enableV2ManifestPaths: true, + }); + const table = + method === "createTable" + ? await db.createTable("frozen_options", [{ id: 1 }], options) + : await db.createEmptyTable("frozen_options", schema, options); + expect(await (table as LocalTable).usesV2ManifestPaths()).toBe(true); + expect(options).not.toHaveProperty("storageOptions"); + }); + + it("should accept frozen storage options and preserve override precedence", async () => { + const storageOptions = Object.freeze({ + newTableDataStorageVersion: "legacy", + newTableEnableV2ManifestPaths: "false", + }); + const options = Object.freeze({ + storageOptions, + dataStorageVersion: "stable", + enableV2ManifestPaths: true, + }); + const table = + method === "createTable" + ? await db.createTable("frozen_storage", [{ id: 1 }], options) + : await db.createEmptyTable("frozen_storage", schema, options); + expect(await (table as LocalTable).usesV2ManifestPaths()).toBe(true); + expect(options.storageOptions).toBe(storageOptions); + expect(storageOptions).toEqual({ + newTableDataStorageVersion: "legacy", + newTableEnableV2ManifestPaths: "false", + }); + }); + + it("should not retain a previous override when reusing storage options", async () => { + const storageOptions = { newTableEnableV2ManifestPaths: "false" }; + const create = async (name: string, enableV2ManifestPaths?: boolean) => + method === "createTable" + ? db.createTable(name, [{ id: 1 }], { + storageOptions, + enableV2ManifestPaths, + }) + : db.createEmptyTable(name, schema, { + storageOptions, + enableV2ManifestPaths, + }); + const first = await create("override", true); + expect(await (first as LocalTable).usesV2ManifestPaths()).toBe(true); + const second = await create("without_override"); + expect(await (second as LocalTable).usesV2ManifestPaths()).toBe(false); + expect(storageOptions).toEqual({ + newTableEnableV2ManifestPaths: "false", + }); + }); + }, + ); + it("should be able to migrate tables to the V2 manifest paths", async () => { const db = await connect(tmpDir.name); const table = (await db.createEmptyTable( diff --git a/nodejs/lancedb/connection.ts b/nodejs/lancedb/connection.ts index f866d1ecd..44fde96f1 100644 --- a/nodejs/lancedb/connection.ts +++ b/nodejs/lancedb/connection.ts @@ -893,23 +893,24 @@ export class LocalConnection extends Connection { private getStorageOptions( options?: Partial, ): Record | undefined { + let storageOptions = options?.storageOptions; if (options?.dataStorageVersion !== undefined) { - if (options.storageOptions === undefined) { - options.storageOptions = {}; - } - options.storageOptions["newTableDataStorageVersion"] = - options.dataStorageVersion; + storageOptions = { + ...storageOptions, + newTableDataStorageVersion: options.dataStorageVersion, + }; } if (options?.enableV2ManifestPaths !== undefined) { - if (options.storageOptions === undefined) { - options.storageOptions = {}; - } - options.storageOptions["newTableEnableV2ManifestPaths"] = - options.enableV2ManifestPaths ? "true" : "false"; + storageOptions = { + ...storageOptions, + newTableEnableV2ManifestPaths: options.enableV2ManifestPaths + ? "true" + : "false", + }; } - return cleanseStorageOptions(options?.storageOptions); + return cleanseStorageOptions(storageOptions); } async createTable( diff --git a/nodejs/lancedb/index.ts b/nodejs/lancedb/index.ts index 1057e2d53..4ad8b2a93 100644 --- a/nodejs/lancedb/index.ts +++ b/nodejs/lancedb/index.ts @@ -420,10 +420,10 @@ export async function connect( throw new Error("uri is required"); } - finalOptions = (finalOptions as ConnectionOptions) ?? {}; - (finalOptions).storageOptions = cleanseStorageOptions( - (finalOptions).storageOptions, - ); + finalOptions = { + ...finalOptions, + storageOptions: cleanseStorageOptions(finalOptions.storageOptions), + }; // Create native header provider if one was provided let nativeProvider: NativeJsHeaderProvider | undefined; @@ -630,11 +630,10 @@ export async function connectNamespace( properties = configOrProperties as Record; } - const finalOptions: ConnectNamespaceOptions = (options ?? - {}) as ConnectNamespaceOptions; - finalOptions.storageOptions = cleanseStorageOptions( - finalOptions.storageOptions, - ); + const finalOptions: ConnectNamespaceOptions = { + ...options, + storageOptions: cleanseStorageOptions(options?.storageOptions), + }; const nativeConn = await LanceDbConnection.newWithNamespace( implName,