Skip to content

fix: use fully qualified name to resolve DuckDB ambiguous reference error#135

Closed
jerome-ng wants to merge 1 commit intoOHDSI:developfrom
jerome-ng:jerome-ng/1914-duckdb-ambiguous-schema-reference
Closed

fix: use fully qualified name to resolve DuckDB ambiguous reference error#135
jerome-ng wants to merge 1 commit intoOHDSI:developfrom
jerome-ng:jerome-ng/1914-duckdb-ambiguous-schema-reference

Conversation

@jerome-ng
Copy link
Copy Markdown
Contributor

No description provided.

Copilot AI review requested due to automatic review settings March 5, 2026 08:47
@jerome-ng jerome-ng requested a review from p-hoffmann as a code owner March 5, 2026 08:47
@jerome-ng jerome-ng changed the base branch from main to develop March 5, 2026 08:48
@jerome-ng jerome-ng closed this Mar 5, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a DuckDB ambiguous reference error by using fully qualified names. However, the diff is extremely broad — it appears to be a large-scale repository restructuring that: renames the root package, reorganizes extensions and plugins into new paths, introduces a new core/server application (with MCP server, plugin system, auth, etc.), adds extensive integration tests, and adds several new plugin examples and infrastructure files.

Changes:

  • Renames and restructures the root package from @ohdsi/trex to @trexsql/ext, reorganizing plugin paths and CI workflows
  • Introduces a new core/server TypeScript/Deno application with auth, MCP server, plugin management, and transform/migration support
  • Adds a comprehensive suite of integration tests for distributed DB, pgwire, ETL, migration, FHIR/CQL, AI, and more

Reviewed changes

Copilot reviewed 1 out of 1 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
core/server/auth.ts New BetterAuth-based auth module with SSO provider management
core/server/plugin/transform.ts New transform plugin handler with endpoint registration and DuckDB query execution
core/server/plugin/migration.ts New migration plugin runner using trex_migration_run_schema
core/server/plugin/utils.ts Plugin directory scanner and utility functions
core/server/mcp/tools/trexdb.ts MCP tools for querying the trexsql engine
core/server/lib/sql.ts SQL escaping/validation utilities
integration-tests/ New integration test suite for all extension modules
package.json Renamed package and updated dependencies
docker-compose.yml Restructured service definitions
Dockerfile Replaced edge-runtime build with trexsql-based build

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

Comment on lines 84 to 86
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The query SELECT COUNT(*)::int AS count FROM "user" does not include a schema qualifier. Given that this pool is configured with search_path=trex,public, the query will resolve against the trex schema at runtime, but it is inconsistent with all other queries in this file which explicitly use trex."user". If search_path ever changes or this code is reused in a different context, the query could silently target the wrong table. It should use FROM trex."user" for consistency and correctness.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above — UPDATE "user" lacks the trex. schema prefix. This is inconsistent with all other queries in the file that use trex."user" and is exactly the kind of ambiguous reference the PR title says it's fixing. This should be UPDATE trex."user".

Suggested change
'UPDATE trex."user" SET role = $1 WHERE id = $2',

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function name waitfor should follow camelCase conventions and be renamed to waitFor to match standard TypeScript naming conventions used elsewhere in the codebase (e.g., wait_for in Python tests, and camelCase style in the TS files).

Copilot uses AI. Check for mistakes.
Comment on lines 3 to 5
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two imports from the same module should be combined into a single import statement: import { pool, reloadAuthProviders } from "../../auth.ts";

Suggested change
import { pool, reloadAuthProviders } from "../../auth.ts";

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A hardcoded password (Toor1234) is committed in the Docker Compose file for the HANA service. Even for a test/development environment, this is a poor practice since the password is publicly visible in the repository. Consider using an environment variable (e.g., ${HANA_MASTER_PASSWORD:-Toor1234}) so it can be overridden without changing source code.

Suggested change
- ${HANA_MASTER_PASSWORD:-Toor1234}

Copilot uses AI. Check for mistakes.
Comment on lines 124 to 125
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pg-trex-up target is missing a blank line separator after the test-etl-db rule. This is a formatting inconsistency compared to other target separations in the Makefile (e.g., test-pg-trex and check-migration are similarly missing a blank line on line 138/139). These should have blank lines for readability.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants