Skip to content

Conversation

@sharangk
Copy link
Contributor

@sharangk sharangk commented Aug 27, 2019

What changes were proposed in this pull request?

Document CREATE DATABASE statement in SQL Reference Guide.

Why are the changes needed?

Currently Spark lacks documentation on the supported SQL constructs causing
confusion among users who sometimes have to look at the code to understand the
usage. This is aimed at addressing this issue.

Does this PR introduce any user-facing change?

Yes.

Before:

There was no documentation for this.

After:

image
image

How was this patch tested?

Manual Review and Tested using jykyll build --serve

@sharangk
Copy link
Contributor Author

@dilipbiswal , @gatorsmile , Please review. Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

[ WITH DBPROPERTIES ( property_name = property_value [ , ... ] ) ]
Since it's optional to have multiple property name value pairs, I think it's better to use [ , ... ] to show it's optional.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dilipbiswal @gatorsmile
I am wondering if we should define a standard for the syntax format. Should we have a white space between the braces, =, |, ... and the text? I currently put a white space because I saw a couple of sql references have these standard. I am OK with either having white space or no white space, but I am thinking we may need to define a standard and write out the standard explicitly in the umbrella jira so everybody can follow. Also, shall we have 4 white space indentation for the 2nd line? I am ok with either 2 or 4, but prefer everybody to follow the same way.

Copy link
Contributor

Choose a reason for hiding this comment

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

@huaxingao About while spacing , i am +1 on this. About the indentation on the 2nd line, i don't have a strong preference. @gatorsmile do you have a preference here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated white spaces wherever required. Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please rebase your branch and follow the parameters format in #25523? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Formatted based on #25523 . Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: ### Related Statements

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

Between two tests, lets have a empty line ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

Indent this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is an optional clause, should we specify what happens when this is not specified ? The database is created in the default warehouse directory ? Could you please check ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Thanks

@dilipbiswal
Copy link
Contributor

@sharangk Thanks for working on it. Can we please re-word some descriptions ? It sounds similar to some other docs.

@sharangk
Copy link
Contributor Author

sharangk commented Sep 3, 2019

Thanks for the timely review. I will work the comments and resubmit the PR.

@sharangk sharangk force-pushed the createDbDoc branch 4 times, most recently from b494bfb to 246290d Compare September 5, 2019 11:13
@sharangk
Copy link
Contributor Author

sharangk commented Sep 5, 2019

Handled the comments. Thanks @dilipbiswal @huaxingao for timely review.
@gatorsmile , Could you please have a look at the changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny nit : Specifies the description for the database.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny nit: add Specifies

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put the 2nd line under a comment like

-- Create database `customer_db`. This throws exception if database with name customer_db
-- already exists.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put the 2nd line under a comment like

-- Create database `customer_db` only if database with same name doesn't exist with
-- `Comments`,`Specific Location` and `Database properties`.

Copy link
Contributor

Choose a reason for hiding this comment

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

lets indent the 2nd line so that it looks continuation of first line like :

CREATE DATABASE IF NOT EXISTS customer_db COMMENT 'This is customer database' LOCATION '/user' 
  WITH DBPROPERTIES (ID=001, Name='John');

@dilipbiswal
Copy link
Contributor

@sharangk Mostly looks good. I have put some minor comments.

…erence

# Why are the changes needed?
Currently Spark lacks documentation on the supported SQL constructs causing
confusion among users who sometimes have to look at the code to understand the
usage. This is aimed at addressing this issue.

## How was this patch tested?
This is just a documentation change, verified manually.
…erence

# Why are the changes needed?
Currently Spark lacks documentation on the supported SQL constructs causing
confusion among users who sometimes have to look at the code to understand the
usage. This is aimed at addressing this issue.

## How was this patch tested?
This is just a documentation change, verified manually.
@sharangk
Copy link
Contributor Author

@dilipbiswal , Thanks for the review. Resubmitted the PR with updates. Please have a look. Thanks !!!

<dd>Creates a database with the given name if it doesn't exists. If a database with the same name already exists, nothing will happen.</dd>

<dt><code><em>database_directory</em></code></dt>
<dd>Path of the file system in which database to be created. If the specified path does not exist in the underlying file system, this command creates a directory with the path. If location is not specified, database will be created in default warehouse directory.</dd>
Copy link
Contributor

Choose a reason for hiding this comment

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

@sharangk in which the specified database is to be created ?

@sharangk
Copy link
Contributor Author

@dilipbiswal , Thanks. Handled the review comments.

@dilipbiswal
Copy link
Contributor

LGTM
cc @gatorsmile

@gatorsmile
Copy link
Member

ok to test

@SparkQA
Copy link

SparkQA commented Sep 17, 2019

Test build #110828 has finished for PR 25595 at commit 0225553.

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

@gatorsmile
Copy link
Member

Thanks! Merged to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants