-
Notifications
You must be signed in to change notification settings - Fork 468
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
storage/adapter: Opt-in migration of sources to the new table model #30483
base: main
Are you sure you want to change the base?
Conversation
…sorting of item updates
…rce-table-migration # Conflicts: # src/adapter/src/catalog/apply.rs # src/adapter/src/catalog/migrate.rs # src/adapter/src/catalog/open.rs # src/catalog/src/durable/transaction.rs # src/sql/src/names.rs # test/testdrive-old-kafka-src-syntax/mzcompose.py
MitigationsCompleting required mitigations increases Resilience Coverage.
Risk Summary:The pull request has a high-risk score of 81, driven by predictors such as "Sum Bug Reports Of Files" and "Delta of Executable Lines." Historically, PRs with these predictors are 116% more likely to cause a bug than the repo baseline. Additionally, the repository's predicted bug trend is increasing, which aligns with the current observed trend. Note: The risk score is not based on semantic analysis but on historical predictors of bug occurrence in the repository. The attributes above were deemed the strongest predictors based on that history. Predictors and the score may change as the PR evolves in code, time, and review activity. Bug Hotspots:
|
42c6af0
to
c505cfc
Compare
I've re-opened #30168 with my own fork to avoid CI issues. |
src/sql/src/names.rs
Outdated
if let RawItemName::Id(id, _, _) = item_name { | ||
let parsed_id = id.parse::<GlobalId>().unwrap(); | ||
self.ids.insert(parsed_id); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm skeptical that this is correct, because it will skip over all named references leading to a potentially incorrect order.
c285e6f
to
1972e06
Compare
1972e06
to
db98078
Compare
Motivation
The subsequent PR will implement https://github.com/MaterializeInc/database-issues/issues/8678, which will also disable use of the 'old style' source statements using the same feature-flag introduced here. Once this PR and that PR land, then enabling the
force_source_table_syntax
flag will completely switch over users to the new syntax.Tips for reviewer
To test this I've added a new scenario to
platform-checks
calledActivateSourceVersioningMigration
, that runs materialize on an existing version for each check'sinitialize()
method, and then restarts materialize on the latest version with theforce_source_table_syntax
, activating the migration of any sources created using the 'old style' syntax. Then thevalidate()
step is run on this new version, confirming that all the queries continue to work.There are already existing
platform-checks
Checks
that use the 'old style' source syntax:TableFromPgSource
,TableFromMySqlSource
,LoadGeneratorAsOfUpTo
, and one I added calledUpsertLegacy
, that cover the 4 source types we need to test. There are also many other checks that use the old syntax when running on 'old' versions before 0.119, but I wasn't sure how to make theActivateSourceVersioningMigration
scenario target a specific version rather than just the 'previous' version for the base run. @def- @nrainer-materialize let me know if you have ideas on doing that.I've also updated the
legacy upgrade tests
to activate this migration after the upgrade which should provide additional coverage too.Nightly
https://buildkite.com/materialize/nightly/builds?branch=rjobanp%3Asource-table-migration
Checklist
$T ⇔ Proto$T
mapping (possibly in a backwards-incompatible way), then it is tagged with aT-proto
label.