Skip to content

sql-parser: quote bracketed ids that need it on display #39749

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bosconi wants to merge 1 commit into
base: main
Choose a base branch
from
Open

sql-parser: quote bracketed ids that need it on display #39749

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 27 additions & 13 deletions src/sql-parser/src/ast/metadata.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,23 +107,33 @@ impl RawItemName {
}
}

/// Writes the id of a bracketed raw name (`[<id> AS …]`, `[<id>]`) so that it
/// reparses to the same string.
///
/// The parser takes only an identifier token there (and, when `allow_number`,
/// a number token), so a crafted id with spaces or any keyword (e.g. `row`,
/// which lexes as a keyword token) must be quoted. Any keyword counts, not
/// only the reserved ones `Ident::can_be_printed_bare` checks. A normal global
/// id like `u1` is printed bare in every mode — including the stable mode
/// `pg_get_viewdef` renders in, which would otherwise force-quote it.
fn write_bracketed_id<W: fmt::Write>(f: &mut AstFormatter<W>, id: &str, allow_number: bool) {

@ggevay ggevay Oct 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simpler: drop Token::Number from parse_raw_ident_str instead of handling it here. It's left over from integer cluster ids (IN CLUSTER [1], 395050f). ClusterId::from_str now needs an s/u prefix, so a numeric cluster id never resolves, and only three testdata/ddl cases use one. (Network policies resolve the bracket text as a name, and ["1"] still covers that.) Then this becomes:

fn write_bracketed_id<W: fmt::Write>(f: &mut AstFormatter<W>, id: &str) {
    let ident = Ident::new_unchecked(id);
    if ident.has_only_bare_chars() && ident.as_keyword().is_none() {
        f.write_str(id);
    } else {
        f.write_str(ident.to_ast_string_stable());
    }
}

That also fixes the bot's finding: new_unchecked runs before the number check, so IN CLUSTER [<300 digits>] hits the 255-byte soft assert. Identifier tokens are capped at 255 bytes.

Also, the comment on the empty-id check in parse_raw_ident_str is stale with this change ([""] would now print as [""]). Please keep the check, but give the reason that parse_identifier rejects zero-length identifiers everywhere else, and let parse_raw_name reuse parse_raw_ident_str so item ids get it too.

let ident = Ident::new_unchecked(id);
let is_number = !id.is_empty() && id.chars().all(|ch| ch.is_ascii_digit());
if (ident.has_only_bare_chars() && ident.as_keyword().is_none()) || (allow_number && is_number)
{
f.write_str(id);
} else {
f.write_str(ident.to_ast_string_stable());
}
}

impl AstDisplay for RawItemName {
fn fmt<W: fmt::Write>(&self, f: &mut AstFormatter<W>) {
match self {
RawItemName::Name(o) => f.write_node(o),
RawItemName::Id(id, o, v) => {
// `id` is parsed from an identifier token (`[<id> AS …]`), so a
// crafted id with spaces/keywords must be quoted to reparse as a
// single identifier. A normal global id like `u1` is printed
// bare in every mode — including the stable mode `pg_get_viewdef`
// renders in, which would otherwise force-quote it.
f.write_str("[");
let id = Ident::new_unchecked(id.clone());
if id.can_be_printed_bare() {
f.write_str(id.as_str());
} else {
f.write_node(&id);
}
write_bracketed_id(f, id, false);
f.write_str(" AS ");
f.write_node(o);
if let Some(v) = v {
Expand Down Expand Up @@ -162,7 +172,9 @@ impl AstDisplay for RawClusterName {
match self {
RawClusterName::Unresolved(id) => f.write_node(id),
RawClusterName::Resolved(id) => {
f.write_str(format!("[{}]", id));
f.write_str("[");
write_bracketed_id(f, id, true);
f.write_str("]");
}
}
}
Expand Down Expand Up @@ -194,7 +206,9 @@ impl AstDisplay for RawNetworkPolicyName {
match self {
RawNetworkPolicyName::Unresolved(id) => f.write_node(id),
RawNetworkPolicyName::Resolved(id) => {
f.write_str(format!("[{}]", id));
f.write_str("[");
write_bracketed_id(f, id, true);
f.write_str("]");
}
}
}
Expand Down
39 changes: 39 additions & 0 deletions src/sql-parser/tests/testdata/id
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,3 +87,42 @@ SELECT * FROM [ü1 AS materialize.public.foo]
SELECT * FROM ["ü1" AS materialize.public.foo]
=>
Select(SelectStatement { query: Query { ctes: Simple([]), body: Select(Select { distinct: None, projection: [Wildcard], from: [TableWithJoins { relation: Table { name: Id("ü1", UnresolvedItemName([Ident("materialize"), Ident("public"), Ident("foo")]), None), alias: None }, joins: [] }], selection: None, group_by: [], having: None, qualify: None, options: [] }), order_by: [], limit: None, offset: None }, as_of: None })

# An id that is a keyword must stay quoted when displayed: `[<id> AS …]` only
# accepts an identifier token, so a bare keyword id fails to reparse
# (SQL-769). Applies to every position that takes a raw item name, including a
# cast's type.

@ggevay ggevay Oct 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add it to the comment that this is just a fuzzer-only requirement, real queries can't hit it.

parse-statement
SELECT * FROM ["row" AS materialize.public.foo]
----
SELECT * FROM ["row" AS materialize.public.foo]
=>
Select(SelectStatement { query: Query { ctes: Simple([]), body: Select(Select { distinct: None, projection: [Wildcard], from: [TableWithJoins { relation: Table { name: Id("row", UnresolvedItemName([Ident("materialize"), Ident("public"), Ident("foo")]), None), alias: None }, joins: [] }], selection: None, group_by: [], having: None, qualify: None, options: [] }), order_by: [], limit: None, offset: None }, as_of: None })

parse-statement
SELECT x::["row" AS materialize.public.foo]
----
SELECT x::["row" AS materialize.public.foo]
=>
Select(SelectStatement { query: Query { ctes: Simple([]), body: Select(Select { distinct: None, projection: [Expr { expr: Cast { expr: Identifier([Ident("x")]), data_type: Other { name: Id("row", UnresolvedItemName([Ident("materialize"), Ident("public"), Ident("foo")]), None), typ_mod: [] } }, alias: None }], from: [], selection: None, group_by: [], having: None, qualify: None, options: [] }), order_by: [], limit: None, offset: None }, as_of: None })

parse-statement roundtrip
SELECT listbar::["row" AS linger.over]::[aabortkafka AS linger.over] = cte
----
SELECT listbar::["row" AS linger.over]::[aabortkafka AS linger.over] = cte

# The same applies to the `[<id>]` resolved cluster and network policy names,
# which accept an identifier or a number token. A numeric id stays bare.
parse-statement
CREATE INDEX i IN CLUSTER ["row"] ON t (a)
----
CREATE INDEX i IN CLUSTER ["row"] ON t (a)
=>
CreateIndex(CreateIndexStatement { name: Some(Ident("i")), in_cluster: Some(Resolved("row")), on_name: Name(UnresolvedItemName([Ident("t")])), key_parts: Some([Identifier([Ident("a")])]), with_options: [], if_not_exists: false })

parse-statement
COMMENT ON NETWORK POLICY ["row"] IS 'x'
----
COMMENT ON NETWORK POLICY ["row"] IS 'x'
=>
Comment(CommentStatement { object: NetworkPolicy { name: Resolved("row") }, comment: Some("x") })
Loading