Add REFRESH options for MVs -- parsing, purification, planning #23870

ggevay · 2023-12-13T20:05:16Z

This is the parsing, purification, planning of REFRESH options on materialized views. This is the full PR, which includes the Compute part.

Design document.

Motivation

Tips for reviewer

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:

shepherdlybot · 2023-12-13T20:53:15Z

Mitigations

Completing required mitigations increases Resilience Coverage.

(Required) Code Review 🔍 Detected
(Required) Feature Flag 🔍 Detected
(Required) Integration Test 🔍 Detected
(Required) Observability
(Required) QA Review
Unit Test

Risk Summary:

The pull request carries a high risk score of 80, which is significantly influenced by the predictors: the sum of bug reports in the files changed and the delta of executable lines. Historically, pull requests with these characteristics are 128% more likely to introduce a bug compared to the repository baseline. Additionally, there are 13 files modified that have shown a high frequency of bug fixes recently, which further contributes to the risk. The repository's predicted bug trend indicates a decrease but with a recent increase in riskier pull requests, while the observed bug trend shows a general decrease with occasional spikes in bug fixes.

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:
What's This?

File	Percentile
../catalog/builtin_table_updates.rs	95
../src/parser.rs	98
../catalog/state.rs	94
../catalog/migrate.rs	93
../session/vars.rs	98
../src/catalog.rs	100
../src/names.rs	91
../src/catalog.rs	93
../catalog/open.rs	93
../plan/statement.rs	94
../coord/command_handler.rs	94
../src/pure.rs	99
../statement/ddl.rs	97

src/sql-parser/src/ast/defs/statement.rs

src/sql-parser/src/parser.rs

src/adapter/src/coord/command_handler.rs

src/sql/src/plan/statement/ddl.rs

benesch

The code here looks really quite solid. I left you a bunch of comments around parser structure and error message style that should be straightforward to fix and don't fundamentally alter the nature of the PR.

There are two harder comments:

I'd love to try to simplify the purification plumbing—specifically, reusing the existing purify_statement method to avoid introducing materialized view-specific public API to the sql crate. ISTM this might be straightforward by not caring about non-EpochMillis timelines. Holler if that's not the case.
Seeing Joe's comment about DST with intervals and your comment about monthly intervals makes me nervous about supporting REFRESH EVERY. I think we should have a quick discussion on the Slack, wherever makes sense, about whether we should change gears and try REFRESH ON SCHEDULE instead.

src/repr/src/lib.rs

src/sql-parser/src/ast/defs/statement.rs

test/sqllogictest/materialized_views.slt

src/sql/src/plan/statement/ddl.rs

src/sql/src/pure.rs

src/sql/src/plan/statement.rs

src/sql/src/plan/statement/ddl.rs

src/repr/src/refresh_schedule.rs

src/adapter/src/catalog/migrate.rs

src/adapter/src/coord/command_handler.rs

src/sql-parser/src/ast/defs/statement.rs

src/sql/src/catalog.rs

src/sql/src/plan/with_options.rs

src/sql-parser/src/ast/defs/value.rs

src/sql-parser/src/ast/defs/statement.rs

src/sql/src/plan/with_options.rs

src/sql/src/plan/statement/ddl.rs

src/sql/src/pure.rs

