feat: extract logic for toString into visitor - #886
Conversation
| import { duckdbVisitor } from './visit/duckdb-visitor.js'; | ||
|
|
||
| // Initialize the default visitor | ||
| setDefaultVisitor(duckdbVisitor); |
There was a problem hiding this comment.
This causes a side effect which is probably not good. If we are okay with the general approach in this pull request, I can remove this and change the rest of mosaic to initialize the visitor.
There was a problem hiding this comment.
Agreed, I'd much rather see explicit visitor setup, even if it's doing this same thing in a constructor
| // Abstract methods that must be implemented by concrete visitors | ||
| abstract visitAggregate(node: AggregateNode): string; | ||
| abstract visitBetween(node: BetweenOpNode): string; | ||
| abstract visitBinary(node: BinaryOpNode): string; | ||
| abstract visitCase(node: CaseNode): string; | ||
| abstract visitCast(node: CastNode): string; | ||
| abstract visitCollate(node: CollateNode): string; | ||
| abstract visitColumnParam(node: ColumnParamNode): string; | ||
| abstract visitColumnRef(node: ColumnRefNode): string; | ||
| abstract visitDescribeQuery(node: DescribeQuery): string; | ||
| abstract visitExpression(node: ExprNode): string; | ||
| abstract visitFragment(node: FragmentNode): string; | ||
| abstract visitFromClause(node: FromClauseNode): string; | ||
| abstract visitFunction(node: FunctionNode): string; | ||
| abstract visitIn(node: InOpNode): string; | ||
| abstract visitInterval(node: IntervalNode): string; | ||
| abstract visitList(node: ListNode): string; | ||
| abstract visitLiteral(node: LiteralNode): string; | ||
| abstract visitLogicalOperator(node: LogicalOpNode<ExprNode>): string; | ||
| abstract visitNotBetween(node: NotBetweenOpNode): string; | ||
| abstract visitOrderBy(node: OrderByNode): string; | ||
| abstract visitParam(node: ParamNode): string; | ||
| abstract visitSampleClause(node: SampleClauseNode): string; | ||
| abstract visitScalarSubquery(node: ScalarSubqueryNode): string; | ||
| abstract visitSelectClause(node: SelectClauseNode): string; | ||
| abstract visitSelectQuery(node: SelectQuery): string; | ||
| abstract visitSetOperation(node: SetOperation): string; | ||
| abstract visitTableRef(node: TableRefNode): string; | ||
| abstract visitUnary(node: UnaryOpNode): string; | ||
| abstract visitUnaryPostfix(node: UnaryPostfixOpNode): string; | ||
| abstract visitUnnest(node: UnnestNode): string; | ||
| abstract visitVerbatim(node: VerbatimNode): string; | ||
| abstract visitWhen(node: WhenNode): string; | ||
| abstract visitWindow(node: WindowNode): string; | ||
| abstract visitWindowClause(node: WindowClauseNode): string; | ||
| abstract visitWindowDef(node: WindowDefNode): string; | ||
| abstract visitWindowExtentExpr(node: WindowFrameExprNode): string; | ||
| abstract visitWindowFrame(node: WindowFrameNode): string; | ||
| abstract visitWindowFunction(node: WindowFunctionNode): string; | ||
| abstract visitWithClause(node: WithClauseNode): string; |
There was a problem hiding this comment.
I wonder if this makes future changes tougher at all. For instance if we add a new abstract method, that same PR would have to implement the methods for all visitor implementations, right? In general that seems ok, but if we end up with 5-10 visitors, the person adding new functionality might only be versed in one of them, and wouldn't feel comfortable editing all of them.
Maybe if this abstract class had concrete methods that returned an error for not being implemented, then each language implementation wouldn't have to be 100% in lockstep as new features get supported. Instead of a build error that fails CI, it's now a runtime error if you try to use a new feature with an unsupported grammar, but allows existing queries to complete without issue. gRPC in Go requires something similar, where a default unimplemented concrete server returns errors if not explicitly overridden.
Another approach might be to have this be a concrete implementation of the DuckDB grammar. That solves the problem from above. It also should slim down the per-database implementation, since most of the basic SELECT, ORDER BY, etc., code wouldn't have to be copy/pasted into the concrete implementations. You would only have to override specific methods that deviate from the DuckDB output. That means that the DuckDB visitor couldn't be tree-shaken out, but that doesn't seem like an issue worth optimizing for. It also might make for slightly more brittle language implementations, in the event that DuckDB diverges in grammar, but that seems very unlikely for the shared cases, and again probably not worth bike shedding over.
Overall, I love how this is shaping up. I'm not married to either of these suggestions, just talking through possible design ramifications.
There was a problem hiding this comment.
For the first issue, I am imagining that most dialects would not be in the core mosaic repo so one would only need to implement a new method when updating mosaic. I can also imagine that people would extend the duckdb implementation rather than the base visitor. So I think we can already so that you propose but I separated out the duckdb implementation rather than merging it with the base class which means one can make their totally independent version if desired.
There was a problem hiding this comment.
Perfect, I've been in Go and out of OOP/class driven languages too long. No other notes on the implementation, I think it's good to merge unless @jheer has feedback.
Are you opposed to keeping the first few dialects in the core repo for visibility?
There was a problem hiding this comment.
Nope, we can keep the first few dialects as long as we have testing for them to make sure they actually work
8ab0af5 to
df34653
Compare
|
I made the following edits:
I then tested with all example specs in the browser. |
| @@ -1,4 +1,3 @@ | |||
| import { ToStringVisitor } from './to-string-visitor.js'; | |||
There was a problem hiding this comment.
I don't feel strongly about this, but having these under the visit/codegen folder has a connotation to me that these were generated by code from a spec, vs handwritten here to generate SQL.
There was a problem hiding this comment.
If others feel similarly I have no problem changing this. It's just internal organization so doesn't affect the exported API.
When we implement for dialects, we may need to revisit this and pass something about the dialect so that people can adjust the output. |
Agree. I tried to find a cleaner solution but it's tricky due to the order of operations inside vgplot mark initialization. One idea would be to pass the visitor through the custom node so it can still route sub-nodes through the proper dialect handler. |
|
This is awesome, I'll open a PR soon for BigQuery support. I'm at a conference next week, so it may be the week after |
3 participants
See #880 (comment)