Conversation
For now we’re going to always add FINAL to TRQL queries for data correctness. In the future we will implement an automated optimization where we use `SELECT argMax(column, _version)` and `WHERE _is_deleted = 0`. But this is a more complex change and needs more investigation of downsides.
|
WalkthroughThis change modifies the FINAL keyword emission logic in JOIN handling within the SQL printer. Previously, FINAL was conditionally appended based on a Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (4)**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.ts📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧬 Code graph analysis (1)internal-packages/tsql/src/query/printer.ts (1)
⏰ 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)
✏️ Tip: You can disable this entire section by setting 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| joinStrings.push(`AS ${this.printIdentifier(node.alias)}`); | ||
| } |
There was a problem hiding this comment.
🚩 Pre-existing double-alias bug for direct table references with explicit aliases
When a direct table reference has an explicit alias (e.g., FROM runs AS r), the code at internal-packages/tsql/src/query/printer.ts:1494-1497 already embeds the alias into the table string: clickhouseName AS r. Then at internal-packages/tsql/src/query/printer.ts:1557-1559, if node.alias is set, it pushes AS r again. This would produce clickhouseName AS r AS r FINAL, which is invalid ClickHouse syntax.
This is pre-existing (not introduced by this PR), and no tests exercise explicit table aliases in FROM clauses. The alias block at line 1557-1559 appears intended for subqueries/placeholders (which don't embed their own alias), but it fires unconditionally for all expression types including direct table references that already include the alias.
With this PR always adding FINAL, the invalid output becomes more likely to surface if table aliases are ever used in TSQL queries.
(Refers to lines 1557-1560)
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
For now we’re going to always add FINAL to TRQL queries for data correctness.
In the future we will implement an automated optimization where we use
SELECT argMax(column, _version)andWHERE _is_deleted = 0. But this is a more complex change and needs more investigation of downsides.