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

Populate envelope/key-format/value-format columns on mz_kafka_source_tables table #30076

Merged

Conversation

rjobanp
Copy link
Contributor

@rjobanp rjobanp commented Oct 17, 2024

Motivation

Now that each individual export (table) of a Kafka source can have it's own envelope and/or encoding specified, these columns don't make much sense on the mz_sources catalog table. Instead, we decided to port them to the mz_kafka_source_tables catalog table -- see discussion in the referenced issue.

Tips for reviewer

The large line diff in src/catalog/src/memory/objects.rs is just moving two methods from the Source type to the DataSourceDesc type for re-use

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.

@rjobanp rjobanp requested a review from a team as a code owner October 17, 2024 22:01
@rjobanp rjobanp requested review from jkosh44 and benesch October 17, 2024 22:01
@rjobanp rjobanp force-pushed the populate-kafka-source-table-cols branch from caac148 to 15d01b1 Compare October 17, 2024 22:37
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.

Code LGTM but I had two questions/comments

  • What's happening with these columns in mz_sources are we keeping them but always filling them in with NULL?
  • Can you update the docs for mz_kafka_source_tables.

@rjobanp
Copy link
Contributor Author

rjobanp commented Oct 18, 2024

What's happening with these columns in mz_sources are we keeping them but always filling them in with NULL?

Yes for now we will - I've put a task in the epic to remove those columns once we're fully migrated to the new model https://github.com/MaterializeInc/database-issues/issues/8677

Can you update the docs for mz_kafka_source_tables

Yes!

@rjobanp rjobanp force-pushed the populate-kafka-source-table-cols branch from 15d01b1 to 329a9f6 Compare October 18, 2024 15:16
@rjobanp rjobanp requested a review from a team as a code owner October 18, 2024 15:16
@jkosh44
Copy link
Contributor

jkosh44 commented Oct 18, 2024

What's happening with these columns in mz_sources are we keeping them but always filling them in with NULL?

Yes for now we will - I've put a task in the epic to remove those columns once we're fully migrated to the new model https://github.com/MaterializeInc/database-issues/issues/8677

Ah OK, we may want to hard code that into pack_source_update in builtin_table_updates.rs then instead of passing in an Option.

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

@rjobanp
Copy link
Contributor Author

rjobanp commented Oct 18, 2024

Ah OK, we may want to hard code that into pack_source_update in builtin_table_updates.rs then instead of passing in an Option.

Ah - to clarify - these columns will still be non-null for existing kafka sources that define the envelope and encoding directly on the main source object. However for 'new model' kafka sources, they will be null and the tables will have these values defined. Once we've migrated all sources to the new model then those columns can be removed

@jkosh44
Copy link
Contributor

jkosh44 commented Oct 18, 2024

Ah OK, we may want to hard code that into pack_source_update in builtin_table_updates.rs then instead of passing in an Option.

Ah - to clarify - these columns will still be non-null for existing kafka sources that define the envelope and encoding directly on the main source object. However for 'new model' kafka sources, they will be null and the tables will have these values defined. Once we've migrated all sources to the new model then those columns can be removed

Ok, I understand now. In that case it's good as is.

@rjobanp rjobanp merged commit 170fa64 into MaterializeInc:main Oct 18, 2024
81 checks passed
@rjobanp rjobanp deleted the populate-kafka-source-table-cols branch October 18, 2024 16:38
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