[Spark-4041][SQL]attributes names in table scan should converted to lowercase when compare with relation attributes

[scwf]

In MetastoreRelation the attributes name is lowercase because of hive using lowercase for fields name, so we should convert attributes name in table scan lowercase in indexWhere(_.name == a.name).
neededColumnIDs may be not correct if not convert to lowercase.

[AmplabJenkins]

Can one of the admins verify this patch?

[AmplabJenkins]

Can one of the admins verify this patch?

[yhuai]

Can you add a unit test?

[liancheng]

Nit: it would be safer if you use _.name.toLowerCase == a.name.toLowerCase.

[scwf]

@yhuai, it's hard to make a unit test for this since addColumnMetadataToConf and hiveExtraConf both can not accessed in test case, they are private. Actually i debug the code and get this issue, it seems this will leads to NPE in hive-0.13 on my local test but ok in master branch(hive-0.12)
@liancheng, updated

[liancheng]

I think this change is generally safe. LGTM, thanks.

[SparkQA]

QA tests have started for PR 2884 at commit 3ff3a80.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 2884 at commit 3ff3a80.

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

[scwf]

test failed due to streaming compile error, can you retest this?

[SparkQA]

QA tests have started for PR 2884 at commit 3ff3a80.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 2884 at commit 3ff3a80.

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

[liancheng]

Hm, the failure was caused by a known Jenkins configuration issue.

[liancheng]

retest this please

[SparkQA]

QA tests have started for PR 2884 at commit 3ff3a80.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 2884 at commit 3ff3a80.

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

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22096/
Test PASSed.

[marmbrus]

Wait, should this be done by name at all? Couldn't we be using an AttributeMap from Attribute->ordinal instead?

[scwf]

Yes, column names are case insensitive in hive, we should use lowercase for names in hive module(only change here is not enough, also need convert to lowercase there https://github.com/apache/spark/blob/master/sql/hive/src/main/scala/org/apache/spark/sql/hive/TableReader.scala#L273).
I think using an AttributeMap can not fix this problem, how about add a lowerName for Attribute and in hive we use this method instead?

[scwf]

Added a test case for lower case issue, the test will throw NPE if not converted to lowercase

[marmbrus]

Great, thanks for finding this and adding a test. Regarding the implementation, I'd like to try to avoid doing too much string munging as its generally easy to forget to do (hence the issue). Also, in general we try to avoid looking at string names anywhere other than in analysis. This is the whole idea behind having expression ids in AttributeReferences (and the idea behind AttributeMaps).

Since we can't completely get away from string names when working with Hive, what do you think about this approach: https://github.com/marmbrus/spark/compare/hiveTableScanCase

I think this more cleanly isolates the need to reason about case sensitivity into the analysis phase.

[scwf]

Cool, i think this is better

[scwf]

retest this please

[SparkQA]

Test build #473 has started for PR 2884 at commit 6174046.

• This patch merges cleanly.

[SparkQA]

Test build #473 has finished for PR 2884 at commit 6174046.

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

[scwf]

retest this again, seems Jenkins get something wrong and failed in JoinSuite. locally it passed.

[SparkQA]

Test build #475 has started for PR 2884 at commit 6174046.

• This patch merges cleanly.

[SparkQA]

QA tests have started for PR 2884 at commit 6174046.

• This patch does not merge cleanly.

[scwf]

CliSuite failed due to Futures timed out, @liancheng any idea about this? seems #2823 does not fix the problem.

[SparkQA]

QA tests have finished for PR 2884 at commit 6174046.

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

[SparkQA]

Test build #475 has finished for PR 2884 at commit 6174046.

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

[marmbrus]

Minor comment: In the future please put SPARK-XXXX in all capitals in the title so that our merge scripts recognize it. Thanks!

Thanks for working on this! Merged to master.