Skip to content

Add support for SHOW DATABASES/SCHEMAS/TABLES/VIEWS in Hive #1487

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
merged 6 commits into from
Nov 3, 2024
Merged

Add support for SHOW DATABASES/SCHEMAS/TABLES/VIEWS in Hive #1487

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4e67a52
Added support for SHOW DATABASES/SCHEMAS/VIEWS and extended SHOW TABL…
yoavcloud Oct 26, 2024
a5edbb7
Improve parsing for optional literal string without a keyword indication
yoavcloud Oct 30, 2024
9249aa3
Code review comments
yoavcloud Oct 30, 2024
febab0b
Remove redundant whitespace
yoavcloud Oct 30, 2024
407003a
Code review comments, also added support for materialized views
yoavcloud Oct 31, 2024
07a651a
formatting
yoavcloud Nov 1, 2024
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
68 changes: 64 additions & 4 deletions src/ast/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,9 @@ pub use self::value::{
TrimWhereField, Value,
};

use crate::ast::helpers::stmt_data_loading::{
DataLoadingOptions, StageLoadSelectItem, StageParamsObject,
use crate::{
ast::helpers::stmt_data_loading::{DataLoadingOptions, StageLoadSelectItem, StageParamsObject},
keywords::Keyword,
};
#[cfg(feature = "visitor")]
pub use visitor::*;
Expand Down Expand Up @@ -2782,12 +2783,30 @@ pub enum Statement {
filter: Option<ShowStatementFilter>,
},
/// ```sql
/// SHOW DATABASES [LIKE 'pattern']
/// ```
ShowDatabases { pattern: Option<String> },
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to also use ShowStatementFilter to represent this (if there's a chance that the syntax can be extended)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea

/// ```sql
/// SHOW SCHEMAS [LIKE 'pattern']
/// ```
ShowSchemas { pattern: Option<String> },
/// ```sql
/// SHOW TABLES
/// ```
/// Note: this is a MySQL-specific statement.
ShowTables {
extended: bool,
full: bool,
// The keyword used to indicate the DB name (IN/FROM)
db_name_keyword: Option<Keyword>,
db_name: Option<Ident>,
filter: Option<ShowStatementFilter>,
},
/// ```sql
/// SHOW VIEWS
/// ```
ShowViews {
// The keyword used to indicate the DB name (IN/FROM)
db_name_keyword: Option<Keyword>,
db_name: Option<Ident>,
filter: Option<ShowStatementFilter>,
},
Expand Down Expand Up @@ -4363,9 +4382,24 @@ impl fmt::Display for Statement {
}
Ok(())
}
Statement::ShowDatabases { pattern } => {
write!(f, "SHOW DATABASES")?;
if let Some(pattern) = pattern {
write!(f, " LIKE '{}'", pattern)?;
}
Ok(())
}
Statement::ShowSchemas { pattern } => {
write!(f, "SHOW SCHEMAS")?;
if let Some(pattern) = pattern {
write!(f, " LIKE '{}'", pattern)?;
}
Ok(())
}
Statement::ShowTables {
extended,
full,
db_name_keyword,
db_name,
filter,
} => {
Expand All @@ -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)?;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think representation wise we can represent the IN/FROM explicitly? e.g.

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

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})?;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good comments, with the ShowClause the unreachable dilemma is irrelevant

