-
Notifications
You must be signed in to change notification settings - Fork 0
chore: update README following PLK v1 upgrade #114
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice and clean update to the README. The new structure and examples make it much easier to get started. I’ll start learning Scribe following this documentation.
Each dialect is _quite_ similar (hence dialect) so much of the base translation from PartiQL's logical plan to an SQL AST is captured by `org.partiql.transpiler.sql.SqlTransform`. | ||
For now, Scribe provides four simple SQL text targets - PartiQL, Redshift, SparkSQL, and Trino. | ||
Each dialect is _quite_ similar (hence dialect) so much of the base translation from PartiQL's logical plan to an SQL AST is captured by `org.partiql.scribe.sql.PlanToAst`. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also provide a link for org.partiql.scribe.sql.PlanToAst
* `RelConverter` — Ruleset for `Rel` plan to AST `ExprQuerySet` conversion | ||
* `RexConverter` — Ruleset for `Rex` plan to AST `Expr` conversion | ||
* `AstToSql` — Ruleset for AST to SQL conversion | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a separate conversation, but are we planning to provide some concrete examples showing how to implement and use these APIs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the concrete examples are not necessary for now. We already have examples in the codebase that are searchable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to compile them as exmaple links?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just my opinion but users can easily search this repo for implementations of all of the above. If we include links, there are a couple issues
- hard to keep all the links up-to-date (e.g. if we move stuff around)
- there are four dialects that implement each of the above interfaces, which could be a lot of links to include
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self-review: submodule for PLK was out-of-date. updated in this PR
import org.partiql.spi.catalog.Session | ||
|
||
public open class SparkTarget : SqlTarget() { | ||
override val target: String = "Redshift" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self-review: this was previously incorrect
* `RelConverter` — Ruleset for `Rel` plan to AST `ExprQuerySet` conversion | ||
* `RexConverter` — Ruleset for `Rex` plan to AST `Expr` conversion | ||
* `AstToSql` — Ruleset for AST to SQL conversion | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the concrete examples are not necessary for now. We already have examples in the codebase that are searchable.
val parserResult = parser.parse(statement, partiqlContext) | ||
val parsedStatements = parserResult.statements | ||
if (parsedStatements.size != 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May not a good check here. The parserResult are not accurate when error occurs and should be discarded unless you are aware of the error types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example is just for demonstration purposes to highlight the new Scribe APIs w/ a valid query. We have plenty of other examples on our website and PLK of how to use the parser + planner.
* `RelConverter` — Ruleset for `Rel` plan to AST `ExprQuerySet` conversion | ||
* `RexConverter` — Ruleset for `Rex` plan to AST `Expr` conversion | ||
* `AstToSql` — Ruleset for AST to SQL conversion | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to compile them as exmaple links?
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.