Conversation
dateAdd() wasn’t being converted correctly to ClickHouse. We were getting a syntax error
|
WalkthroughAdds a new comprehensive ClickHouse TSQL function smoke-test suite that runs many categorized TSQL queries against a task_runs test schema with helpers to seed data and assert execution. Introduces unit tests validating date/time functions emit interval units as bare keywords. Updates printer logic to extract and emit interval-unit keywords for certain date functions via a new call-argument printing flow. Updates function lookup logic and exposed function listings in the TSQL functions module. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Technical detailsFiles modified: 4 Key changes:
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal-packages/clickhouse/src/tsqlFunctions.test.ts (1)
110-121: Each test block re-inserts data into a fresh container — consider sharing setup.Every
clickhouseTestcallback callssetupClient, which insertsdefaultTaskRuninto a new container. If theclickhouseTestfixture already provides a shared container per describe block (or can be configured to), you could insert once and reuse across all test blocks, reducing test suite runtime significantly.If containers are already pooled/shared by the fixture, the repeated inserts are still redundant work. A
beforeAll-style setup inserting once would be cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal-packages/clickhouse/src/tsqlFunctions.test.ts` around lines 110 - 121, Tests repeatedly call setupClient which inserts defaultTaskRun into a new container for every clickhouseTest; instead, move the insert to a shared setup so it runs once per describe (or reuse the fixture's pooled container). Modify the test suite to perform the insert of defaultTaskRun in a beforeAll (or equivalent shared fixture) and have setupClient only construct and return the ClickhouseClient without inserting; reference the setupClient function, clickhouseTest callbacks, and defaultTaskRun to locate where to remove the per-test insert and where to add the shared beforeAll insertion.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal-packages/clickhouse/src/tsqlFunctions.test.tsinternal-packages/tsql/src/query/functions.tsinternal-packages/tsql/src/query/printer.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: Always import tasks from@trigger.dev/sdk, never use@trigger.dev/sdk/v3or deprecatedclient.defineJobpattern
Every Trigger.dev task must be exported and have a uniqueidproperty with no timeouts in the run function
Files:
internal-packages/clickhouse/src/tsqlFunctions.test.tsinternal-packages/tsql/src/query/printer.tsinternal-packages/tsql/src/query/functions.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Import from
@trigger.dev/coreusing subpaths only, never import from root
Files:
internal-packages/clickhouse/src/tsqlFunctions.test.tsinternal-packages/tsql/src/query/printer.tsinternal-packages/tsql/src/query/functions.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use vitest for all tests in the Trigger.dev repository
Files:
internal-packages/clickhouse/src/tsqlFunctions.test.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
internal-packages/clickhouse/src/tsqlFunctions.test.tsinternal-packages/tsql/src/query/printer.tsinternal-packages/tsql/src/query/functions.ts
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
internal-packages/clickhouse/src/tsqlFunctions.test.tsinternal-packages/tsql/src/query/printer.tsinternal-packages/tsql/src/query/functions.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.{ts,tsx,js,jsx}: Test files should live beside the files under test and use descriptivedescribeanditblocks
Tests should avoid mocks or stubs and use the helpers from@internal/testcontainerswhen Redis or Postgres are needed
Use vitest for running unit tests
**/*.test.{ts,tsx,js,jsx}: Use vitest exclusively for testing and never mock anything - use testcontainers instead
Place test files next to source files with naming pattern: source file (e.g.,MyService.ts) →MyService.test.ts
Files:
internal-packages/clickhouse/src/tsqlFunctions.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use testcontainers helpers (
redisTest,postgresTest,containerTest) from@internal/testcontainersfor Redis/PostgreSQL testing instead of mocks
Files:
internal-packages/clickhouse/src/tsqlFunctions.test.ts
🧬 Code graph analysis (3)
internal-packages/clickhouse/src/tsqlFunctions.test.ts (2)
internal-packages/clickhouse/src/taskRuns.ts (1)
insertTaskRuns(199-210)internal-packages/testcontainers/src/index.ts (1)
clickhouseTest(219-223)
internal-packages/tsql/src/query/printer.ts (2)
internal-packages/tsql/src/query/functions.ts (1)
TSQL_COMPARISON_MAPPING(33-46)internal-packages/tsql/src/query/ast.ts (2)
Constant(463-466)Field(468-472)
internal-packages/tsql/src/query/functions.ts (1)
internal-packages/tsql/src/index.ts (4)
TSQLFunctionMeta(78-78)TSQL_CLICKHOUSE_FUNCTIONS(76-76)TSQL_AGGREGATIONS(75-75)findTSQLFunction(73-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (27)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: sdk-compat / Node.js 20.20 (ubuntu-latest)
- GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: sdk-compat / Bun Runtime
🔇 Additional comments (5)
internal-packages/tsql/src/query/printer.ts (2)
2811-2811: Good fix for prototype pollution in comparison mapping lookup.Using
Object.prototype.hasOwnProperty.call()instead of theinoperator correctly prevents matching inherited properties liketoString,constructor, etc. fromObject.prototype.
2868-2946: Well-structured interval unit handling for date functions.The implementation cleanly separates concerns:
INTERVAL_UNITSdefines the valid keywords,DATE_FUNCTIONS_WITH_INTERVAL_UNITscopes which functions need special treatment,visitCallArgsorchestrates the dispatch, andextractIntervalUnithandles both constant strings and bare field identifiers. The inclusion of sub-second units (nanosecond,microsecond,millisecond) correctly supportsdateDiff's broader unit vocabulary.internal-packages/clickhouse/src/tsqlFunctions.test.ts (1)
147-830: Comprehensive smoke test coverage — looks good.The test suite methodically covers all major TSQL function categories with real ClickHouse execution, which is a strong regression safety net for the printer and function mapping changes.
internal-packages/tsql/src/query/functions.ts (2)
437-437: Correct fix forifNotFinitearity.ClickHouse's
ifNotFinite(x, y)requires exactly 2 arguments. The previousminArgs: 1would have allowed invalid single-argument calls.
553-612: Clean refactor of function lookup with null-prototype maps.Using
Object.create(null)for the lowercase map eliminates prototype pollution entirely, and the pre-computed maps avoid repeated lowercasing on every lookup. The two-tier lookup (exact match → case-insensitive fallback withcaseSensitiveguard) is well-structured.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal-packages/clickhouse/src/tsqlFunctions.test.ts`:
- Around line 110-121: Tests repeatedly call setupClient which inserts
defaultTaskRun into a new container for every clickhouseTest; instead, move the
insert to a shared setup so it runs once per describe (or reuse the fixture's
pooled container). Modify the test suite to perform the insert of defaultTaskRun
in a beforeAll (or equivalent shared fixture) and have setupClient only
construct and return the ClickhouseClient without inserting; reference the
setupClient function, clickhouseTest callbacks, and defaultTaskRun to locate
where to remove the per-test insert and where to add the shared beforeAll
insertion.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
1 participant
What changed