if let Some(filter) = filter {
write!(f, " {filter}")?;
}
Ok(())
}
Statement::ShowViews {
db_name_keyword,
db_name,
filter,
} => {
write!(f, "SHOW VIEWS")?;
if let Some(db_name) = db_name {
let keyword = match &db_name_keyword {
Some(Keyword::FROM) => "FROM",
Some(Keyword::IN) => "IN",
_ => unreachable!(),
};
write!(f, " {} {db_name}", keyword)?;
}
if let Some(filter) = filter {
write!(f, " {filter}")?;
Expand Down Expand Up @@ -6057,6 +6115,7 @@ pub enum ShowStatementFilter {
Like(String),
ILike(String),
Where(Expr),
NoKeyword(String),
}

impl fmt::Display for ShowStatementFilter {
Expand All @@ -6066,6 +6125,7 @@ impl fmt::Display for ShowStatementFilter {
Like(pattern) => write!(f, "LIKE '{}'", value::escape_single_quote_string(pattern)),
ILike(pattern) => write!(f, "ILIKE {}", value::escape_single_quote_string(pattern)),
Where(expr) => write!(f, "WHERE {expr}"),
NoKeyword(pattern) => write!(f, "'{}'", value::escape_single_quote_string(pattern)),
}
}
}
Expand Down
3 changes: 3 additions & 0 deletions src/keywords.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ define_keywords!(
CYCLE,
DATA,
DATABASE,
DATABASES,
DATA_RETENTION_TIME_IN_DAYS,
DATE,
DATE32,
Expand Down Expand Up @@ -662,6 +663,7 @@ define_keywords!(
SAFE_CAST,
SAVEPOINT,
SCHEMA,
SCHEMAS,
SCOPE,
SCROLL,
SEARCH,
Expand Down Expand Up @@ -822,6 +824,7 @@ define_keywords!(
VERSION,
VERSIONING,
VIEW,
VIEWS,
VIRTUAL,
VOLATILE,
WAREHOUSE,
Expand Down
55 changes: 50 additions & 5 deletions src/parser/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9579,6 +9579,8 @@ impl<'a> Parser<'a> {
Ok(self.parse_show_columns(extended, full)?)
} else if self.parse_keyword(Keyword::TABLES) {
Ok(self.parse_show_tables(extended, full)?)
} else if self.parse_keyword(Keyword::VIEWS) {
Ok(self.parse_show_views()?)
} else if self.parse_keyword(Keyword::FUNCTIONS) {
Ok(self.parse_show_functions()?)
} else if extended || full {
Expand All @@ -9605,13 +9607,35 @@ impl<'a> Parser<'a> {
session,
global,
})
} else if self.parse_keyword(Keyword::DATABASES) {
self.parse_show_databases()
} else if self.parse_keyword(Keyword::SCHEMAS) {
self.parse_show_schemas()
} else {
Ok(Statement::ShowVariable {
variable: self.parse_identifiers()?,
})
}
}

fn parse_show_databases(&mut self) -> Result<Statement, ParserError> {
let pattern = if self.parse_keyword(Keyword::LIKE) {
Some(self.parse_literal_string()?)
} else {
None
};
Ok(Statement::ShowDatabases { pattern })
}

fn parse_show_schemas(&mut self) -> Result<Statement, ParserError> {
let pattern = if self.parse_keyword(Keyword::LIKE) {
Some(self.parse_literal_string()?)
} else {
None
};
Ok(Statement::ShowSchemas { pattern })
}

pub fn parse_show_create(&mut self) -> Result<Statement, ParserError> {
let obj_type = match self.expect_one_of_keywords(&[
Keyword::TABLE,
Expand Down Expand Up @@ -9667,14 +9691,30 @@ impl<'a> Parser<'a> {
extended: bool,
full: bool,
) -> Result<Statement, ParserError> {
let db_name = match self.parse_one_of_keywords(&[Keyword::FROM, Keyword::IN]) {
Some(_) => Some(self.parse_identifier(false)?),
None => None,
};
let (db_name_keyword, db_name) =
match self.parse_one_of_keywords(&[Keyword::FROM, Keyword::IN]) {
Some(keyword) => (Some(keyword), Some(self.parse_identifier(false)?)),
None => (None, None),
};
let filter = self.parse_show_statement_filter()?;
Ok(Statement::ShowTables {
extended,
full,
db_name_keyword,
db_name,
filter,
})
}

fn parse_show_views(&mut self) -> Result<Statement, ParserError> {
let (db_name_keyword, db_name) =
match self.parse_one_of_keywords(&[Keyword::FROM, Keyword::IN]) {
Some(keyword) => (Some(keyword), Some(self.parse_identifier(false)?)),
None => (None, None),
};
let filter = self.parse_show_statement_filter()?;
Ok(Statement::ShowViews {
db_name_keyword,
db_name,
filter,
})
Expand Down Expand Up @@ -9704,7 +9744,12 @@ impl<'a> Parser<'a> {
} else if self.parse_keyword(Keyword::WHERE) {
Ok(Some(ShowStatementFilter::Where(self.parse_expr()?)))
} else {
Ok(None)
self.maybe_parse(|parser| -> Result<String, ParserError> {
parser.parse_literal_string()
})?
.map_or(Ok(None), |filter| {
Ok(Some(ShowStatementFilter::NoKeyword(filter)))
})
}
}

Expand Down
14 changes: 14 additions & 0 deletions tests/sqlparser_hive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,20 @@ fn parse_use() {
);
}

#[test]
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@yoavcloud yoavcloud Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I understand the preference now is for the parser to err on the side of accepting statements even if the dialect doesn't actually support them right? If so, then yes, we can move the tests to common. The SHOW syntax is somewhat popular but it's not a standard.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah we have a few scenarios where the parser accepts input that isn't supported by the dialect. I figured here it could be easier to do so just as well, the alternative would be to add explicit methods to the dialect trait i.e dialect.supports_show_databases() dialect.supports_show_schemas() etc which I think would also be fine if that's preferrable from an impl pov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kept it at the generic level. I have some more SHOW statements for Snowflake to add, so we'll see if these challenge this decision.

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'");
Copy link
Contributor

Choose a reason for hiding this comment

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

for tables/views in addition to IN can we also include testcases for FROM?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, according to the hive docs, it's only for views though.

hive_and_generic().verified_stmt("SHOW VIEWS");
hive_and_generic().verified_stmt("SHOW VIEWS IN db1");
hive_and_generic().verified_stmt("SHOW VIEWS IN db1 'abc'");
}

fn hive() -> TestedDialects {
TestedDialects::new(vec![Box::new(HiveDialect {})])
}
Expand Down
10 changes: 9 additions & 1 deletion tests/sqlparser_mysql.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ use matches::assert_matches;
use sqlparser::ast::MysqlInsertPriority::{Delayed, HighPriority, LowPriority};
use sqlparser::ast::*;
use sqlparser::dialect::{GenericDialect, MySqlDialect};
use sqlparser::keywords::Keyword;
use sqlparser::parser::{ParserError, ParserOptions};
use sqlparser::tokenizer::Token;
use test_utils::*;
Expand Down Expand Up @@ -329,6 +330,7 @@ fn parse_show_tables() {
Statement::ShowTables {
extended: false,
full: false,
db_name_keyword: None,
db_name: None,
filter: None,
}
Expand All @@ -338,6 +340,7 @@ fn parse_show_tables() {
Statement::ShowTables {
extended: false,
full: false,
db_name_keyword: Some(Keyword::FROM),
db_name: Some(Ident::new("mydb")),
filter: None,
}
Expand All @@ -347,6 +350,7 @@ fn parse_show_tables() {
Statement::ShowTables {
extended: true,
full: false,
db_name_keyword: None,
db_name: None,
filter: None,
}
Expand All @@ -356,6 +360,7 @@ fn parse_show_tables() {
Statement::ShowTables {
extended: false,
full: true,
db_name_keyword: None,
db_name: None,
filter: None,
}
Expand All @@ -365,6 +370,7 @@ fn parse_show_tables() {
Statement::ShowTables {
extended: false,
full: false,
db_name_keyword: None,
db_name: None,
filter: Some(ShowStatementFilter::Like("pattern".into())),
}
Expand All @@ -374,13 +380,15 @@ fn parse_show_tables() {
Statement::ShowTables {
extended: false,
full: false,
db_name_keyword: None,
db_name: None,
filter: Some(ShowStatementFilter::Where(
mysql_and_generic().verified_expr("1 = 2")
)),
}
);
mysql_and_generic().one_statement_parses_to("SHOW TABLES IN mydb", "SHOW TABLES FROM mydb");
mysql_and_generic().verified_stmt("SHOW TABLES IN mydb");
mysql_and_generic().verified_stmt("SHOW TABLES FROM mydb");
}

#[test]
Expand Down