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

docs(70): SQL spec for 1.0 #71

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

joshua-spacetime
Copy link

No description provided.

@joshua-spacetime joshua-spacetime self-assigned this Jul 16, 2024
@joshua-spacetime joshua-spacetime force-pushed the joshua/docs/70/sql branch 2 times, most recently from 42b54e2 to 4f000d5 Compare July 17, 2024 00:49
@joshua-spacetime joshua-spacetime marked this pull request as ready for review July 17, 2024 06:48
@@ -1,407 +0,0 @@
# SQL Support
Copy link
Author

Choose a reason for hiding this comment

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

Note, I deleted this file and replaced it with a new one for an easier to review diff. I'm not sure if this will render. I will rename back to index.md before merging.


> **CREATE TABLE** _table_name_ (_column_name_ _data_type_, ...);

![create-table](/images/syntax/create_table.svg)
Copy link
Author

Choose a reason for hiding this comment

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

Images point to the spacetime-web repo. Which means they don't render here.

docs/sql/sql.md Outdated Show resolved Hide resolved
docs/sql/sql.md Show resolved Hide resolved
docs/sql/sql.md Outdated Show resolved Hide resolved
docs/sql/sql.md Outdated Show resolved Hide resolved
docs/sql/sql.md Show resolved Hide resolved
docs/sql/sql.md Show resolved Hide resolved
docs/sql/sql.md Show resolved Hide resolved
docs/sql/sql.md Show resolved Hide resolved
docs/sql/sql.md Show resolved Hide resolved
docs/sql/sql.md Show resolved Hide resolved
@joshua-spacetime joshua-spacetime force-pushed the joshua/docs/70/sql branch 3 times, most recently from 4ccb7c6 to c03adb3 Compare July 19, 2024 20:35
Copy link

@mamcx mamcx left a comment

Choose a reason for hiding this comment

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

I think this LGTM, but wait for a native review.

docs/sql/sql.md Outdated Show resolved Hide resolved
docs/sql/sql.md Show resolved Hide resolved
docs/sql/sql.md Outdated Show resolved Hide resolved
docs/sql/sql.md Outdated Show resolved Hide resolved
docs/sql/sql.md Outdated Show resolved Hide resolved
docs/sql/sql.md Outdated Show resolved Hide resolved
docs/sql/sql.md Outdated Show resolved Hide resolved

-- Delete all rows with a specific item_id
DELETE FROM Inventory WHERE item_id = 1;
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Love the examples!

Copy link
Author

Choose a reason for hiding this comment

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

Thank you @mamcx!

docs/sql/sql.md Outdated Show resolved Hide resolved
docs/sql/sql.md Outdated Show resolved Hide resolved
docs/sql/sql.md Outdated Show resolved Hide resolved
Copy link
Contributor

@cloutiertyler cloutiertyler left a comment

Choose a reason for hiding this comment

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

Approved with various nits mentioned throughout. The only thing I would consider more deeply is whether unquoted identifiers really need to be case insensitive.

@joshua-spacetime
Copy link
Author

I'm waiting to merge this until, at the very least, the spec as been fully implemented, since merging to master automatically deploys doc changes.

@joshua-spacetime
Copy link
Author

NOTE: There are some sql tickets scheduled for 1.0, so I'm moving this to 1.0. It can be merged to master anytime after 0.12 is released. It won't be deployed until it's incorporated into the release branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants