[SPARK-12437][SQL] [WIP]Encapsulate the table and column name in quotes

[naveenminchu]

test this please

[hvanhovell]

@naveenminchu is there any sensible way to test this? If so, could you add a test?

[hvanhovell]

@naveenminchu I tried triggering a build, but it seems that I do not have sufficient rights for this (I do have them for my own PRs).

[rxin]

Different databases have different quoting syntax. You need to use the JDBCDialect's quoting function to quote the table/column names.

[naveenminchu]

@rxin Agree 100%

[JoshRosen]

Can you add a test for this in the MySQL docker integration tests suite (see the docker-integration-tests subproject)?

[JoshRosen]

Also, quick question: what if the name is qualified with a schema and also contains special characters? e.g. what if my user-supplied table name is something like default."table with spaces in name"? Will this handle that case properly? What about "some database"."some-table-name"? My concern is that a naive, unconditional quoting will mishandle these cases.

spark-redshift has some fancy logic to handle all of these corner-cases; see databricks/spark-redshift#102 and pay attention to how the TableName wrapper works.

[rxin]

what's this change for?

[rxin]

And +1 on adding a test.

[rxin]

BTW if I understand this correctly it only quotes table names, but not column names? Are column names already quoted?

[naveenminchu]

@rxin Yes as u suggested I have used JdbcDialect

Will also look in to test part

[naveenminchu]

@rxin & @JoshRosen Please take a quick look of changes

[pjfanning]

Would it be possible to add a table or tables to the dataPreparation function that have column names that are key words? For example:

create table escaped names (key BIGINT, long description TEXT);

[naveenminchu]

@pjfanning Added test as suggested
For example:
create table escaped names (key BIGINT, long description TEXT);

[pjfanning]

Hi Naveen - your new tests look good to me. I do notice though that you probably need to resync with master due to conflicts (there is a 'This branch has conflicts that must be resolved' warning).
https://help.github.com/articles/syncing-a-fork/

[JoshRosen]

Woah, this is kinda dense and confusing. Can you please rewrite this in a simpler fashion and add comments? This is just my drive-by first impression.

[JoshRosen]

Also, can you add unit tests for this function? Not end-to-end tests, but a test which exercises this method in isolation, similar to the ones that I have in spark-redshift?

[JoshRosen]

The method contract here is confusing to me: is this supposed to introduce an additional layer of quotes? I find it confusing to couple both the responsibilities of quoting identifiers and parsing table names from a schema-qualified name.

I'd prefer to have this return just the table name, unquoted, and leave it up to later components to escape it. If you do think we should do quoting here, please explain that part of the API contract in the Scaladoc.

[JoshRosen]

I'm still not a fan of this foldLeft. Can you please write this in a more clear way?

[JoshRosen]

Ping. Any updates here?

[naveenminchu]

Will address it these week sometime