Improve some code comments

Author: ggevay

src/catalog/src/builtin.rs

@@ -4386,7 +4386,7 @@ pub static MZ_STATEMENT_LIFECYCLE_HISTORY: LazyLock<BuiltinSource> = LazyLock::n
         is_retained_metrics_object: false,
         // TODO[btv]: Maybe this should be public instead of
         // `MONITOR_REDACTED`, but since that would be a backwards-compatible
-        // chagne, we probably don't need to worry about it now.
+        // change, we probably don't need to worry about it now.
         access: vec![
             SUPPORT_SELECT,
             ANALYTICS_SELECT,
@@ -9125,7 +9125,7 @@ pub static MZ_EXPECTED_GROUP_SIZE_ADVICE: LazyLock<BuiltinView> = LazyLock::new(
         -- such pattern, we look for how many levels can be eliminated without hitting a level
         -- that actually substantially filters the input. The advice is constructed so that
         -- setting the hint for the affected region will eliminate these redundant levels of
-        -- the hierachical rendering.
+        -- the hierarchical rendering.
         --
         -- A number of helper CTEs are used for the view definition. The first one, operators,
         -- looks for operator names that comprise arrangements of inputs to each level of a

src/expr/src/linear.rs

@@ -1502,8 +1502,8 @@ pub fn memoize_expr(
                     // updated if they reference a column reference themselves.
                     if *col > input_arity {
                         if let MirScalarExpr::Column(col2, _) = memoized_parts[*col - input_arity] {
-                            // We do _not_ propagate column names, since mis-associationg names and column
-                            // references will be very confusing (and possibly bug inducing).
+                            // We do _not_ propagate column names, since mis-associating names and column
+                            // references will be very confusing (and possibly bug-inducing).
                             *col = col2;
                         }
                     }

src/expr/src/scalar/func.rs

@@ -2198,6 +2198,9 @@ fn range_difference<'a>(
     propagates_nulls = true
 )]
 fn eq<'a>(a: Datum<'a>, b: Datum<'a>) -> Datum<'a> {
+    // SQL equality demands that if either input is null, then the result should be null. However,
+    // we don't need to handle this case here; it is handled when `BinaryFunc::eval` checks
+    // `propagates_nulls`.
     Datum::from(a == b)
 }
 

src/server-core/src/lib.rs

@@ -191,7 +191,7 @@ impl ConnectionUuidHandle {
         *self.0.lock().expect("lock poisoned") = Some(conn_uuid);
     }
 
-    /// Returns a displayble that renders a possibly missing connection UUID.
+    /// Returns a displayable that renders a possibly missing connection UUID.
     pub fn display(&self) -> impl fmt::Display {
         self.get().display_or("<unknown>")
     }

src/sql/src/func.rs

@@ -4956,6 +4956,7 @@ pub static OP_IMPLS: LazyLock<BTreeMap<&'static str, Func>> = LazyLock::new(|| {
         //   same type in planning. However, it's possible that we will perform
         //   equality on types not listed here (e.g. `Varchar`) due to decisions
         //   made in the optimizer.
+        // - Null inputs are handled by `BinaryFunc::eval` checking `propagates_nulls`.
         "=" => Scalar {
             params!(Numeric, Numeric) => BinaryFunc::Eq => Bool, 1752;
             params!(Bool, Bool) => BinaryFunc::Eq => Bool, 91;

src/sql/src/plan/hir.rs

@@ -236,12 +236,7 @@ pub enum HirScalarExpr {
     /// Given `expr` with arity 1. If expr returns:
     /// * 0 rows, return NULL
     /// * 1 row, return the value of that row
-    /// * >1 rows, the sql spec says we should throw an error but we can't
-    ///   (see <https://github.com/MaterializeInc/database-issues/issues/154>)
-    ///   so instead we return all the rows.
-    ///   If there are multiple `Select` expressions in a single SQL query, the result is that we take the product of all of them.
-    ///   This is counter to the spec, but is consistent with eg postgres' treatment of multiple set-returning-functions
-    ///   (see <https://tapoueh.org/blog/2017/10/set-returning-functions-and-postgresql-10/>).
+    /// * >1 rows, we return an error
     Select(Box<HirRelationExpr>, NameMetadata),
     Windowing(WindowExpr, NameMetadata),
 }

src/transform/src/column_knowledge.rs

@@ -783,8 +783,6 @@ fn optimize(
     #[allow(deprecated)]
     expr.visit_mut_pre_post(
         &mut |e| {
-            // We should not eagerly memoize `if` branches that might not be taken.
-            // TODO: Memoize expressions in the intersection of `then` and `els`.
             if let MirScalarExpr::If { then, els, .. } = e {
                 Some(vec![then, els])
             } else {

src/transform/src/dataflow.rs

@@ -99,6 +99,10 @@ pub fn optimize_dataflow(
         transform_ctx.df_meta,
     )?;
 
+    // Warning: If you want to add a transform call here, consider it very carefully whether it
+    // could accidentally invalidate information that we already derived above in
+    // `optimize_dataflow_monotonic` or `prune_and_annotate_dataflow_index_imports`.
+
     mz_repr::explain::trace_plan(dataflow);
 
     Ok(())

src/transform/src/normalize_ops.rs

@@ -41,7 +41,7 @@ impl crate::Transform for NormalizeOps {
                 crate::canonicalization::TopKElision::action(expr);
                 // (b) Fuse various like-kinded operators. Might enable further canonicalization.
                 crate::fusion::Fusion::action(expr);
-                // (c) Fuse join trees (might lift in-between Filters).
+                // (c) Fuse join trees (might lift in-between MFPs).
                 crate::fusion::join::Join::action(expr)?;
                 // (d) Extract column references in Map as Project.
                 crate::canonicalization::ProjectionExtraction::action(expr);

src/transform/src/typecheck.rs

@@ -78,7 +78,7 @@ pub enum TypeError<'a> {
         source: &'a MirRelationExpr,
         /// The column types we found (`sub` type)
         got: Vec<ColumnType>,
-        /// The solumn types we expected (`sup` type)
+        /// The column types we expected (`sup` type)
         expected: Vec<ColumnType>,
         /// The difference between these types
         diffs: Vec<RelationTypeDifference>,
@@ -460,7 +460,7 @@ impl Typecheck {
 
     /// New non-transient global IDs will be treated as an error
     ///
-    /// Only turn this on after the context has been appropraitely populated by, e.g., an earlier run
+    /// Only turn this on after the context has been appropriately populated by, e.g., an earlier run
     pub fn disallow_new_globals(mut self) -> Self {
         self.disallow_new_globals = true;
         self