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

sql: add support for CREATE TABLE ... FROM WEBHOOK #30821

Merged

Conversation

ParkMyCar
Copy link
Member

This PR implements support for the CREATE TABLE ... FROM WEBHOOK syntax which is needed for the larger "Source Versioning" project, if the enable_create_table_from_source feature flag is enabled. This change is purely adding support for parsing the syntax and nothing about planning or sequencing changes.

One oddity is CREATE TABLE ... FROM SOURCE does not support specifying IN CLUSTER, the dataflow that writes data into the table gets installed on the same cluster the SOURCE is installed on. Webhooks don't get installed on a cluster today so not supporting CREATE TABLE ... FROM WEBHOOK IN CLUSTER ... doesn't lose any functionality, but it is the one difference between this new and the existing syntax.

Motivation

Closes https://github.com/MaterializeInc/database-issues/issues/8494

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

* add support in the sql parser
* add test case in testdrive
* update platform checks to use CREATE TABLE ... FROM WEBHOOK
@ParkMyCar ParkMyCar requested review from a team as code owners December 13, 2024 19:57
@ParkMyCar ParkMyCar requested a review from jkosh44 December 13, 2024 19:57
@ParkMyCar ParkMyCar force-pushed the sql/create-table-from-webhook branch from 975c22e to ad88856 Compare December 16, 2024 14:43
@jkosh44
Copy link
Contributor

jkosh44 commented Dec 16, 2024

One oddity is CREATE TABLE ... FROM SOURCE does not support specifying IN CLUSTER, the dataflow that writes data into the table gets installed on the same cluster the SOURCE is installed on. Webhooks don't get installed on a cluster today so not supporting CREATE TABLE ... FROM WEBHOOK IN CLUSTER ... doesn't lose any functionality, but it is the one difference between this new and the existing syntax.

When creating the underlying webhook source, does that allow you to specify a specific cluster?

@jkosh44
Copy link
Contributor

jkosh44 commented Dec 16, 2024

One oddity is CREATE TABLE ... FROM SOURCE does not support specifying IN CLUSTER, the dataflow that writes data into the table gets installed on the same cluster the SOURCE is installed on. Webhooks don't get installed on a cluster today so not supporting CREATE TABLE ... FROM WEBHOOK IN CLUSTER ... doesn't lose any functionality, but it is the one difference between this new and the existing syntax.

When creating the underlying webhook source, does that allow you to specify a specific cluster?

Actually maybe I don't understand how webhooks work with this new model. Do they have both a source and a table or just a table?

@ParkMyCar
Copy link
Member Author

Actually maybe I don't understand how webhooks work with this new model. Do they have both a source and a table or just a table?

Just a table, FWIW the inability to specify a cluster for CREATE TABLE ... FROM WEBHOOK is something I checked on with @morsapaes and she's okay with too

Copy link
Contributor

@jkosh44 jkosh44 left a comment

Choose a reason for hiding this comment

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

LGTM

@ParkMyCar ParkMyCar merged commit e8e32f0 into MaterializeInc:main Dec 16, 2024
79 checks passed
ParkMyCar added a commit that referenced this pull request Dec 17, 2024
This PR fixes the `webhook.py` platform-check.

In #30821 I failed to
consider the scenario of when a platform check runs on a version <
v0.128.0 and we create the webhook source with the `CREATE SOURCE ...
FROM WEBHOOK`, and then later check with a version >= v0.128.0, like our
Nightly tests do.

This PR changes the check to only use the new `CREATE TABLE ... FROM
WEBHOOK` syntax if we're on a version >= `v0.128.0`

### Motivation

Fix Nightly

### Checklist

- [ ] This PR has adequate test coverage / QA involvement has been duly
considered. ([trigger-ci for additional test/nightly
runs](https://trigger-ci.dev.materialize.com/))
- [ ] This PR has an associated up-to-date [design
doc](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/README.md),
is a design doc
([template](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/00000000_template.md)),
or is sufficiently small to not require a design.
  <!-- Reference the design in the description. -->
- [ ] If this PR evolves [an existing `$T ⇔ Proto$T`
mapping](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/command-and-response-binary-encoding.md)
(possibly in a backwards-incompatible way), then it is tagged with a
`T-proto` label.
- [ ] If this PR will require changes to cloud orchestration or tests,
there is a companion cloud PR to account for those changes that is
tagged with the release-blocker label
([example](MaterializeInc/cloud#5021)).
<!-- Ask in #team-cloud on Slack if you need help preparing the cloud
PR. -->
- [ ] If this PR includes major [user-facing behavior
changes](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/guide-changes.md#what-changes-require-a-release-note),
I have pinged the relevant PM to schedule a changelog post.
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.

2 participants