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

yoavcloud · 2024-10-30T05:27:41Z

This PR adds support for the above statements in the Hive dialect, making the previously MySQL-only structs a bit more generic.

git-hulk · 2024-10-30T05:33:25Z

src/ast/mod.rs

+                    let keyword = match &db_name_keyword {
+                        Some(Keyword::FROM) => "FROM",
+                        Some(Keyword::IN) => "IN",
+                        _ => "", // unexpected


Can use unreachable!() to panic if it's an unexpected condition.

git-hulk · 2024-10-30T05:34:49Z

src/ast/mod.rs

+                    let keyword = match &db_name_keyword {
+                        Some(Keyword::FROM) => "FROM",
+                        Some(Keyword::IN) => "IN",
+                        _ => "", // unexpected


Same as the above comment.

git-hulk · 2024-10-30T05:39:24Z

tests/sqlparser_hive.rs

@@ -534,6 +534,20 @@ fn parse_use() {
    );
 }

+#[test]
+fn test_show() {
+    hive().verified_stmt("SHOW DATABASES");


It would be better to use hive_and_generic since it also works for generic dialects.

…ES for hive dialect

git-hulk · 2024-10-30T06:01:40Z

tests/sqlparser_mysql.rs

            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");


Suggested change

mysql_and_generic().verified_stmt( "SHOW TABLES FROM mydb");

mysql_and_generic().verified_stmt("SHOW TABLES FROM mydb");

git-hulk

LGTM, cc @iffyio

iffyio · 2024-10-31T10:08:39Z

tests/sqlparser_hive.rs

+    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'");


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

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

iffyio · 2024-10-31T10:11:00Z

tests/sqlparser_hive.rs

@@ -534,6 +534,20 @@ fn parse_use() {
    );
 }

+#[test]


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

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.

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

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.

iffyio · 2024-10-31T10:22:28Z

src/ast/mod.rs

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


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

Good comments, with the ShowClause the unreachable dilemma is irrelevant

iffyio · 2024-10-31T10:28:44Z

src/ast/mod.rs

@@ -2782,12 +2783,30 @@ pub enum Statement {
        filter: Option<ShowStatementFilter>,
    },
    /// ```sql
+    /// SHOW DATABASES [LIKE 'pattern']
+    /// ```
+    ShowDatabases { pattern: Option<String> },


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

iffyio

LGTM! cc @alamb

coveralls · 2024-11-01T15:30:02Z

Pull Request Test Coverage Report for Build 11632792517

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

91 of 95 (95.79%) changed or added relevant lines in 4 files are covered.
3 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.02%) to 89.396%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/ast/mod.rs	32	36	88.89%

Files with Coverage Reduction	New Missed Lines	%
src/ast/mod.rs	1	82.14%
src/dialect/postgresql.rs	2	73.5%

Totals
Change from base Build 11580333290:	0.02%
Covered Lines:	30500
Relevant Lines:	34118

💛 - Coveralls

yoavcloud · 2024-11-01T16:34:40Z

Sorry, did a last change and forgot cargo fmt... Changes committed now, if you can run the CI again that would be great @alamb

alamb · 2024-11-01T20:41:11Z

🚀

yoavcloud · 2024-11-03T06:34:40Z

@iffyio @alamb why isn't this merged?

alamb · 2024-11-03T13:06:52Z

@iffyio @alamb why isn't this merged?

Sorry -- am behind.

yoavcloud · 2024-11-03T18:30:44Z

Thank you!

)

git-hulk reviewed Oct 30, 2024

View reviewed changes

yoavcloud added 3 commits October 30, 2024 07:44

Added support for SHOW DATABASES/SCHEMAS/VIEWS and extended SHOW TABL…

4e67a52

…ES for hive dialect

Improve parsing for optional literal string without a keyword indication

a5edbb7

Code review comments

9249aa3

yoavcloud force-pushed the hive_show_tables_in branch from 17aa53f to 9249aa3 Compare October 30, 2024 05:47

yoavcloud requested a review from git-hulk October 30, 2024 05:48

git-hulk reviewed Oct 30, 2024

View reviewed changes

Remove redundant whitespace

febab0b

yoavcloud requested a review from git-hulk October 30, 2024 06:04

git-hulk approved these changes Oct 30, 2024

View reviewed changes

iffyio reviewed Oct 31, 2024

View reviewed changes

Code review comments, also added support for materialized views

407003a

yoavcloud requested a review from iffyio October 31, 2024 22:37

iffyio approved these changes Nov 1, 2024

View reviewed changes

formatting

07a651a

alamb enabled auto-merge (squash) November 1, 2024 20:41

alamb approved these changes Nov 3, 2024

View reviewed changes

alamb merged commit e2197ee into apache:main Nov 3, 2024
10 checks passed

wugeer pushed a commit to wugeer/sqlparser-rs that referenced this pull request Nov 5, 2024

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

d34b399

)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

yoavcloud commented Oct 30, 2024

git-hulk Oct 30, 2024

yoavcloud Oct 30, 2024

git-hulk Oct 30, 2024

yoavcloud Oct 30, 2024

git-hulk Oct 30, 2024

yoavcloud Oct 30, 2024

git-hulk Oct 30, 2024

yoavcloud Oct 30, 2024

git-hulk left a comment

iffyio Oct 31, 2024

yoavcloud Oct 31, 2024

iffyio Oct 31, 2024

yoavcloud Oct 31, 2024 •

edited

Loading

iffyio Oct 31, 2024

yoavcloud Oct 31, 2024

iffyio Oct 31, 2024

yoavcloud Oct 31, 2024

iffyio Oct 31, 2024

yoavcloud Oct 31, 2024

iffyio left a comment

coveralls commented Nov 1, 2024 •

edited

Loading

yoavcloud commented Nov 1, 2024

alamb commented Nov 1, 2024

yoavcloud commented Nov 3, 2024

alamb commented Nov 3, 2024

yoavcloud commented Nov 3, 2024

	mysql_and_generic().verified_stmt( "SHOW TABLES FROM mydb");
	mysql_and_generic().verified_stmt("SHOW TABLES FROM mydb");

@@ @@ -534,6 +534,20 @@ fn parse_use() { @@
                   );
               }
+              #[test]

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

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

Conversation

yoavcloud commented Oct 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

git-hulk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoavcloud Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iffyio left a comment

Choose a reason for hiding this comment

coveralls commented Nov 1, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11632792517

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

yoavcloud commented Nov 1, 2024

alamb commented Nov 1, 2024

yoavcloud commented Nov 3, 2024

alamb commented Nov 3, 2024

yoavcloud commented Nov 3, 2024

yoavcloud Oct 31, 2024 •

edited

Loading

coveralls commented Nov 1, 2024 •

edited

Loading