feat: support additional SQL dialects in mosaic-sql#880
Conversation
574019e to
2f69ca1
Compare
|
Thanks @derekperkins. I'd love to generalize We could have a centralized place for SQL serialization, which walks an AST and dispatches on the node type to perform SQL string code generation. This process could be parameterized by a Meanwhile, the AST nodes could still support a What do you think? |
|
That sounds great to me. It didn't feel great colocated where I put it, but there didn't seem to be a cleaner place. I don't think my js skills are necessarily up to the task of splitting that implementation out, but I can definitely take on the first few dialect translations. |
|
As I continue to think about it, this may open up some other opportunities. We, like many others, have data in a few different silos. I described some of that in more detail (#815 (comment)), but TL;DR, the idea was the prior to engaging mosaic, we would load a tiny subset of data from BigQuery into DuckDB, to be used for the mosaic backend. With the SQL generation split out from the AST, that could open up options maybe for multi-tier execution (#389). Based on certain criteria, the coordinator dialect / connector could be toggled.
The last two options might both still run one of the mosaic server implementations, but the generated SQL would be specific to the destination. Hybrid execution is certainly out of scope for this specific change, but keeping it in mind might inform some decisions along the way. Maybe this is just inane rantings of a data guy on too little sleep :) |
|
Very cool. I'd love to have better support for different backend. Using a visitor as suggested makes the most sense to me so people can bring their own dialects and we can treeshake out the default duckdb dialect if it's not used. |
|
I'm not sure I'll have cycles. The main thing would be to take all the mosaic/packages/mosaic/sql/src/ast/aggregate.ts Lines 111 to 123 in 64ac2aa |
|
@domoritz that's awesome, thanks! I'm assuming I should just have them start with those commits and follow your same pattern? |
|
Yeah, I think so. I just opened it up for review so maybe @jheer and you can take a look and we can decide how we want to merge this already as is. |
|
Reviewed. I'll go ahead and close this PR in favor of yours. |
This is very much a proof of concept, primarily for the purpose of driving discussion of how to add additional support for other SQL engines. The primary concern with #681 is that it requires a python dependency for sqlglot. As we already have a well defined AST, it feels unnecessary to stringify the duckdb SQL statement, to then just parse it into another AST for translation. Specific SQL overlap will vary between engines, but a primary assumption I am making here is that most basic AST nodes will share the same SQL output, with overrides for more engine specific functions.
I had thought about possibly sending the AST over the wire to the server, as opposed to the stringified SQL, so translation could happen there, but that leaves a similar issue to #681, where you'd either have to run node to manage it on the backend, or recreate the AST and SQL generation in whatever language you were using.
I added a
dialectfield toSQLNode, which defaults toduckdb(could also use an empty value if that saves memory for the common case), which all other nodes inherit from. Then inside eachtoString, there is a dialect specific override. For the purposes of this PoC, I chose ClickHouse, StarRocks, and BigQuery as alternative dialects, and specifically list functions, as it shows significant edge cases.ClickHouse has equivalent functions to DuckDB, but differently named functions for all three:
list_contains=>haslist_contains_any=>hasAnylist_contains_all=>hasAllStarRocks is similar to ClickHouse, with equivalent but differently named functions:
list_contains=>array_containslist_contains_any=>arrays_overlaplist_contains_all=>array_contains_allBigQuery however has no convenience functions, so it requires a nested statement for 2 of the three cases. I'm not 100% sure that the AST as currently constituted can express all of these, so they are pseudocoded strings for the purposes of this PoC, but I'm assuming it should work returned as an expression function:
list_contains=>element IN UNNEST(list1)list_contains_any=>EXISTS(SELECT * FROM UNNEST(list1) AS l1 WHERE l1 IN UNNEST(list2))list_contains_all=>(SELECT COUNT(l2) = ARRAY_LENGTH(list2) FROM UNNEST(list1) AS l1 JOIN UNNEST(list1) AS l2 ON l1 = l2)As written, the data flows don't actually work. The functions themselves don't actually inherit from
SQLNode, so theswitch (this.dialect)isn't actually going to work, I'm just not sure where the best place to set that and make it available to all functions, without necessarily requiring dialect to be passed through at every callsite. Is there some global / coordinator scope that the functions might have access to?This has the benefit of requiring zero dependencies on other libraries, and should be the most targeted approach, but also requires manual support for any dialects we want to add. Since the AST is already targeting the most common subset of SQL functionality, my gut feeling is that overrides won't be required for most expressions.
Co-locating the SQL dialect translations per node should make it relatively simple to add overrides as necessary, but since there is no common place where all the per dialect overrides live, it might be harder to verify full coverage. Once we determine a way to actually pipe the dialect through SQL generation, maybe there is a different architecture that makes sense. I'm not a JavaScript guru, so I'm not tied to any implementation details.
Let me know @jheer if you think this is worth chasing down, or if there's another approach to test out.
Fixes #399