adapter/sources: Support custom timelines on source tables to handle CdCv2 envelope type correctly #30013
Conversation
| // Allow users to specify a timeline. If they do not, determine a default | ||
| // timeline for the source. | ||
| let timeline = match timeline { | ||
| None => match envelope { | ||
| SourceEnvelope::CdcV2 => { | ||
| Timeline::External(scx.catalog.resolve_full_name(&name).to_string()) | ||
| } | ||
| _ => Timeline::EpochMilliseconds, | ||
| }, | ||
| // TODO(benesch): if we stabilize this, can we find a better name than | ||
| // `mz_epoch_ms`? Maybe just `mz_system`? | ||
| Some(timeline) if timeline == "mz_epoch_ms" => Timeline::EpochMilliseconds, | ||
| Some(timeline) if timeline.starts_with("mz_") => { | ||
| return Err(PlanError::UnacceptableTimelineName(timeline)); | ||
| } | ||
| Some(timeline) => Timeline::User(timeline), | ||
| }; |
There was a problem hiding this comment.
this is the same logic as already exists in plan_create_source. It will be removed from that function once we drop support for outputting data to the primary source collection
…CdCv2 envelope type correctly
This reverts commit 9bfaac4.
rjobanp
force-pushed
the
source-table-timelines
branch
from
395421b to
be5fa3a
Compare
| @@ -99,7 +99,7 @@ idx_c FOR 240000 | |||
| # Test subsource propagation. Test sources with and without subsources and view dependencies to | |||
There was a problem hiding this comment.
changes in this file are a revert of 9bfaac4 which seemed to break the intent of the test (to validate that the retain history values are propagated from source -> subsources / tables)
| pub fn timeline(&self) -> Timeline { | ||
| Timeline::EpochMilliseconds |
There was a problem hiding this comment.
I'm honestly worried about how many places in the code blindly assumes that all tables are on the EpochMillisecond timeline. Nothing to do I suppose but see what breaks.
There was a problem hiding this comment.
Yeah I ran the nightly tests on this branch since I had a similar worry, but things seemed okay 🤷🏽
Motivation
The above bug demonstrated hung queries whenever using a
CREATE TABLE .. FROM SOURCE .. ENVELOPE MATERIALIZEstatement, sinceENVELOPE MATERIALIZE(also known asEnvelope::CdcV2) uses a non-default timeline by default.This PR introduces the ability to set a custom timeline on
CREATE TABLE .. FROM SOURCEand to correct the default timeline when usingENVELOPE MATERIALIZE, matching the existing timeline handling semantics ofCREATE SOURCEstatements.The tests for this are in @nrainer-materialize 's PR #29801 and I tested this change on his branch locally to verify the fix.
Tips for reviewer
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.