Implement SparkSession.Catalog

[GoEddie]

This is for #226

We are excited to review your PR.

So we can do the best job, please check:

• There's a descriptive title that will make sense to other developers some time from now.
• There's associated issues. All PR's should have issue(s) associated - unless a trivial self-evident change such as fixing a typo. You can use the format Fixes #nnnn in your description to cause GitHub to automatically close the issue(s) when your PR is merged.
• Your change description explains what the change does, why you chose your approach, and anything else that reviewers should know.
• You have included any necessary tests in the same PR.

Implementing SparkSession.Catalog, this includes most of the catalog functions except any deprecated ones and some of the experimental functions.

[suhsteve]

Are these usings still needed ?

[GoEddie]

The new test uses Path.Combine to build a path, I know in other places the code just manually creates a path but I think Path.Combine is better (I was going to do another pr converting the manually built strings to Path.Combine)

[suhsteve]

Surround your test and put something like the following at the beginning of CatalogFunctions()

using (var tempDir = new TemporaryDirectory())
{
...
}

Then you should be able to do something like

DataFrame table = catalog.CreateTable("users", Path.Combine(tempDir.Path, "users.parquet"));

[imback82]

@GoEddie what do you think about @suhsteve 's suggestion so that we clean up the resources per test (or suite)

[GoEddie]

Just to clarify, is the guidance to copy from the comments from spark docs or describe them in more detail?

In some feedback here I have been asked to copy from the spark codebase and some to add more detail.

I don't mind which but just need some guidance!

"copy from spark": #231 (comment)
"add more detail": #231 (comment)

Example "tableExists":

https://spark.apache.org/docs/2.2.1/api/java/org/apache/spark/sql/catalog/Catalog.html#tableExists-java.lang.String-

public abstract boolean tableExists(String dbName,
                                    String tableName)
Check if the table or view with the specified name exists in the specified database.
Parameters:
dbName - is a name that designates a database.
tableName - is an unqualified name that designates a table.
Returns:
(undocumented)

I have added a return type and can add something like "bool - true if the table exists, false if it doesn't" - the meaning of the return should be obvious??

Looking throughthe codebase some places are more descriptive and some less:

        /// <summary>
        /// Returns a sort expression based on the descending order of the column.
        /// </summary>
        /// <param name="columnName">Column name</param>
        /// <returns>Column object</returns>
        public static Column Desc(string columnName)
        {
            return ApplyFunction("desc", columnName);
        }

        /// <summary>
        /// Apply boolean AND operator with the given column.
        /// </summary>
        /// <param name="other">Column to apply AND operator</param>
        /// <returns>New column after applying the operator</returns>

Let me know and i'll fix them all up :)

[imback82]

Just to clarify, is the guidance to copy from the comments from spark docs or describe them in more detail?

Thanks for bringing this up @GoEddie. Looks like there was an inconsistency in the review process.

What I typically do about comments when new APIs are introduced is to start from the Spark comments and update them as fit (not all their comments are great either. :)). I understand that there are already inconsistencies in comments in our codebase. Since it's hard to go back and fix all the time, we try to make the bar a little higher on the new PRs. If you think review comments are not reasonable, please include me in the discussion so I can resolve them.

BTW, We also publish API references based on the comments: https://docs.microsoft.com/en-us/dotnet/api/microsoft.spark.sql.dataframe?view=spark-dotnet.

Thanks for your contribution @GoEddie!

[GoEddie]

Thanks @imback82 i'll make sure to start with the spark comments and add to them if they need it.

[GoEddie]

I think that is all the comments dealt with - if your not happy or want changes let me know :)

[imback82]

Thanks @GoEddie, I will review this soon.

[GoEddie]

Great let me know if you want any changes :)

[imback82]

I have few minor comments. Great work!

[imback82]

@GoEddie what do you think about @suhsteve 's suggestion so that we clean up the resources per test (or suite)

[GoEddie]

Thanks @imback82 for reviewing - I think that is everything - let me know if there are any other changes :)

For this one #231 (comment) the suggestion was when I was having trouble setting the path to spark-warehouse - this is reading the users.parquet file from the resources directory so there aren't any resources to clean up here.

[imback82]

do you know why this formatting changed? I think the existing formatting is fine. (Ctrl + K, D gives me the existing formatting). Could you double check the latest commit?

[GoEddie]

I think resharper got a bit excited :)

[imback82]

LGTM