iffyio commented on code in PR #1949:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1949#discussion_r2247708131
##########
src/dialect/mod.rs:
##########
@@ -1036,8 +1036,14 @@ pub trait Dialect: Debug + Any {
/// Returns true if the specified keyword should be parsed as a table
factor alias.
/// When explicit is true, the keyword is preceded by an `AS` word. Parser
is provided
/// to enable looking ahead if needed.
- fn is_table_factor_alias(&self, explicit: bool, kw: &Keyword, parser: &mut
Parser) -> bool {
- explicit || self.is_table_alias(kw, parser)
+ ///
+ /// When the dialect supports statements without semicolon delimiter,
actual keywords aren't parsed as aliases.
+ fn is_table_factor_alias(&self, explicit: bool, kw: &Keyword, _parser:
&mut Parser) -> bool {
+ if self.supports_statements_without_semicolon_delimiter() {
+ kw == &Keyword::NoKeyword
+ } else {
+ explicit || self.is_table_alias(kw, _parser)
+ }
Review Comment:
Not sure I followed the intent here, would you be able to provide example
sqls to illustrate and how they should be parsed with vs without the flag?
##########
src/parser/mod.rs:
##########
@@ -266,6 +266,22 @@ impl ParserOptions {
self.unescape = unescape;
self
}
+
+ /// Set if semicolon statement delimiters are required.
Review Comment:
Ah yeah the `with_trailing_commas` option was introduced as parser setting
originally and later had to be added as a dialect option since it was dialect
specific, I figure ideally it would have been present only on the dialect.
My current thinking is be that ideally we have one way to configure the
semicolon option since it would be nice to not have two knobs for the flag. In
terms of which way to configure, I'm guessing it could make more sense to have
the flag as a parser setting - i.e. whether or not semicolon is required is
more reflecting the tool that originally processed the input file, vs an aspect
of the sql language/dialect itself, so that it can differ in certain contexts
for the same dialect.
##########
src/parser/mod.rs:
##########
@@ -16464,7 +16505,28 @@ impl<'a> Parser<'a> {
/// Parse [Statement::Return]
fn parse_return(&mut self) -> Result<Statement, ParserError> {
- match self.maybe_parse(|p| p.parse_expr())? {
+ let rs = self.maybe_parse(|p| {
+ let expr = p.parse_expr()?;
+
+ match &expr {
+ Expr::Value(_)
+ | Expr::Function(_)
+ | Expr::UnaryOp { .. }
+ | Expr::BinaryOp { .. }
+ | Expr::Case { .. }
+ | Expr::Cast { .. }
+ | Expr::Convert { .. }
+ | Expr::Subquery(_) => Ok(expr),
+ // todo: how to retstrict to variables?
Review Comment:
@aharpervc in the example, is the ambiguity stemming from tsql not
supporting `IF` expressions but only `IF` statements? If so we could flag that
difference in the parser dialect, avoiding this diff. If it does support `IF`
statements then the grammar indeed looks either ambiguous or relying on the
newline character to differentiate between the two scenarios?
--
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
