Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…ical queries Implements the IAnalyticsService contract from @objectstack/spec with: - CubeRegistry (manifest + auto-discovery from metadata) - SemanticCompiler (AnalyticsQuery + Cube → LogicalPlan) - NativeSQLStrategy (SQL push-down via Knex) - ObjectQLStrategy (driver.aggregate() delegation) - MemoryFallbackStrategy (in-memory aggregation for dev/test) - AnalyticsPlugin (kernel plugin, registers 'analytics' service) - generateSql() dry-run support - 44 integration tests covering all strategy branches Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com> Agent-Logs-Url: https://github.com/objectstack-ai/objectql/sessions/5b8e0997-4932-497c-8f39-0620b160ef0a
There was a problem hiding this comment.
Pull request overview
This PR adds a new foundation plugin, @objectql/plugin-analytics, implementing the IAnalyticsService contract and providing a strategy-based execution pipeline intended to support analytical query push-down across different driver backends.
Changes:
- Introduces the
@objectql/plugin-analyticspackage (CubeRegistry, SemanticCompiler, AnalyticsService, and driver strategies). - Adds a comprehensive Vitest suite for the plugin and wires the package into the monorepo test aliasing and lockfile.
- Updates release documentation (ROADMAP + CHANGELOG) to reflect the new plugin.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.ts | Adds path alias for @objectql/plugin-analytics in tests. |
| pnpm-lock.yaml | Registers the new plugin package dependencies in the workspace lockfile. |
| packages/foundation/plugin-analytics/package.json | New package manifest for the analytics plugin. |
| packages/foundation/plugin-analytics/tsconfig.json | TypeScript build config for the new package. |
| packages/foundation/plugin-analytics/src/index.ts | Public exports for plugin/service/strategies and types. |
| packages/foundation/plugin-analytics/src/types.ts | Defines internal IR/types and re-exports spec types for consumers. |
| packages/foundation/plugin-analytics/src/cube-registry.ts | Implements cube registration + metadata-based inference. |
| packages/foundation/plugin-analytics/src/semantic-compiler.ts | Compiles AnalyticsQuery into a driver-agnostic LogicalPlan. |
| packages/foundation/plugin-analytics/src/analytics-service.ts | Implements IAnalyticsService and selects execution strategies. |
| packages/foundation/plugin-analytics/src/strategy-sql.ts | SQL push-down strategy + SQL generation. |
| packages/foundation/plugin-analytics/src/strategy-objectql.ts | Aggregation delegation strategy via driver.aggregate(). |
| packages/foundation/plugin-analytics/src/strategy-memory.ts | In-memory aggregation fallback strategy. |
| packages/foundation/plugin-analytics/src/plugin.ts | Registers the analytics service into the kernel and service registry. |
| packages/foundation/plugin-analytics/test/analytics.test.ts | Integration-style tests for registry/compiler/strategies/service/plugin. |
| ROADMAP.md | Adds plugin-analytics to the published plugin list. |
| CHANGELOG.md | Adds Unreleased entry for the new plugin (also includes unrelated items). |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
CHANGELOG.md:25
- The Unreleased changelog includes multiple entries unrelated to this PR (apps/demo patch-symlinks changes, apps/demo deps,
@objectql/typesdependency move). Since those files aren’t modified in this diff, these bullets appear to be accidental or copied from another change set. Consider removing/moving them to the PR where those changes actually occur to keep release notes accurate.
### Fixed
- **`apps/demo/scripts/patch-symlinks.cjs`** — enhanced to automatically resolve and copy ALL transitive dependencies before dereferencing symlinks. Previously, only direct dependencies listed in `apps/demo/package.json` were available after symlink dereferencing, causing `ERR_MODULE_NOT_FOUND` for transitive deps like `@objectstack/rest`, `zod`, `pino`, `better-auth`, etc. The script now walks each package's pnpm virtual store context (`.pnpm/<name>@<ver>/node_modules/`) and copies any missing sibling dependency into the top-level `node_modules/`, repeating until the full transitive closure is present.
- **`apps/demo`** — added explicit `@objectstack/spec` and `zod` devDependencies as defense-in-depth for Vercel deployment.
- **`@objectql/types`** — moved `@objectstack/spec` and `zod` from `devDependencies` to `dependencies`. The compiled JS output contains runtime imports of `@objectstack/spec` (via `z.infer<typeof Data.X>` patterns), so they must be declared as production dependencies.
| // Build a query object compatible with the driver's aggregate interface | ||
| const query = this.buildAggregateQuery(plan); | ||
| const rows: Record<string, unknown>[] = await d.aggregate(plan.objectName, query); | ||
|
|
There was a problem hiding this comment.
ObjectQLStrategy builds a UnifiedQuery-like object (aggregations/groupBy/where) and passes it to driver.aggregate(). MongoDriver.aggregate() (and MemoryDriver.aggregate()) expect a Mongo aggregation pipeline array, so with supports.queryAggregations=true this strategy will be selected and then fail at runtime. Consider adding a dedicated MongoPipelineStrategy (LogicalPlan -> pipeline) and selecting it for pipeline-based drivers, or making this strategy emit the correct shape based on driver type.
|
|
||
| // Aggregations | ||
| query.aggregations = plan.measures.map(m => ({ | ||
| function: m.aggregation === 'countDistinct' ? 'count' : m.aggregation, |
There was a problem hiding this comment.
countDistinct is mapped to function 'count' here, which drops the distinct semantics and returns incorrect results. Either emit a distinct-capable aggregation representation for drivers that support it (e.g., COUNT(DISTINCT ...) in SQL) or implement a Mongo pipeline translation (e.g., $addToSet + $size).
| function: m.aggregation === 'countDistinct' ? 'count' : m.aggregation, | |
| function: m.aggregation === 'countDistinct' ? 'count' : m.aggregation, | |
| distinct: m.aggregation === 'countDistinct', |
| const d = driver as any; | ||
| if (typeof d.find !== 'function') { | ||
| throw new Error('MemoryFallbackStrategy requires a driver with a find() method.'); | ||
| } |
There was a problem hiding this comment.
These strategies throw plain Error instances. Elsewhere in the codebase (and in SemanticCompiler/AnalyticsService), structured ObjectQLError is used for API-safe error propagation. Consider throwing ObjectQLError here as well (with stable codes) so failures serialize consistently through API layers.
| const values = rows | ||
| .map(r => r[field]) | ||
| .filter((v): v is number => typeof v === 'number'); | ||
|
|
||
| if (values.length === 0) return 0; | ||
|
|
||
| switch (aggregation) { | ||
| case 'sum': | ||
| return values.reduce((a, b) => a + b, 0); | ||
| case 'avg': | ||
| return values.reduce((a, b) => a + b, 0) / values.length; | ||
| case 'min': | ||
| return Math.min(...values); | ||
| case 'max': | ||
| return Math.max(...values); | ||
| case 'countDistinct': | ||
| return new Set(values).size; |
There was a problem hiding this comment.
MemoryFallbackStrategy filters measure values to numbers only. That makes countDistinct incorrect for non-numeric fields (e.g., status/category), because all non-numeric values are dropped before the Set() size is computed. Update countDistinct to consider any non-null primitive (string/number/boolean/date) instead of only numbers.
| const values = rows | |
| .map(r => r[field]) | |
| .filter((v): v is number => typeof v === 'number'); | |
| if (values.length === 0) return 0; | |
| switch (aggregation) { | |
| case 'sum': | |
| return values.reduce((a, b) => a + b, 0); | |
| case 'avg': | |
| return values.reduce((a, b) => a + b, 0) / values.length; | |
| case 'min': | |
| return Math.min(...values); | |
| case 'max': | |
| return Math.max(...values); | |
| case 'countDistinct': | |
| return new Set(values).size; | |
| const rawValues = rows | |
| .map(r => r[field]) | |
| .filter((v): v is unknown => v != null); | |
| switch (aggregation) { | |
| case 'sum': | |
| case 'avg': | |
| case 'min': | |
| case 'max': { | |
| const numericValues = rawValues.filter( | |
| (v): v is number => typeof v === 'number', | |
| ); | |
| if (numericValues.length === 0) return 0; | |
| switch (aggregation) { | |
| case 'sum': | |
| return numericValues.reduce((a, b) => a + b, 0); | |
| case 'avg': | |
| return numericValues.reduce((a, b) => a + b, 0) / numericValues.length; | |
| case 'min': | |
| return Math.min(...numericValues); | |
| case 'max': | |
| return Math.max(...numericValues); | |
| default: | |
| // Should be unreachable due to outer switch, but keep a safe fallback. | |
| return rows.length; | |
| } | |
| } | |
| case 'countDistinct': { | |
| const primitiveValues = rawValues.filter( | |
| (v): v is string | number | boolean | Date => | |
| typeof v === 'string' || | |
| typeof v === 'number' || | |
| typeof v === 'boolean' || | |
| v instanceof Date, | |
| ); | |
| if (primitiveValues.length === 0) return 0; | |
| const normalizedForSet = primitiveValues.map(v => | |
| v instanceof Date ? v.getTime() : v, | |
| ); | |
| return new Set(normalizedForSet).size; | |
| } |
| if (typeof d.aggregate !== 'function') { | ||
| throw new Error( | ||
| 'ObjectQLStrategy requires a driver that implements aggregate(). ' + | ||
| 'Use MemoryFallbackStrategy for drivers without aggregation support.', | ||
| ); |
There was a problem hiding this comment.
These strategies throw plain Error instances. Elsewhere (SemanticCompiler/AnalyticsService) structured ObjectQLError is used, which is safer for API serialization and stable error codes. Consider throwing ObjectQLError here as well (e.g., ANALYTICS_DRIVER_UNSUPPORTED) so kernel consumers can handle failures consistently.
| drivers.forEach((driver: any, index: number) => { | ||
| const driverName = driver.name || (index === 0 ? 'default' : `driver_${index + 1}`); | ||
| datasources![driverName] = driver; | ||
| }); |
There was a problem hiding this comment.
When datasources are derived from kernel.getAllDrivers(), the first driver will be keyed by driver.name if present (e.g., SqlDriver.name='SqlDriver'), not by 'default'. But CubeDefinition.datasource defaults to 'default' (including auto-discovered cubes), so AnalyticsService can throw ANALYTICS_DATASOURCE_NOT_FOUND in the common single-driver case. Consider always adding datasources.default = drivers[0] (without removing name-based keys) or defaulting cubes to the first datasource key.
| selectParts.push(`${this.aggregateExpression(m)} as "${m.alias}"`); | ||
| } | ||
|
|
||
| let sql = `SELECT ${selectParts.join(', ')} FROM "${plan.objectName}"`; | ||
|
|
There was a problem hiding this comment.
buildQueryPlain() interpolates identifiers (field/alias/objectName) directly into the SQL string with double quotes. This is not safely escaped if an identifier contains quotes, and it’s dialect-specific (e.g., MySQL). Consider either always using a real Knex instance for SQL generation, or add strict identifier validation/escaping before interpolation.
| throw new Error('NativeSQLStrategy requires a SQL driver with a knex instance.'); | ||
| } |
There was a problem hiding this comment.
NativeSQLStrategy.getKnex() throws a plain Error when knex is missing. Prefer throwing ObjectQLError with a stable code so callers can surface consistent, API-friendly errors.
| // SELECT — dimensions as group-by columns | ||
| for (const dim of plan.dimensions) { | ||
| builder.select(`${dim.field} as ${dim.alias}`); | ||
| } |
There was a problem hiding this comment.
NativeSQLStrategy uses string interpolation for select (e.g. "${dim.field} as ${dim.alias}") which bypasses Knex identifier escaping and can enable SQL injection if cube definitions or metadata-derived field names are not trusted. Prefer knex.ref(dim.field).as(dim.alias) or knex.raw('?? as ??', [dim.field, dim.alias]) to ensure identifiers are safely escaped.
Implements
IAnalyticsServicefrom@objectstack/specas a new foundation plugin supporting analytical query push-down across SQL, MongoDB, and memory drivers.Architecture
Strategy selection is automatic based on driver capabilities:
knexinstance → full SQL push-downaggregate()+queryAggregations→ delegate to driverPackage:
@objectql/plugin-analyticsMetadataRegistryobjects (numeric fields → sum/avg/min/max measures, other fields → dimensions)orders.count), producesLogicalPlanIAnalyticsService.query(),getMeta(),generateSql()RuntimePluginwithinit()adapter for@objectstack/corecompat, registers'analytics'kernel serviceUsage
Tests
44 tests covering CubeRegistry, SemanticCompiler, all three strategy branches, AnalyticsService dispatch logic, and AnalyticsPlugin lifecycle. No pre-existing tests affected.
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
fastdl.mongodb.org/home/REDACTED/work/_temp/ghcca-node/node/bin/node node /home/REDACTED/work/objectql/objectql/node_modules/.bin/../vitest/vitest.mjs run --reporter=verbose conf�� get --local .2/bin/sh ome/REDACTED/work/sh(dns block)/home/REDACTED/work/_temp/ghcca-node/node/bin/node node /home/REDACTED/work/objectql/objectql/node_modules/.bin/../vitest/vitest.mjs run --reporter=verbose cal/�� /var/lib/waagent(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.