Skip to content
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

feat: support experimental access to ast #108

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

carsonfarmer
Copy link
Member

@carsonfarmer carsonfarmer commented Aug 15, 2023

Summary

Adds support (via updating the compiler) for full access to the underlying AST object. However, in this PR we focus solely on access to CreateTable statements to keep things simple and consistent.

It adds two new functions:

/**
   * Internal API: Build an AST from a string containing (possibly multiple) SQL statement(s).
   * @internal
   * @param sql A string containing SQL statement(s).
   * @return A `Promise` that resolves to an AST object.
   */
  export function createStatementToObject(sql: string): Promise<CreateTable>;

  /**
   * Internal API: Build a string containing (possibly multiple) SQL statement(s) from an input AST.
   * @internal
   * @param ast An AST object.
   * @return A `Promise` that resolves to a string containing (possibly multiple) SQL statement(s).
   */
  export function createStatementFromObject(ast: CreateTable): Promise<string>;

It also adds support for deriving types from go structs thanks to github.com/OneOfOne/struct2ts. The Makefile now has a new command that generates Typescript types from the relevant Go structs, and also creates a valid d.ts file to use.

WARNING: This does appear to increase the built size quite a bit. We might now be at the point where the size is unacceptably large.

Before:

➜ du -h main.wasm  
476K    main.wasm

After:

du -h main.wasm 
708K    main.wasm

Context

This was based on a request from building Studio.

Implementation overview

We updated the compiler, and added an experimental function to return a raw JSON AST component for CreateTable statements.

Implementation details and review orientation

This is just a first step, it requires tests, a better API, etc

Checklist

  • Are changes backward compatible with existing SDKs, or is there a plan to manage it correctly?
  • Are changes covered by existing tests, or were new tests included?
  • Are code changes optimized for future code readers, commenting on problematic areas to understand (if any)?
  • Future-self question: Did you avoid unjustified/unnecessary complexity to achieve the goal?

@carsonfarmer carsonfarmer added enhancement New feature or request javascript Pull requests that update Javascript code labels Aug 15, 2023
@carsonfarmer carsonfarmer self-assigned this Aug 15, 2023
@carsonfarmer carsonfarmer marked this pull request as draft August 15, 2023 17:46
Signed-off-by: Carson Farmer <[email protected]>
@joewagner
Copy link
Collaborator

Just taking an early look here. This is gonna be really nice for the studio, and also SDK features in general!

@carsonfarmer carsonfarmer marked this pull request as ready for review September 15, 2023 21:35
@carsonfarmer
Copy link
Member Author

Hey @joewagner can you take a look at this? Also, ideally we'll rebuild the TS types from the Go structs at publish time. What do you think is the best way to go about this?

@joewagner
Copy link
Collaborator

Hey @joewagner can you take a look at this? Also, ideally we'll rebuild the TS types from the Go structs at publish time. What do you think is the best way to go about this?

I'll take a look asap!
RE: generating the types
I'll have to look at how you're doing the ts type creation. It seems like it would be part of the build process, i.e. when we compile the wasm we also create the typescript types. Does that make sense to you?

joewagner
joewagner previously approved these changes Sep 18, 2023
Copy link
Collaborator

@joewagner joewagner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@carsonfarmer This is gonna be a really nice feature. Let's release this week!

Copy link
Member Author

@carsonfarmer carsonfarmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, perhaps @brunocalza can take a quick look here?

Signed-off-by: Carson Farmer <[email protected]>
Signed-off-by: Carson Farmer <[email protected]>
@brunocalza
Copy link
Collaborator

Also, perhaps @brunocalza can take a quick look here?

Looks good. The only comment is that with this we're exposing the AST, which means the AST becomes an API. Changes in the AST without breaking clients become way harder to make. The AST structure seems very stable, so maybe that's not a big deal. And also fine if the benefit of exposing is greater than the pain of breaking changes :)

@carsonfarmer
Copy link
Member Author

The only comment is that with this we're exposing the AST, which means the AST becomes an API. Changes in the AST without breaking clients become way harder to make.

This is a very valid concern. I did mark the API as @internal, so ideally breaking changes are ok and expected. But I do think the benefits here outweigh the downsides? Also, I'm not actually sure that anyone other than us are actually using the parser in js directly?

@asutula
Copy link
Contributor

asutula commented Sep 20, 2023

This is looking good @carsonfarmer. Related to @brunocalza's comment about exposing the actual AST, just want to remind you that it is not the actual AST data structure we'll be working with in Studio. There is a JSON data structure derived from the AST which is what the gateway returns when you get metadata about a table. That is what we store when importing a table to Studio, and that JSON structure is what we want to create/store when creating a table in Studio. You can see the validator code that processes the AST into that JSON object here: https://github.com/tablelandnetwork/go-tableland/blob/main/internal/gateway/impl/gateway_store.go#L98C4-L98C4

With direct access to the AST in Studio, we'd have to port that JSON-creating code in order to make the same JSON structure, or possibly there is a way to keep the AST internal and just have your createStatementToObject and createStatementFromObject do that conversion to/from JSON? If that is possible, we'd still want some typescript type that represent that JSON structure exported from the parser lib.

If that direction does seem appealing, it would make sense to extract the AST <-> JSON Go code to some shared file that could be used by both the gateway and parser Go code.

@carsonfarmer
Copy link
Member Author

carsonfarmer commented Sep 20, 2023

No this PR doesn't expose the full AST to the client. It only exposes this type:

export interface CreateTable {
  Table: Table | null;
  ColumnsDef: ColumnDef[] | null;
  Constraints: TableConstraint[] | null;
  StrictMode: boolean;
}

And this type is what is already exported from the parser via this PR. This type is the structure you'd need to get the table definitions into (see js/go-types.d.ts).

There isn't an easy way to create a create statement from what you currently have in the client. It would require parsing the constraints into their specific constraint types (which are exposed as interfaces/types here). I was thinking you could use this type rather than the one you get from the API (because this one is more expressive). I can probably create a JS/TS wrapper in the studio to convert from string constraints to constraint objects...

* @param {any} schema
* @returns {string} The CREATE statement string
*/
function generateCreateTableStatement(tableName, schema) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function converts from your Schema format into a create statement string @asutula. From there, we can generate the more verbose/complete AST format. However, we can't round-trip any constraints that include an expression, only things like primary key, unique, etc. Not check, generated as, and others.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants