[SPARK-14184][SQL] Support native execution of SHOW DATABASE command and fix SHOW TABLE to use table identifier pattern #11991

dilipbiswal · 2016-03-28T03:03:40Z

What changes were proposed in this pull request?

This PR addresses the following

Supports native execution of SHOW DATABASES command
Fixes SHOW TABLES to apply the identifier_with_wildcards pattern if supplied.

SHOW TABLE syntax

SHOW TABLES [IN database_name] ['identifier_with_wildcards'];

SHOW DATABASES syntax

SHOW (DATABASES|SCHEMAS) [LIKE 'identifier_with_wildcards'];

How was this patch tested?

Tests added in SQLQuerySuite (both hive and sql contexts) and DDLCommandSuite

Note: Since the table name pattern was not working , tests are added in both SQLQuerySuite to
verify the application of the table pattern.

dilipbiswal · 2016-03-28T03:04:50Z

cc @gatorsmile @yhuai

gatorsmile · 2016-03-28T03:08:27Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/commands.scala

gatorsmile · 2016-03-28T03:11:53Z

You also need to add test cases in SQLContext for verifying correctness of the Command execution. So far, it only covers the Parsing correctness.

dilipbiswal · 2016-03-28T05:25:13Z

@gatorsmile Thanks for your review. I have addressed most of your comments except the last one. Currently create/drop database commands are not supported for SQLContext. Thus i am unable to do a meaningful test.

gatorsmile · 2016-03-28T05:32:46Z

@dilipbiswal Sorry for that. Will try to finish that part ASAP. Thanks!

andrewor14 · 2016-03-28T22:31:21Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkQl.scala

why accept FROM? It seems kind of weird. Hive doesn't do this either: https://cwiki.apache.org/confluence/display/Hive/LanguageManual+DDL#LanguageManualDDL-ShowTables

@andrewor14 Hello, we always transform to TOK_FROM for both TOK_IN and TOK_FROM in the grammer file.

spark/sql/catalyst/src/main/antlr3/org/apache/spark/sql/catalyst/parser/SparkSqlParser.g

Line 1473 in 637a78f

| KW_SHOW KW_TABLES ((KW_FROM|KW_IN) db_name=identifier)? (KW_LIKE showStmtIdentifier|showStmtIdentifier)? -> ^(TOK_SHOWTABLES ^(TOK_FROM $db_name)? showStmtIdentifier?)

So now, both form works.

show tables from default show tables in default

I will fix up the comments to use 'IN' as opposed to 'FROM'. Did you want to disallow this syntax in the grammer file ? Would there be a backward-compatibility issue if we do that ?

I see, that's too bad. Let's just keep it

andrewor14 · 2016-03-28T22:33:32Z

ok to test

andrewor14 · 2016-03-28T22:34:22Z

FYI this blocks on #12009.

SparkQA · 2016-03-28T22:42:24Z

Test build #54373 has finished for PR 11991 at commit 1e15bc2.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

hvanhovell · 2016-03-28T23:43:46Z

@dilipbiswal Could you also add support for the ANTLR4 parser? You'll need to a rule to SqlBase.q4 and do the parsing in SparkSqlParser.scala. Let me know if you need help.

dilipbiswal · 2016-03-29T00:19:59Z

@hvanhovell Sure Herman. I will give it a try and get back to you with questions :-)

viirya · 2016-03-29T00:45:29Z

@hvanhovell Can you please add @dilipbiswal into whitelist? Thanks!

andrewor14 · 2016-03-29T00:47:23Z

add to whitelist

SparkQA · 2016-03-29T01:16:47Z

Test build #54380 has finished for PR 11991 at commit 92fe39f.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-03-29T02:53:37Z

Test build #54395 has finished for PR 11991 at commit 92fe39f.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

rxin · 2016-03-29T03:21:11Z

I just merged #12015

Can you update to use the new ANTLR4 parser instead? We are going to remove the ANTLR3 one in the next day or two. Thanks.

dilipbiswal · 2016-03-29T08:05:17Z

@rxin Yes.. I am working on using the new ANTLR4 parser. Thank you.

SparkQA · 2016-03-29T17:35:29Z

Test build #54443 has finished for PR 11991 at commit b66f16e.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

hvanhovell · 2016-03-29T19:39:59Z

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/ng/SqlBase.g4

You can remove the 'from' option now.

Sure. Will do

@hvanhovell Actually Herman, i tried the following in Hive.

hive> show tables from default; OK default__t1_t1_index__

should we keep the from option ? There are also some tests which use from option.

Lets keep it then.

gatorsmile · 2016-03-30T00:58:31Z

@dilipbiswal The PR #12009 has been merged. Could you add a test case for show database? Thanks!

SparkQA · 2016-03-30T17:22:17Z

Test build #54527 has finished for PR 11991 at commit fe60df8.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

hvanhovell · 2016-03-30T19:18:16Z

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/ng/SqlBase.g4

Do we need to seperate tokens here? I thought they were synonyms?

