iffyio commented on code in PR #1487:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1487#discussion_r1824201506
##########
tests/sqlparser_hive.rs:
##########
@@ -534,6 +534,20 @@ fn parse_use() {
);
}
+#[test]
+fn test_show() {
+ hive_and_generic().verified_stmt("SHOW DATABASES");
+ hive_and_generic().verified_stmt("SHOW DATABASES LIKE '%abc'");
+ hive_and_generic().verified_stmt("SHOW SCHEMAS");
+ hive_and_generic().verified_stmt("SHOW SCHEMAS LIKE '%abc'");
+ hive_and_generic().verified_stmt("SHOW TABLES");
+ hive_and_generic().verified_stmt("SHOW TABLES IN db1");
+ hive_and_generic().verified_stmt("SHOW TABLES IN db1 'abc'");
Review Comment:
for tables/views in addition to `IN` can we also include testcases for
`FROM`?
##########
tests/sqlparser_hive.rs:
##########
@@ -534,6 +534,20 @@ fn parse_use() {
);
}
+#[test]
Review Comment:
thinking we can move this test over to common? the current code isn't
specific to hive and the feature/syntax seems popular enough that the parser
can always accept it as it does today
##########
src/ast/mod.rs:
##########
@@ -2782,12 +2783,30 @@ pub enum Statement {
filter: Option<ShowStatementFilter>,
},
/// ```sql
+ /// SHOW DATABASES [LIKE 'pattern']
+ /// ```
+ ShowDatabases { pattern: Option<String> },
Review Comment:
Would it make sense to also use `ShowStatementFilter` to represent this (if
there's a chance that the syntax can be extended)?
##########
src/ast/mod.rs:
##########
@@ -4376,7 +4410,31 @@ impl fmt::Display for Statement {
full = if *full { "FULL " } else { "" },
)?;
if let Some(db_name) = db_name {
- write!(f, " FROM {db_name}")?;
+ let keyword = match &db_name_keyword {
+ Some(Keyword::FROM) => "FROM",
+ Some(Keyword::IN) => "IN",
+ _ => unreachable!(),
+ };
+ write!(f, " {} {db_name}", keyword)?;
+ }
Review Comment:
I think representation wise we can represent the IN/FROM explicitly? e.g.
```rust
enum ShowClause {
IN,
FROM
}
Statement::ShowViews{ clause: Option<ShowClause>, db_name: Option<Ident> }
```
The unreachable panic I don't think fits well here since there's no
invariant guarding that panic (the parser codepath is quite far away from this
and could possibly change subtly to trigger it), I think it'd be reasonable as
in the previous version to display an incorrect sql (which would be less
consequential given we have a bug vs than crash the user's service). Though in
this case likely we can rely on the compiler to make such a state unavoidable
via the enum route.
Also would it make more sense to skip the nested `if let` since the db_name
and db_name_keyword are set independently. as in
```rust
if let Some(db_name_keyword) = db_name_keyword {
write(f, " {db_name_keyword}")?;
}
if let Some(db_name) = db_name {
write(f, " {db_name})?;
}
```
--
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
