Skip to content

Fix placeholder spans #1979

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

Merged
iffyio merged 4 commits into from
Aug 1, 2025
Merged

Fix placeholder spans #1979

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c54bef6
Support for colon preceeded placeholders
xitep Jul 27, 2025
7932fc5
Revert "Support for colon preceeded placeholders"
xitep Jul 31, 2025
10cd9e2
Placeholder spans, strict parsing
xitep Jul 31, 2025
e63108b
Cover placeholder detection with tests
xitep Jul 31, 2025
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
6 changes: 6 additions & 0 deletions src/dialect/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -841,6 +841,12 @@ pub trait Dialect: Debug + Any {
false
}

/// Returns true if this dialect allow colon placeholders
/// e.g. `SELECT :var` (JPA named parameters)
fn supports_colon_placeholder(&self) -> bool {
false
}
Copy link
Copy Markdown
Contributor

@iffyio iffyio Jul 28, 2025

Choose a reason for hiding this comment

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

in fact, sqlparser's parser combines a colon token followed by a word token into a placeholder, but does not adjust the span of the resulting ast node :/ and allows for whitespace between the colon and the word.

@xitep could you elaborate on this part? I'm not sure I followed the intent, is my understanding correct that currently the parser indeed handles e.g. SELECT :var? If so what's the requirement for the tokenizer updates?

Copy link
Copy Markdown
Contributor Author

@xitep xitep Jul 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

hello @iffyio, yes the parser can deal with things like :var already, but i'm not sure this is a feature or rather just a side effect of the code pointed out above. here's an example:

    let q = sqlparser::parser::Parser::new(&sqlparser::dialect::GenericDialect)
        .try_with_sql("select :var")
        .unwrap()
        .parse_query()
        .unwrap();
    let _ = sqlparser::ast::visit_expressions(&q, |expr: &Expr| {
        if let Expr::Value(v) = expr
            && let Value::Placeholder(s) = &v.value
        {
            println!("value: {v:?}");
            println!("placeholder: {s:?})");
        }
        ControlFlow::<()>::Continue(())
    });
// OUTPUT:
// value: ValueWithSpan { value: Placeholder(":var"), span: Span(Location(1,8)..Location(1,9)) }
// placeholder: ":var")