@hvanhovell Herman, yeah.. they are synonyms. I was not sure, If we just define it as one Token as say "DATABASES" , we can still refer to it from the nonReserved rule as
"DATABASES" ?

Yeah we can. It would be the same as the DATABASE lexer rule.

SparkQA · 2016-03-30T19:30:17Z

Test build #54539 has finished for PR 11991 at commit 9a11daa.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

andrewor14 · 2016-03-30T21:44:56Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkQl.scala

FYI to avoid conflicts with #12071 let's revert all the changes in this file since we're not using this parser anymore anyway.

@andrewor14 Hi Andrew, is it ok if i have a very minor change in this file just to make sure compilation goes through. Or it will still cause issue with merging ?

ShowTablesCommand(databaseName, None) => we introduced a new parameter to the command.

I see... never mind then. It seems that we can't avoid the conflicts after all.

andrewor14 · 2016-03-30T21:48:17Z

Looks great!

SparkQA · 2016-03-30T23:10:02Z

Test build #54554 has finished for PR 11991 at commit 9a4f356.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-03-31T01:31:31Z

Test build #54565 has finished for PR 11991 at commit 2e16ce4.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

andrewor14 · 2016-03-31T17:41:46Z

@dilipbiswal can you rebase? I think SparkQl is now deleted.

…ix SHOW TABLE to use table identifier pattern

dilipbiswal · 2016-03-31T18:21:54Z

@andrewor14 Hi Andrew, just rebased. - FYI

SparkQA · 2016-03-31T19:52:07Z

Test build #54651 has finished for PR 11991 at commit bafca92.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dilipbiswal · 2016-03-31T20:57:43Z

cc @andrewor14

hvanhovell · 2016-03-31T21:00:11Z

LGTM

hvanhovell · 2016-04-01T16:28:48Z

Merging to master. Thanks!

dilipbiswal · 2016-04-01T17:11:12Z

@hvanhovell Thank you, Herman.

gatorsmile reviewed Mar 28, 2016
View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/command/commands.scala Outdated

Copy link

Member

gatorsmile Mar 28, 2016

Choose a reason for hiding this comment

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

indents.

dilipbiswal force-pushed the dkb_show_database branch from 31cd3c0 to db8a5e8 Compare March 28, 2016 03:12

andrewor14 reviewed Mar 28, 2016
View reviewed changes

dilipbiswal force-pushed the dkb_show_database branch from 1e15bc2 to 92fe39f Compare March 28, 2016 23:31

dilipbiswal force-pushed the dkb_show_database branch from 92fe39f to b66f16e Compare March 29, 2016 15:45

hvanhovell reviewed Mar 29, 2016
View reviewed changes

hvanhovell reviewed Mar 30, 2016
View reviewed changes

andrewor14 reviewed Mar 30, 2016
View reviewed changes

dilipbiswal added 12 commits March 31, 2016 11:07

[SPARK-14184] Support native execution of SHOW DATABASE command and f…

8f2c8a7

…ix SHOW TABLE to use table identifier pattern

Review comments

feec138

fix1

42786c3

thriftserver test fix

5f5928b

rebase with antlr4 change

83c5c80

antlr4 based parsing.

a24afa8

Herman's comments

f28fcda

Simon and Xiao's comment

64d75c2

Move show tables to DDLSuiite

568219f

reset database to default

4da2f3c

one token for DATABASES and SCHEMAS

e9dcb76

Reset SparkQl and address more comments

bafca92

dilipbiswal force-pushed the dkb_show_database branch from 2e16ce4 to bafca92 Compare March 31, 2016 18:21

asfgit closed this in 0b04f8f Apr 1, 2016

[SPARK-14184][SQL] Support native execution of SHOW DATABASE command and fix SHOW TABLE to use table identifier pattern #11991

[SPARK-14184][SQL] Support native execution of SHOW DATABASE command and fix SHOW TABLE to use table identifier pattern #11991

Uh oh!

Conversation

dilipbiswal commented Mar 28, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

dilipbiswal commented Mar 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Mar 28, 2016

Uh oh!

dilipbiswal commented Mar 28, 2016

Uh oh!

gatorsmile commented Mar 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewor14 commented Mar 28, 2016

Uh oh!

andrewor14 commented Mar 28, 2016

Uh oh!

SparkQA commented Mar 28, 2016

Uh oh!

hvanhovell commented Mar 28, 2016

Uh oh!

dilipbiswal commented Mar 29, 2016

Uh oh!

viirya commented Mar 29, 2016

Uh oh!

andrewor14 commented Mar 29, 2016

Uh oh!

SparkQA commented Mar 29, 2016

Uh oh!

SparkQA commented Mar 29, 2016

Uh oh!

rxin commented Mar 29, 2016

Uh oh!

dilipbiswal commented Mar 29, 2016

Uh oh!

SparkQA commented Mar 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Mar 30, 2016

Uh oh!

SparkQA commented Mar 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewor14 commented Mar 30, 2016

Uh oh!

SparkQA commented Mar 30, 2016

Uh oh!