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

[RFC] [DML] INSERT, UPSERT, and REPLACE #11

Merged
merged 15 commits into from
Sep 6, 2022
Merged

[RFC] [DML] INSERT, UPSERT, and REPLACE #11

merged 15 commits into from
Sep 6, 2022

Conversation

am357
Copy link
Contributor

@am357 am357 commented Jun 17, 2022

Issue #, if available: #4

Description of changes:
This PR includes the RFC text for addition of PartiQL INSERT and UPDATE specification.

rendered markdown file

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@am357 am357 force-pushed the main branch 2 times, most recently from 44ace81 to c0c0041 Compare June 17, 2022 23:30
RFCs/11-partiql-insert-update-spec.md Outdated Show resolved Hide resolved
RFCs/11-partiql-insert-update-spec.md Outdated Show resolved Hide resolved
@am357 am357 marked this pull request as ready for review June 21, 2022 18:42
@am357 am357 force-pushed the main branch 3 times, most recently from cce7f79 to 745f7c9 Compare July 14, 2022 22:10
@am357 am357 changed the title Add PartiQL INSERT and UPDATE RFC [RFC] [DML] INSERT, UPSERT, and REPLACE Jul 14, 2022
@am357 am357 self-assigned this Jul 14, 2022
@am357 am357 added the RFC label Jul 14, 2022
@am357 am357 force-pushed the main branch 3 times, most recently from 39fcae0 to 09315bf Compare August 17, 2022 17:43
Copy link
Contributor

@almann almann left a comment

Choose a reason for hiding this comment

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

Looks pretty good--I do have a concern with DEFAULT that we should decide on.

RFCs/0011-partiql-insert-update-spec.md Outdated Show resolved Hide resolved
RFCs/0011-partiql-insert-update-spec.md Outdated Show resolved Hide resolved
RFCs/0011-partiql-insert-update-spec.md Outdated Show resolved Hide resolved
almann
almann previously approved these changes Aug 18, 2022
Copy link
Contributor

@almann almann left a comment

Choose a reason for hiding this comment

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

Thanks for the hard work on this--looks good.

- Replace double quotes w/ single quotes
- Move DEFAULT for omitted attributes from MUST to MAY
- Add a comment about addition of  attribute
- Add an example for closed -> open schema
- Clarify referencing a missing value during on-conflict
Copy link

@amrith amrith left a comment

Choose a reason for hiding this comment

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

Looks good Arash, thanks for patiently working through the comments.

amrith
amrith previously approved these changes Aug 26, 2022
Copy link

@amrith amrith left a comment

Choose a reason for hiding this comment

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

LGTM -- oops, duplicate.

almann
almann previously approved these changes Aug 26, 2022
Copy link
Contributor

@almann almann left a comment

Choose a reason for hiding this comment

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

Looks good--minor edit suggested below, but not required if you think it too wordy.

RFCs/0011-partiql-insert-update-spec.md Outdated Show resolved Hide resolved
@am357 am357 dismissed stale reviews from almann and amrith via c1d03f8 August 26, 2022 18:51
Copy link
Contributor

@almann almann left a comment

Choose a reason for hiding this comment

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

:shipit:

@am357 am357 merged commit 28af793 into partiql:main Sep 6, 2022
@am357 am357 mentioned this pull request Sep 6, 2022
```EBNF
<insert statement> ::= INSERT INTO <table name> [ AS <alias> ]
[ ( <attr name> [, <attr name> ]... ) ]
<values>
Copy link
Member

Choose a reason for hiding this comment

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

Leaving a post-merge comment here: Since we use EXCLUDED as a variable name that references a value to be inserted, we could alternatively propose a follow-up RFC that specifies the syntax to allow an alias for the value to be inserted. Such as:

<insert statement> ::= INSERT INTO <table name> [ AS <alias> ] 
    [  ( <attr name> [, <attr name> ]... ) ]
        <values> [ AS <alias> ]
    ...

Note the [ AS <alias> ] after <values>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johnedquinn it's an intriguing idea: what do you consider the value it brings in addition to EXCLUDED?

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

Successfully merging this pull request may close these issues.

6 participants