note:

  1. the span is incorrect, it covers only the colon
  2. the placeholder represents :<word> (which in fact is what i'm after)

however, you'll get the same output when using "select : var" or even "select : /*foobar*/ var" as input :-/ this is not what i want.

so the intention is to make :<word> a first level token, a placeholder.

does this help?

Copy link
Copy Markdown
Contributor

@iffyio iffyio Jul 29, 2025

Choose a reason for hiding this comment

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

I think that make sense, it seems that the handling of placeholders isn't very consistent by the parser as some are done in tokenizer while others later on. I think it could be good to do a more general hopefully improvement if we feel that ideally placeholders are managed by the tokenizer, the we can parse and represent it consistently/accordingly.

Did a quick pass through existing support, and I can imagine something like the following representation to merge the different representations explicitly, wdyt?

enum PlaceholderKind {
	/// Example:
	/// ```sql
	/// SELECT ?, ?foo
	/// ```
	SingleQuestionMark,
	/// Example:
	/// ```sql
	/// SELECT @foo
	/// ```
	SingleAt,
	/// Example:
	/// ```sql
	/// SELECT $1, $foo
	/// ```
	SingleDollar,
	/// Example:
	/// ```sql
	/// SELECT :42, :foo
	/// ```
	Colon,
	/// Example:
	/// ```sql
	/// SELECT $$, $$abc$$, $abc$, $$$abc$$$
	/// ```
	DollarEnclosed {
		count: usize
	}
}

Token::Placeholder {
	kind: PlaceholderKind,
	value: Option<String>
}

It would also remove the need for this code as well

datafusion-sqlparser-rs/src/parser/mod.rs

Lines 9637 to 9646 in 15d8bfe

// Not calling self.parse_identifier(false)? because only in placeholder we want to check numbers as idfentifies
// This because snowflake allows numbers as placeholders
let next_token = self.next_token();
let ident = match next_token.token {
Token::Word(w) => Ok(w.into_ident(next_token.span)),
Token::Number(w, false) => Ok(Ident::new(w)),
_ => self.expected("placeholder", next_token),
}?;
let placeholder = tok.to_string() + &ident.value;
ok_value(Value::Placeholder(placeholder))

Copy link
Copy Markdown
Contributor Author

@xitep xitep Jul 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

ok, i can try it out and upgrade this PR accordingly. here's one thought to be clarified before i set off with it:

Token::Placeholder {
	kind: PlaceholderKind,
	value: Option<String>
}

this implies to me that "value" would be stripped off of the placeholder demarcation character(s); e.g. for the input ":foo" -> value would read Some("foo"), for the input "$$foo$$" it would read Some("foo"), for the input "$" value would be None, correct?

But then, what would sqlparser::ast::Value::Placeholder(..) look like? Clients surely might want to know exactly which placeholder style/kind was used; since sqlparser supports various formats, some clients might opt in to reject what they don't want their queries to contain.

all in all, it looks to me that in your proposal we want to end up with value being the original, full token as identified in the scanned input (which, naturally, is never None or empty), therefore:

Token::Placeholder {
	kind: PlaceholderKind,
	value: String,  // ~ preferrably &'a str
}

any feedback on this thought?

Copy link
Copy Markdown
Contributor

@iffyio iffyio Jul 30, 2025

Choose a reason for hiding this comment

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

Yeah that's correct that the input would be stripped into two, the kind and the value as in your examples.

Value::Placeholder I imagine would have the same representation mirroring Token::Placeholder since the goal becomes that placeholder parsing is handled by the tokenizer i.e.Value::Placeholder(Token::Placeholder)

which, naturally, is never None or empty

I think we might not always have a variable name, hence the Option - sqlite for example uses anonymous ? in some contexts for placeholders e.g. WHERE city IN (?, ?, ?, ?). In that scenario the representation becomes Token::Placeholder{kind: PlaceholderKind::SingleQuestionMark, value: None}

value: String, // ~ preferrably &'a str

ah heads up I think a reference &str wouldn't be feasible here without introducing lifetimes to the Token/Value enums which would be quite an invasive change and likely not worth it.

Copy link
Copy Markdown
Contributor Author

@xitep xitep Jul 30, 2025

Choose a reason for hiding this comment

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

wrt ...

	DollarEnclosed {
		count: usize
	}

i'm seeing "$$foo$$" being treated as a "double quoted string", not as a placeholder. if dialect.supports_dollar_placeholder() == true) then $... is considered a placeholder, but the dollars at the start and end don't have to be balanced. this make the DollarEnclosed variant essentially void.

Copy link
Copy Markdown
Contributor Author

@xitep xitep Jul 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

alright, after a lot of struggle i came to the conclusion that the approach with first-class placeholder is not feasible in sqlparser. After having the tokenizer and the model change ready, this test (now failing) convinced me, that my concern cannot be resolved at the level of the tokenizer since sqlparser needs to be very flexible and determine the meaning only on the level of the parser. you can have a look here at how the change we discussed would have looked like. (this would be the consequential change to the parser.)

fortunately, i have found a far smaller change that satisfies my needs and will update this PR.


/// Does the dialect support with clause in create index statement?
/// e.g. `CREATE INDEX idx ON t WITH (key = value, key2)`
fn supports_create_index_with_clause(&self) -> bool {
Expand Down
91 changes: 91 additions & 0 deletions src/tokenizer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1521,6 +1521,11 @@ impl<'a> Tokenizer<'a> {
match chars.peek() {
Some(':') => self.consume_and_return(chars, Token::DoubleColon),
Some('=') => self.consume_and_return(chars, Token::Assignment),
Some(c)
if self.dialect.supports_colon_placeholder() && c.is_alphabetic() =>
{
self.tokenize_colon_preceeded_placeholder(chars).map(Some)
}
_ => Ok(Some(Token::Colon)),
}
}
Expand Down Expand Up @@ -1756,6 +1761,30 @@ impl<'a> Tokenizer<'a> {
}
}

/// Tokenizes an identifier followed immediately after a colon,
/// aka named query parameter, e.g. `:name`. The next char of the
/// processed char stream is to be an alphabetic - panics otherwise.
fn tokenize_colon_preceeded_placeholder(
Copy link
Copy Markdown
Contributor

@iffyio iffyio Jul 29, 2025

Choose a reason for hiding this comment

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

I think that it would likely be ideal to use existing logic like tokenize_identifier_or_keyword or similar. Also heads up in general for the parser that returning an error when input is invalid etc is preferred over panicking

Copy link
Copy Markdown
Contributor Author

@xitep xitep Jul 30, 2025

Choose a reason for hiding this comment

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

  • wrt tokenize_identifier_or_keyword ... i guess it would make it quite hard to support the $ then as part of the identifier (if enabled by Dialect::supports_dollar_placeholder https://docs.rs/sqlparser/latest/sqlparser/dialect/trait.Dialect.html#method.supports_dollar_placeholder.) i see the placeholder identifiers distinct (ie. with different restrictions) from other SQL identifiers.
  • thanks for the heads-up; the panic here would be a programming error, as that contract says that the caller must make sure ... but I'm fine with the established convention and would make it return an error instead.

&self,
chars: &mut State,
) -> Result<Token, TokenizerError> {
let mut s = String::with_capacity(16);
s.push(':');
s.push(chars.next().expect("initial character missing"));
while let Some(&ch) = chars.peek() {
if ch.is_alphanumeric()
|| ch == '_'
|| matches!(ch, '$' if self.dialect.supports_dollar_placeholder())
{
s.push(ch);
chars.next();
} else {
break;
}
}
Ok(Token::Placeholder(s))
}

/// Tokenize dollar preceded value (i.e: a string/placeholder)
fn tokenize_dollar_preceded_value(&self, chars: &mut State) -> Result<Token, TokenizerError> {
let mut s = String::new();
Expand Down Expand Up @@ -2952,6 +2981,68 @@ mod tests {
);
}

#[test]
fn tokenize_colon_placeholder() {
#[derive(Debug)]
struct TestDialect(bool);
impl Dialect for TestDialect {
fn supports_colon_placeholder(&self) -> bool {
true
}
fn supports_dollar_placeholder(&self) -> bool {
self.0
}
fn is_identifier_start(&self, ch: char) -> bool {
ch.is_alphabetic() || ch == '_'
}
fn is_identifier_part(&self, ch: char) -> bool {
ch.is_alphabetic() || ch.is_ascii_digit() || ch == '_'
}
}

let sql = "SELECT :foo FROM bar";
let tokens = Tokenizer::new(&TestDialect(false), sql)
.tokenize_with_location()
.unwrap();
assert_eq!(
tokens.iter().map(|t| t.token.clone()).collect::<Vec<_>>(),
vec![
Token::make_keyword("SELECT"),
Token::Whitespace(Whitespace::Space),
Token::Placeholder(":foo".into()),
Token::Whitespace(Whitespace::Space),
Token::make_keyword("FROM"),
Token::Whitespace(Whitespace::Space),
Token::make_word("bar", None)
]
);
assert_eq!(
tokens[2].span,
Span::new(Location::of(1, 8), Location::of(1, 12))
);

let sql = "SELECT :foo$bar FROM bar";
let tokens = Tokenizer::new(&TestDialect(true), sql)
.tokenize_with_location()
.unwrap();
assert_eq!(
tokens.iter().map(|t| t.token.clone()).collect::<Vec<_>>(),
vec![
Token::make_keyword("SELECT"),
Token::Whitespace(Whitespace::Space),
Token::Placeholder(":foo$bar".into()),
Token::Whitespace(Whitespace::Space),
Token::make_keyword("FROM"),
Token::Whitespace(Whitespace::Space),
Token::make_word("bar", None)
]
);
assert_eq!(
tokens[2].span,
Span::new(Location::of(1, 8), Location::of(1, 16))
);
}

#[test]
fn tokenize_dollar_placeholder() {
let sql = String::from("SELECT $$, $$ABC$$, $ABC$, $ABC");
Expand Down