sql: make interval planning across WITH options and expressions

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.rs

@@ -67,6 +67,7 @@ use crate::names::{
 pub(crate) mod error;
 pub(crate) mod explain;
 pub(crate) mod expr;
+pub(crate) mod literal;
 pub(crate) mod lowering;
 pub(crate) mod notice;
 pub(crate) mod plan_utils;

src/sql/src/plan/literal.rs

@@ -0,0 +1,52 @@
+// Copyright Materialize, Inc. and contributors. All rights reserved.
+//
+// Use of this software is governed by the Business Source License
+// included in the LICENSE file.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0.
+
+use mz_repr::adt::interval::Interval;
+use mz_repr::strconv;
+use mz_sql_parser::ast::IntervalValue;
+
+use crate::plan::PlanError;
+
+pub fn plan_interval(iv: &IntervalValue) -> Result<Interval, PlanError> {
+    let leading_precision = parser_datetimefield_to_adt(iv.precision_high);
+    let mut i = strconv::parse_interval_w_disambiguator(
+        &iv.value,
+        match leading_precision {
+            mz_repr::adt::datetime::DateTimeField::Hour
+            | mz_repr::adt::datetime::DateTimeField::Minute => Some(leading_precision),
+            _ => None,
+        },
+        parser_datetimefield_to_adt(iv.precision_low),
+    )?;
+    i.truncate_high_fields(parser_datetimefield_to_adt(iv.precision_high));
+    i.truncate_low_fields(
+        parser_datetimefield_to_adt(iv.precision_low),
+        iv.fsec_max_precision,
+    )?;
+    Ok(i)
+}
+
+fn parser_datetimefield_to_adt(
+    dtf: mz_sql_parser::ast::DateTimeField,
+) -> mz_repr::adt::datetime::DateTimeField {
+    use mz_sql_parser::ast::DateTimeField::*;
+    match dtf {
+        Millennium => mz_repr::adt::datetime::DateTimeField::Millennium,
+        Century => mz_repr::adt::datetime::DateTimeField::Century,
+        Decade => mz_repr::adt::datetime::DateTimeField::Decade,
+        Year => mz_repr::adt::datetime::DateTimeField::Year,
+        Month => mz_repr::adt::datetime::DateTimeField::Month,
+        Day => mz_repr::adt::datetime::DateTimeField::Day,
+        Hour => mz_repr::adt::datetime::DateTimeField::Hour,
+        Minute => mz_repr::adt::datetime::DateTimeField::Minute,
+        Second => mz_repr::adt::datetime::DateTimeField::Second,
+        Milliseconds => mz_repr::adt::datetime::DateTimeField::Milliseconds,
+        Microseconds => mz_repr::adt::datetime::DateTimeField::Microseconds,
+    }
+}

src/sql/src/plan/query.rs

@@ -90,7 +90,7 @@ use crate::plan::typeconv::{self, CastContext};
 use crate::plan::with_options::TryFromValue;
 use crate::plan::PlanError::InvalidWmrRecursionLimit;
 use crate::plan::{
-    transform_ast, Params, PlanContext, QueryWhen, ShowCreatePlan, WebhookValidation,
+    literal, transform_ast, Params, PlanContext, QueryWhen, ShowCreatePlan, WebhookValidation,
     WebhookValidationSecret,
 };
 use crate::session::vars::{self, FeatureFlag};
@@ -5217,22 +5217,8 @@ fn plan_literal<'a>(l: &'a Value) -> Result<CoercibleScalarExpr, PlanError> {
             false => (Datum::False, ScalarType::Bool),
             true => (Datum::True, ScalarType::Bool),
         },
-        Value::Interval(iv) => {
-            let leading_precision = parser_datetimefield_to_adt(iv.precision_high);
-            let mut i = strconv::parse_interval_w_disambiguator(
-                &iv.value,
-                match leading_precision {
-                    mz_repr::adt::datetime::DateTimeField::Hour
-                    | mz_repr::adt::datetime::DateTimeField::Minute => Some(leading_precision),
-                    _ => None,
-                },
-                parser_datetimefield_to_adt(iv.precision_low),
-            )?;
-            i.truncate_high_fields(parser_datetimefield_to_adt(iv.precision_high));
-            i.truncate_low_fields(
-                parser_datetimefield_to_adt(iv.precision_low),
-                iv.fsec_max_precision,
-            )?;
+        Value::Interval(i) => {
+            let i = literal::plan_interval(i)?;
             (Datum::Interval(i), ScalarType::Interval)
         }
         Value::String(s) => return Ok(CoercibleScalarExpr::LiteralString(s.clone())),
@@ -5459,27 +5445,6 @@ fn window_frame_bound_ast_to_expr(bound: &WindowFrameBound) -> mz_expr::WindowFr
     }
 }
 
