> On May 14, 2020, 8:12 p.m., Sankar Hariappan wrote:
> > ql/src/test/queries/clientpositive/msck_repair_4.q
> > Lines 8 (patched)
> > <https://reviews.apache.org/r/72462/diff/1/?file=2229577#file2229577line8>
> >
> >     Add a testcase with table path repairtable_n4 having upper case. You 
> > can achieve it by setting location in create table.

Done.


> On May 14, 2020, 8:12 p.m., Sankar Hariappan wrote:
> > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/CheckResult.java
> > Lines 116 (patched)
> > <https://reviews.apache.org/r/72462/diff/1/?file=2229579#file2229579line116>
> >
> >     Why do we need pathSet? Can we check if path == null instead?

Removed.


> On May 14, 2020, 8:12 p.m., Sankar Hariappan wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java
> > Line 1441 (original), 1441 (patched)
> > <https://reviews.apache.org/r/72462/diff/1/?file=2229582#file2229582line1441>
> >
> >     Even table name in directory can be any case. Should we use 
> > equalsIgnoresCase here?

Msck list all the directories under the actual table path, and then checks if 
the new child directories are valid partitions. Since all the partition 
directories are child of the actual table path, it won't matter if we are using 
equals/equalsIgnoreCase.

The second question which comes is, should we check for partitions under all 
paths that matches the actual table path ignoring the case. I don't think we 
should do that, because, then msck will have to check in too many combinations 
of base directory in hdfs (which will be ~2^length of table name).


> On May 14, 2020, 8:12 p.m., Sankar Hariappan wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java
> > Line 1454 (original), 1456 (patched)
> > <https://reviews.apache.org/r/72462/diff/1/?file=2229582#file2229582line1456>
> >
> >     Need to check how Hive treats ptn='A' and ptn='a' as ptn keys are 
> > lowercase but values can be any case.

Added a test case. These two values should be treated as different as they both 
are different strings.


- Adesh


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72462/#review220771
-----------------------------------------------------------


On May 15, 2020, 5:42 a.m., Adesh Rao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72462/
> -----------------------------------------------------------
> 
> (Updated May 15, 2020, 5:42 a.m.)
> 
> 
> Review request for hive and Sankar Hariappan.
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> The fix converts partition keys to lowercase present in hdfs directory, but 
> store the hdfs directory as is for partition path.
> 
> 
> Diffs
> -----
> 
>   ql/src/test/queries/clientpositive/msck_repair_4.q PRE-CREATION 
>   ql/src/test/queries/clientpositive/msck_repair_5.q PRE-CREATION 
>   ql/src/test/queries/clientpositive/msck_repair_6.q PRE-CREATION 
>   ql/src/test/results/clientpositive/msck_repair_4.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/msck_repair_5.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/msck_repair_6.q.out PRE-CREATION 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/CheckResult.java
>  5287f47e21 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreChecker.java
>  6f4400a8ef 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/Msck.java
>  f4e109d1b0 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java
>  92d10cd0e1 
> 
> 
> Diff: https://reviews.apache.org/r/72462/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Adesh Rao
> 
>

Reply via email to