feat: Adding a datasource from a non main schema using fully qualified table name - #671

packages/duckdb-core/__tests__/duckdb-utils.test.ts (1)

3-27: ⚡ Quick win

Consider adding test coverage for table names containing dots.

The current test suite covers common cases well, but doesn't include the edge case where a table name itself contains a dot (e.g., "my.table"), which requires quoting in DuckDB.

📝 Suggested additional test cases

   it('returns the last segment when given a non-main schema', () => {
     expect(getTableNameFromQualified('analytics.events')).toBe('events');
   });
+
+  it('preserves dots within a quoted table name', () => {
+    expect(getTableNameFromQualified('"my.table"')).toBe('my.table');
+  });
+
+  it('preserves dots within a quoted table name in qualified identifier', () => {
+    expect(getTableNameFromQualified('schema."my.table"')).toBe('my.table');
+    expect(getTableNameFromQualified('"db"."schema"."my.table"')).toBe('my.table');
+  });
 });

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/duckdb-core/__tests__/duckdb-utils.test.ts` around lines 3 - 27, Add
tests to cover qualified identifiers where the final table name contains a dot
and is quoted (e.g., '"my.table"' or '"schema"."my.table"') to ensure
getTableNameFromQualified correctly returns the full table name including the
dot; update the test suite around getTableNameFromQualified to include cases for
a single quoted identifier with a dot and a multi-segment qualified identifier
where only the last segment is quoted and contains a dot, asserting the function
returns 'my.table'.

packages/kepler/src/KeplerSlice.ts (1)

575-575: 💤 Low value

String() conversion is unnecessary.

The tableName parameter is already typed as string (line 552), so the String() conversion is redundant.

♻️ Simplify by removing unnecessary conversion

-            const label = getTableNameFromQualified(String(tableName));
+            const label = getTableNameFromQualified(tableName);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/kepler/src/KeplerSlice.ts` at line 575, The call wraps an
already-typed string in String() when computing the label; remove the
unnecessary conversion and pass the tableName variable directly into
getTableNameFromQualified (i.e., replace
getTableNameFromQualified(String(tableName)) with
getTableNameFromQualified(tableName)); ensure there are no other similar
redundant String() uses around the getTableNameFromQualified call or in the
surrounding function (KeplerSlice methods referencing tableName) and run type
checks.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/cells/src/helpers.ts (1)

104-104: ⚡ Quick win

Re-export getUnqualifiedSqlIdentifier from @sqlrooms/duckdb-core

@sqlrooms/duckdb-core already exports getUnqualifiedSqlIdentifier, and using it here keeps this pure helper sourced from the lowest-level package. This won’t materially change the @sqlrooms/cells -> @sqlrooms/duckdb`` dependency, since @sqlrooms/cells imports multiple symbols from `@sqlrooms/duckdb` elsewhere.

♻️ Proposed change

-export {getUnqualifiedSqlIdentifier} from '`@sqlrooms/duckdb`';
+export {getUnqualifiedSqlIdentifier} from '`@sqlrooms/duckdb-core`';

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/cells/src/helpers.ts` at line 104, The current re-export pulls
getUnqualifiedSqlIdentifier from '`@sqlrooms/duckdb`' but it should come from the
lower-level package '`@sqlrooms/duckdb-core`'; change the export so
getUnqualifiedSqlIdentifier is re-exported from '`@sqlrooms/duckdb-core`' (update
the export statement in helpers.ts to reference '`@sqlrooms/duckdb-core`') to keep
the helper sourced from the lowest-level package while preserving existing
imports of other symbols from '`@sqlrooms/duckdb`'.

• pnpm-lock.yaml: Language not supported