iffyio commented on code in PR #2035:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/2035#discussion_r2362158376
##########
src/parser/mod.rs:
##########
@@ -17463,40 +17471,67 @@ impl<'a> Parser<'a> {
_ => return self.expected("another option, EOF, Comma or ')'",
self.peek_token()),
};
}
- Ok(options)
+
+ Ok(KeyValueOptions { delimiter, options })
}
/// Parses a `KEY = VALUE` construct based on the specified key
pub(crate) fn parse_key_value_option(
&mut self,
- key: Word,
+ key: &Word,
) -> Result<KeyValueOption, ParserError> {
self.expect_token(&Token::Eq)?;
- match self.next_token().token {
- Token::SingleQuotedString(value) => Ok(KeyValueOption {
- option_name: key.value,
- option_type: KeyValueOptionType::STRING,
- value,
+ match self.peek_token().token {
+ Token::SingleQuotedString(_) => Ok(KeyValueOption {
+ option_name: key.value.clone(),
+ option_value:
KeyValueOptionKind::Single(self.parse_value()?.into()),
}),
Token::Word(word)
if word.keyword == Keyword::TRUE || word.keyword ==
Keyword::FALSE =>
{
Ok(KeyValueOption {
- option_name: key.value,
- option_type: KeyValueOptionType::BOOLEAN,
- value: word.value.to_uppercase(),
+ option_name: key.value.clone(),
+ option_value:
KeyValueOptionKind::Single(self.parse_value()?.into()),
})
}
- Token::Word(word) => Ok(KeyValueOption {
- option_name: key.value,
- option_type: KeyValueOptionType::ENUM,
- value: word.value,
- }),
- Token::Number(n, _) => Ok(KeyValueOption {
- option_name: key.value,
- option_type: KeyValueOptionType::NUMBER,
- value: n,
+ Token::Number(..) => Ok(KeyValueOption {
+ option_name: key.value.clone(),
+ option_value:
KeyValueOptionKind::Single(self.parse_value()?.into()),
}),
+ Token::Word(word) => {
+ self.next_token();
+ Ok(KeyValueOption {
+ option_name: key.value.clone(),
+ option_value:
KeyValueOptionKind::Single(Value::Placeholder(
+ word.value.clone(),
+ )),
+ })
+ }
+ Token::LParen => {
+ // Can be a list of values or a list of key value properties.
+ // Try to parse a list of values and if that fails, try to
parse
+ // a list of key-value properties.
+ match self.try_parse(|parser| {
Review Comment:
should this use `maybe_parse` instead of `try_parse` (noticed we're ignoring
the error from which on first glance looks unintentional)?
##########
src/ast/helpers/key_value_options.rs:
##########
@@ -41,6 +39,13 @@ pub struct KeyValueOptions {
pub delimiter: KeyValueOptionsDelimiter,
}
+impl KeyValueOptions {
+ /// Returns true iff the options list is empty
+ pub fn is_empty(&self) -> bool {
+ self.options.is_empty()
+ }
+}
+
Review Comment:
I'm thinking we could skip the added API and have the caller call
`value.options.is_empty()` instead? since this is only a thin wrapper might not
be worth it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
