Skip to content

Conversation

@martykulma
Copy link
Contributor

@martykulma martykulma commented Mar 26, 2025

Return appropriate error if user tries to create a single-output load generator with subsources (e.g. FOR ALL TABLES, FOR TABLES ..., FOR SCHEMAS ...)

Motivation

Fixes https://github.com/MaterializeInc/database-issues/issues/9028

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.

@martykulma martykulma force-pushed the maz-fix-singlesource-loadgen-panic branch from ba709c4 to 214f4a1 Compare March 26, 2025 16:59
@martykulma martykulma changed the title return a proper RelationDesc for subsource of single source loadgen Display error if user tries to create subsources for single-output load generators Mar 26, 2025
@martykulma martykulma force-pushed the maz-fix-singlesource-loadgen-panic branch 2 times, most recently from 88fe46c to b0361f9 Compare March 27, 2025 12:20
@martykulma martykulma marked this pull request as ready for review March 27, 2025 13:55
@martykulma martykulma requested a review from a team as a code owner March 27, 2025 13:55
@martykulma martykulma requested a review from ParkMyCar March 27, 2025 13:55
@martykulma martykulma force-pushed the maz-fix-singlesource-loadgen-panic branch from b0361f9 to 8e4bbd4 Compare April 22, 2025 11:31
@martykulma martykulma requested a review from aljoscha April 22, 2025 18:10
Copy link
Contributor

@aljoscha aljoscha left a comment

Choose a reason for hiding this comment

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

This does what it says on the tin, and the code is good!

@petrosagg might want to look at this because he's currently in the business of moving all our sources to only allow the CREATE TABLE FROM SOURCE ... syntax. Which I'm thinking currently doesn't work with these load generator sources, maybe?

@@ -0,0 +1,298 @@
# Copyright Materialize, Inc. and contributors. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

This one is longer because it first adds test coverage that wasn't there before, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, just keeping parity with testdrive, but if we don't need to, I'm happy to drop this.

@def-, should I be adding this testing here, or is it overkill?

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't hurt much either way, but no need to add a separate file here if there is nothing interesting being exercised. All of these old-kafka-src-syntax tests was supposed to be long removed, but then the new CREATE TABLE ... FROM SOURCE syntax work was paused for a while.

@martykulma martykulma requested a review from def- April 23, 2025 18:39
@petrosagg
Copy link
Contributor

Disallowing this syntax in the current version of the product is inline with the migration so no concerns from me

@@ -0,0 +1,298 @@
# Copyright Materialize, Inc. and contributors. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't hurt much either way, but no need to add a separate file here if there is nothing interesting being exercised. All of these old-kafka-src-syntax tests was supposed to be long removed, but then the new CREATE TABLE ... FROM SOURCE syntax work was paused for a while.

contains:FOR SCHEMAS

! CREATE SOURCE g FROM LOAD GENERATOR DATUMS FOR SCHEMAS ("foo");
contains:FOR SCHEMAS
Copy link
Contributor

Choose a reason for hiding this comment

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

could be more verbose with what the error message is

@martykulma martykulma force-pushed the maz-fix-singlesource-loadgen-panic branch from 6c26256 to b25324c Compare April 24, 2025 20:12
@martykulma martykulma merged commit 80f1c30 into main Apr 24, 2025
82 checks passed
@martykulma martykulma deleted the maz-fix-singlesource-loadgen-panic branch April 24, 2025 20:59
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.

5 participants