sql: Change `Ident` to enforce a max length #23082

ParkMyCar · 2023-11-09T21:59:48Z

This PR adds a max length to the Ident struct. In #20999 we added the concept of a "max_identifier_length" which was enforced when parsing SQL, but it was not enforced for identifiers created internally, e.g. when generating a name for a progress subsource by appending "_progress" to the source name.

The largest changes in this PR are the following APIs:

Ident::new(...) will return an Err(IdentError) if the provided string is longer than our max, which is 255 bytes.
ident!("some static str") macro, which enforces our invariants at compile time. This prevents a pattern like Ident::new("some static str").expect("known correct") from polluting the code base.
Ident::new_unchecked(...) checks the invariants with a soft_assert!. This pattern is an escape hatch for cases we know are valid, but can't express in the type system, e.g. appending a number to a static string.

Wherever possible I used Ident::new(...) or ident!(...), but there were some callsites that didn't already return an error, and didn't have a &'static str as an argument. For those cases I used Ident::new_unchecked(...) to prevent this PR from getting too large. At the very least by using new_unchecked(...) our tests will catch invalid Idents via soft asserts.

Motivation

Fixes https://github.com/MaterializeInc/database-issues/issues/6813

But also has the goal of eliminating these kinds of bugs entirely. Chatted about this on Slack and it seemed desirable.

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:
- Internal only change

shepherdlybot · 2023-11-13T22:23:53Z

This PR has higher risk. Make sure to carefully review the file hotspots. In addition to having a knowledgeable reviewer, it may be useful to add observability and/or a feature flag. What's This?

Risk Score	Probability	Buggy File Hotspots
🔴 81 / 100	61%	13

Buggy File Hotspots:

File	Percentile
../plan/error.rs	90
../src/parser.rs	95
../src/util.rs	97
../src/rbac.rs	95
../session/vars.rs	99
../src/names.rs	90
../src/catalog.rs	95
../ast/transform.rs	92
../plan/statement.rs	92
../src/protocol.rs	93
../statement/dml.rs	93
../src/pure.rs	94
../statement/ddl.rs	98

jkosh44

This is awesome! Most of my comments are nits, but I had a couple of questions/comments.

jkosh44 · 2023-11-13T22:27:05Z

src/adapter/src/util.rs

+        name: Some(Ident::new_unchecked(index_name)),
        on_name: RawItemName::Name(mz_sql::normalize::unresolve(view_name)),
        in_cluster: Some(RawClusterName::Resolved(cluster_id.to_string())),
        key_parts: Some(
            keys.iter()
                .map(|i| match view_desc.get_unambiguous_name(*i) {
-                    Some(n) => Expr::Identifier(vec![Ident::new(n.to_string())]),
+                    Some(n) => Expr::Identifier(vec![Ident::new_unchecked(n.to_string())]),


We should probably create a follow-up GH issue to try and remove these types of new_unchecked uses. It's not obvious to me that these are guaranteed to fit.

Filed https://github.com/MaterializeInc/materialize/issues/23191 to track this. It's not obvious to me either, but with the soft_assert! in place, I feel much more confident that we'll atleast catch these issues earlier

jkosh44 · 2023-11-13T22:30:10Z

src/repr/src/relation.rs

@@ -339,6 +339,13 @@ impl RustType<ProtoColumnName> for ColumnName {
    }
 }

+impl From<ColumnName> for mz_sql_parser::ast::Ident {
+    fn from(value: ColumnName) -> Self {
+        // Note: ColumnNames are known to be less than the max length of an Ident (I think?).


Looking through the code base, I see 80 instances of initializing a ColumnName I have no idea if we check in all those places that the name is less than the max allowed characters. We probably want to do something similar to ColumnName where we add a constructor that checks the length. We should probably add a followup GH issue.

Filed https://github.com/MaterializeInc/materialize/issues/23192 to track this. I agree, no idea if we check all, or even any, ColumnNames to make sure they're under our max length. I figured starting with Ident was a good place though

jkosh44 · 2023-11-13T22:30:47Z

src/sql-lexer/src/lexer.rs

+/// Newtype wrapper around [`String`] whose _byte_ length is guaranteed to be less than or equal to
+/// [`MAX_IDENTIFIER_LENGTH`].
+#[derive(Debug, Clone, PartialEq)]
+


Did you mean to put this empty newline in?

Whoops, removed!

jkosh44 · 2023-11-13T22:32:46Z

src/sql-lexer/src/lexer.rs

+        if s.len() > MAX_IDENTIFIER_LENGTH {
+            return Err(s);
+        }
+
+        Ok(SmallString(s))


What's the reason for doing this check in both the Lexer and Parser?

The lexer is what creates Tokens, so it's cleanest to do the check there for SQL parsing. Then I was met with a dilemma about do we move the Ident struct one layer lower into mz_sql_lexer or create this second type?

We could probably move it one layer lower, and then re-export it from the sql-parser crate, but this felt a little cleaner? Not sure, what do you think?