-// Implement these as two identical enums without From/Into impls so that they
-// have no cross-package dependencies, leaving that work up to this crate.
-fn parser_datetimefield_to_adt(
-    dtf: mz_sql_parser::ast::DateTimeField,
-) -> mz_repr::adt::datetime::DateTimeField {
-    use mz_sql_parser::ast::DateTimeField::*;
-    match dtf {
-        Millennium => mz_repr::adt::datetime::DateTimeField::Millennium,
-        Century => mz_repr::adt::datetime::DateTimeField::Century,
-        Decade => mz_repr::adt::datetime::DateTimeField::Decade,
-        Year => mz_repr::adt::datetime::DateTimeField::Year,
-        Month => mz_repr::adt::datetime::DateTimeField::Month,
-        Day => mz_repr::adt::datetime::DateTimeField::Day,
-        Hour => mz_repr::adt::datetime::DateTimeField::Hour,
-        Minute => mz_repr::adt::datetime::DateTimeField::Minute,
-        Second => mz_repr::adt::datetime::DateTimeField::Second,
-        Milliseconds => mz_repr::adt::datetime::DateTimeField::Milliseconds,
-        Microseconds => mz_repr::adt::datetime::DateTimeField::Microseconds,
-    }
-}
-
 pub fn scalar_type_from_sql(
     scx: &StatementContext,
     data_type: &ResolvedDataType,

src/sql/src/plan/with_options.rs

@@ -9,15 +9,16 @@
 
 //! Provides tooling to handle `WITH` options.
 
+use mz_repr::adt::interval::Interval;
 use mz_repr::{strconv, GlobalId};
 use mz_sql_parser::ast::{Ident, KafkaBroker, ReplicaDefinition};
 use mz_storage_types::connections::StringOrSecret;
 use serde::{Deserialize, Serialize};
 use std::time::Duration;
 
-use crate::ast::{AstInfo, IntervalValue, UnresolvedItemName, Value, WithOptionValue};
+use crate::ast::{AstInfo, UnresolvedItemName, Value, WithOptionValue};
 use crate::names::{ResolvedDataType, ResolvedItemName};
-use crate::plan::{Aug, PlanError};
+use crate::plan::{literal, Aug, PlanError};
 
 pub trait TryFromValue<T>: Sized {
     fn try_from_value(v: T) -> Result<Self, PlanError>;
@@ -162,21 +163,9 @@ impl ImpliedValue for StringOrSecret {
     }
 }
 
-// This conversion targets the standard library's `Duration` type rather than
-// the Materialize SQL-specific `Interval` type because, at the time of writing,
-// `Duration` was the desired Rust type for every interval-valued option.
-//
-// In the future, it would be reasonable to add an implementation that targets
-// `Interval` for options that want it as such, e.g., because they need to
-// support negative intervals.
 impl TryFromValue<Value> for Duration {
     fn try_from_value(v: Value) -> Result<Self, PlanError> {
-        let interval = match v {
-            Value::Interval(IntervalValue { value, .. })
-            | Value::Number(value)
-            | Value::String(value) => strconv::parse_interval(&value)?,
-            _ => sql_bail!("cannot use value as interval"),
-        };
+        let interval = Interval::try_from_value(v)?;
         Ok(interval.duration()?)
     }
     fn name() -> String {
@@ -190,6 +179,25 @@ impl ImpliedValue for Duration {
     }
 }
 
+impl TryFromValue<Value> for Interval {
+    fn try_from_value(v: Value) -> Result<Self, PlanError> {
+        match v {
+            Value::Interval(value) => literal::plan_interval(&value),
+            Value::Number(value) | Value::String(value) => Ok(strconv::parse_interval(&value)?),
+            _ => sql_bail!("cannot use value as interval"),
+        }
+    }
+    fn name() -> String {
+        "interval".to_string()
+    }
+}
+
+impl ImpliedValue for Interval {
+    fn implied_value() -> Result<Self, PlanError> {
+        sql_bail!("must provide an interval value")
+    }
+}
+
 #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Serialize, Hash, Deserialize)]
 pub struct OptionalDuration(pub Option<Duration>);