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

catalog: Move builtin to catalog crate #21791

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

jkosh44
Copy link
Contributor

@jkosh44 jkosh44 commented Sep 18, 2023

This commit moves the mz_adapter::catalog::builtin module to mz_catalog::builtin which cleans up some duplicate structs.

Motivation

This PR refactors existing code.

Tips for Reviewer

A lot of tests were moved from builtin.rs to catalog.rs resulting in a large diff.

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered.
  • 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).
  • This PR includes the following user-facing behavior changes:
    • There are no user-facing behavior changes.

@jkosh44 jkosh44 changed the title catalog: Move builtin to catalog module catalog: Move builtin to catalog crate Sep 18, 2023
This commit moves the mz_adapter::catalog::builtin module to
mz_catalog::builtin which cleans up some duplicate structs.
@jkosh44 jkosh44 marked this pull request as ready for review September 18, 2023 01:29
@jkosh44 jkosh44 requested a review from a team as a code owner September 18, 2023 01:29
@shepherdlybot
Copy link

shepherdlybot bot commented Sep 18, 2023

This PR has higher risk. Make sure to carefully review the file hotspots. In addition to having this change reviewed, adequate tests should be considered and it may be useful to add observability and/or a feature flag. What's This?

Risk Score Probability Buggy File Hotspots
🔴 84 / 100 66% 5
Buggy File Hotspots:
File Percentile
../src/coord.rs 100
../src/catalog.rs 99
../src/builtin.rs 98
../sequencer/inner.rs 99
../catalog/builtin_table_updates.rs 97

Copy link
Member

@ParkMyCar ParkMyCar left a comment

Choose a reason for hiding this comment

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

+++++++-------- 🔥

@jkosh44 jkosh44 merged commit 9e3124c into MaterializeInc:main Sep 19, 2023
@jkosh44 jkosh44 deleted the catalog-builtins branch September 19, 2023 14:43
moulimukherjee pushed a commit to benesch/materialize that referenced this pull request Sep 19, 2023
* main: (24 commits)
  catalog: Remove stash details from transactions (MaterializeInc#21784)
  catalog: Move builtin to catalog crate (MaterializeInc#21791)
  sqlsmith/sqlancer: Fix mzcompose permissions
  storage: merge `Controller` with `StorageControllerState`
  canary environment: Add README.md
  canary environment: Target the production cloud
  transform: add `ColumnNames` attribute
  transform: pass context to attribute constructors
  transform: struct-based `DerivedAttributes` container
  transform: add `explain` directive to `test_transforms`
  transform: add column names support in `DefSource` spec blocks
  LICENSE: update change date
  doc: add inherited tables to PG source known limitations
  persist: split out a maelstrom-persist-base mzimage
  Make output of lint-docs-catalog CI-friendly
  adapter: Apply write ts in bootstrapping (MaterializeInc#21802)
  adapter: add more LaunchDarkly contexts
  Correct CREATE CLUSTER example syntax (MaterializeInc#21800)
  rbac: allow `EXPLAIN` commands in `user_privilege_hack`
  mzcompose: Allow the dbt service to accept environment variables
  ...
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