WITH option planning previously ignored interval modifiers; at variance from how intervals were planned when used in expressions. This commit refactors the interval planning code into a helper method that is shared by both expression planning and WITH option planning. This commit also introduces a new `TryFromValue` implementation targeting `Interval`, to tee up the planning of the forthcoming `REFRESH` option (MaterializeInc#23870), which wants the interval as an `Interval` rather than as a `Duration`.

src/sql/src/plan/statement/ddl.rs

src/adapter/src/coord/command_handler.rs

jkosh44 · 2023-12-26T15:14:58Z

I've completely lost track of the details for this PR. Is there any specific questions/things I'm supposed to look at?

ggevay · 2024-01-02T19:12:52Z

This is ready for a hopefully final round of review.

@benesch, could you please check #23870 (comment) ?

@jkosh44, I think I addressed all your comments.

Thanks a lot for the reviews!

(New Nightly run: https://buildkite.com/materialize/nightlies/builds/5828)

jkosh44 · 2024-01-02T19:33:07Z

src/expr/src/refresh_schedule.rs

+
+use mz_repr::Timestamp;
+use serde::{Deserialize, Serialize};
+use std::time::Duration;


We usually separate the std imports into their own block.

Yes, sorry, fixed!

Super minor, but I think this is still outstanding!

Sorry, I resolved it locally, just haven't pushed it yet, because I was waiting for a Nightly.

Edit: Pushed it now.

src/sql-parser/src/ast/defs/statement.rs

src/sql/src/plan/statement/ddl.rs

benesch · 2024-01-03T06:12:55Z

src/sql/src/plan/statement/ddl.rs

+        // Note: Seeing no REFRESH options at all (not even REFRESH ON COMMIT) should be acceptable:
+        // even though purification inserts REFRESH ON COMMIT if no other REFRESH option was given,
+        // we can't rely on this behavior in planning, because this won't happen for old
+        // materialized views that were created before this feature was introduced.


Don't block on this, but .. won't the migration ensure that even old matviews get REFRESH ON COMMIT before they ever hit planning?

Ah yes, thx, I forgot to delete this comment when I added the migration. I'll delete it before merging the PR.

benesch · 2024-01-03T06:14:46Z

src/adapter/src/coord/command_handler.rs

+                    let timeline_context =
+                        match self.validate_timeline_context(resolved_ids.0.clone()) {
+                            Ok(tc) => tc,
+                            Err(e) => return ctx.retire(Err(e)),
+                        };
+                    let timeline = match timeline_context {
+                        TimelineContext::TimelineDependent(timeline) => timeline,
+                        TimelineContext::TimestampDependent
+                        | TimelineContext::TimestampIndependent => {
+                            // We default to EpochMilliseconds, similarly to `determine_timestamp_for`.
+                            // Note that we didn't accurately decide whether we are TimestampDependent
+                            // or TimestampIndependent, because for this we'd need to also check whether
+                            // `query.contains_temporal()`, similarly to how `peek_stage_validate` does.
+                            // However, this doesn't matter here, as we are just going to default to
+                            // EpochMilliseconds in both cases.
+                            Timeline::EpochMilliseconds
+                        }
+                    };
+                    Some(self.get_timestamp_oracle(&timeline).read_ts().await)
+                    // TODO: It might be good to take into account `least_valid_read` in addition to
+                    // the oracle's `read_ts`, but there are two problems:
+                    // 1. At this point, we don't know which indexes would be used. We could do an
+                    // overestimation here by grabbing the ids of all indexes that are on ids
+                    // involved in the query. (We'd have to recursively follow view definitions,
+                    // similarly to `validate_timeline_context`.)
+                    // 2. For a peak, when the `least_valid_read` is later than the oracle's
+                    // `read_ts`, then the peak doesn't return before it completes at the chosen
+                    // timestamp. However, for a CRATE MATERIALIZED VIEW statement, it's not clear
+                    // whether we want to make it block until the chosen time. If it doesn't block,
+                    // then the initial refresh wouldn't be linearized with the CREATE MATERIALIZED
+                    // VIEW statement.
+                    //
+                    // Note: The Adapter is usually keeping a read hold of all objects at the oracle
+                    // read timestamp, so `least_valid_read` usually won't actually be later than
+                    // the oracle's `read_ts`. (see `Coordinator::advance_timelines`)
+                    //
+                    // Note 2: If we choose a timestamp here that is earlier than
+                    // `least_valid_read`, that is somewhat bad, but not catastrophic: The only
+                    // bad thing that happens is that we won't perform that refresh that was
+                    // specified to be at `mz_now()` (which is usually the initial refresh)
+                    // (similarly to how we don't perform refreshes that were specified to be in the
+                    // past).
+                } else {
+                    None
+                };


Ack, works for me.

philip-stoev

All the newly-introduced syntax needs to be in a Platform Checks test, so that its preservation across restarts and upgrades can be validated.

ggevay · 2024-01-03T09:29:27Z

@philip-stoev, I'm planning to add more tests later (e.g. with the follow-up pull request that handles the Compute part of the new syntax). I'll make sure to include Platform Checks! Note that the feature is behind a feature flag, which will definitely be disabled for now, because this current PR implements only the Adapter part of the feature.

I'd like to merge this current PR as soon as possible, because I've been getting an insane amount of merge conflicts.

Edit: Discussed with Philip on slack.

Will add tests later as explained here: #23870 (comment)

ggevay · 2024-01-03T11:31:14Z

Merging! Thank you very much for all the reviews and discussions!

ggevay mentioned this pull request Dec 13, 2023

REFRESH options for MVs -- Compute #23819

Merged

5 tasks

ggevay added the A-ADAPTER Topics related to the ADAPTER layer label Dec 13, 2023

ggevay marked this pull request as ready for review December 13, 2023 20:50

ggevay requested a review from a team as a code owner December 13, 2023 20:51

ggevay requested a review from a team December 13, 2023 20:51

ggevay requested review from a team as code owners December 13, 2023 20:51

ggevay requested review from ParkMyCar and jkosh44 and removed request for ParkMyCar December 13, 2023 20:51

jkosh44 reviewed Dec 13, 2023

View reviewed changes

benesch reviewed Dec 14, 2023

View reviewed changes

src/sql/src/plan/statement/ddl.rs Outdated Show resolved Hide resolved

src/sql/src/plan/statement/ddl.rs Outdated Show resolved Hide resolved

src/repr/src/refresh_schedule.rs Outdated Show resolved Hide resolved

ggevay force-pushed the refresh-every-parsing-planning branch 7 times, most recently from 909ca77 to 0814846 Compare December 20, 2023 13:42

jkosh44 reviewed Dec 20, 2023

View reviewed changes

ggevay force-pushed the refresh-every-parsing-planning branch 2 times, most recently from fbf3aa0 to 7ed1020 Compare December 22, 2023 14:29

benesch reviewed Dec 25, 2023

View reviewed changes

benesch mentioned this pull request Dec 25, 2023

sql: make interval planning consistent across WITH options and expressions #24103

Merged

5 tasks

benesch reviewed Dec 25, 2023

View reviewed changes

src/sql/src/plan/statement/ddl.rs Outdated Show resolved Hide resolved

src/adapter/src/coord/command_handler.rs Outdated Show resolved Hide resolved

src/adapter/src/coord/command_handler.rs Show resolved Hide resolved

ggevay force-pushed the refresh-every-parsing-planning branch 5 times, most recently from a7260a9 to ae67d41 Compare January 2, 2024 19:01

jkosh44 approved these changes Jan 2, 2024

View reviewed changes

hlburak mentioned this pull request Jan 2, 2024

[Epic] REFRESH options with manual cluster scheduling #22878

Closed

benesch approved these changes Jan 3, 2024

View reviewed changes

philip-stoev previously requested changes Jan 3, 2024

View reviewed changes

ggevay added 2 commits January 3, 2024 11:33

Add a wrapper for get_system_type in SessionCatalog

7dc21c3

Add REFRESH options for MVs -- parsing, purification, planning

2b6291d

ggevay force-pushed the refresh-every-parsing-planning branch from a478f5f to 2b6291d Compare January 3, 2024 11:23

ggevay enabled auto-merge (rebase) January 3, 2024 11:26

ggevay merged commit 8f13eb8 into MaterializeInc:main Jan 3, 2024
68 checks passed

materialize-bot mentioned this pull request Jan 5, 2024

release: v0.82.0 required reviews #24230

Closed

9 tasks

ggevay mentioned this pull request Jan 15, 2024

REFRESH NEVER materialized views #23012

Closed

ggevay mentioned this pull request Mar 14, 2024

REFRESH options #26010

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add REFRESH options for MVs -- parsing, purification, planning #23870

Add REFRESH options for MVs -- parsing, purification, planning #23870

ggevay commented Dec 13, 2023 •

edited

shepherdlybot bot commented Dec 13, 2023 •

edited

benesch left a comment

jkosh44 commented Dec 26, 2023

ggevay commented Jan 2, 2024 •

edited

jkosh44 Jan 2, 2024

ggevay Jan 2, 2024

benesch Jan 3, 2024

ggevay Jan 3, 2024 •

edited

benesch Jan 3, 2024

ggevay Jan 3, 2024

benesch Jan 3, 2024

philip-stoev left a comment

ggevay commented Jan 3, 2024 •

edited

ggevay commented Jan 3, 2024

Add REFRESH options for MVs -- parsing, purification, planning #23870

Add REFRESH options for MVs -- parsing, purification, planning #23870

Conversation

ggevay commented Dec 13, 2023 • edited

Motivation

Tips for reviewer

Checklist

shepherdlybot bot commented Dec 13, 2023 • edited

Mitigations

benesch left a comment

Choose a reason for hiding this comment

jkosh44 commented Dec 26, 2023

ggevay commented Jan 2, 2024 • edited

jkosh44 Jan 2, 2024

Choose a reason for hiding this comment

ggevay Jan 2, 2024

Choose a reason for hiding this comment

benesch Jan 3, 2024

Choose a reason for hiding this comment

ggevay Jan 3, 2024 • edited

Choose a reason for hiding this comment

benesch Jan 3, 2024

Choose a reason for hiding this comment

ggevay Jan 3, 2024

Choose a reason for hiding this comment

benesch Jan 3, 2024

Choose a reason for hiding this comment

philip-stoev left a comment

Choose a reason for hiding this comment

ggevay commented Jan 3, 2024 • edited

ggevay commented Jan 3, 2024

ggevay commented Dec 13, 2023 •

edited

shepherdlybot bot commented Dec 13, 2023 •

edited

ggevay commented Jan 2, 2024 •

edited

ggevay Jan 3, 2024 •

edited

ggevay commented Jan 3, 2024 •

edited