> On Jan. 19, 2018, 1:56 p.m., Peter Vary wrote: > > Thanks Marta for the patch. > > Some questions below. > > Also I think it would be nice to have test for the default values when > > adding a partition. > > > > Thanks, > > Peter
Thanks a lot Peter for the review. That's a good idea, added tests for creating partitions with default values. > On Jan. 19, 2018, 1:56 p.m., Peter Vary wrote: > > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java > > Lines 236 (patched) > > <https://reviews.apache.org/r/65219/diff/1/?file=1942108#file1942108line236> > > > > There are some interesting validation in the SD. Maybe we can add test > > for it here too. Good point, thanks. Created tests for some use cases. > On Jan. 19, 2018, 1:56 p.m., Peter Vary wrote: > > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java > > Lines 282 (patched) > > <https://reviews.apache.org/r/65219/diff/1/?file=1942108#file1942108line282> > > > > Is it worth to create a test for an External Table too? Creating partitions for an external table doesn't really differ from creating it for a managed table. But I agree it worth to have a test case for that too. I added it. > On Jan. 19, 2018, 1:56 p.m., Peter Vary wrote: > > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java > > Lines 700 (patched) > > <https://reviews.apache.org/r/65219/diff/1/?file=1942108#file1942108line700> > > > > Like above - validation inside the SD might be important as well Created some test cases for sd attribute validations. > On Jan. 19, 2018, 1:56 p.m., Peter Vary wrote: > > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java > > Lines 749 (patched) > > <https://reviews.apache.org/r/65219/diff/1/?file=1942108#file1942108line749> > > > > Maybe for external table too. And maybe check the locations Added tests for external table with and without setting the location. > On Jan. 19, 2018, 1:56 p.m., Peter Vary wrote: > > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java > > Lines 1117 (patched) > > <https://reviews.apache.org/r/65219/diff/1/?file=1942108#file1942108line1117> > > > > I do not understand this check If the 'metastore.partition.inherit.table.properties' property is set in the metastore config, the partition inherits the listed table parameters. This property is not set in this test, therefore the partition shouldn't inherit the table parameters. That is what this check is for. But it is true that this is not obvious, so I added a comment to this check. - Marta ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65219/#review195811 ----------------------------------------------------------- On Jan. 18, 2018, 4:58 p.m., Marta Kuczora wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65219/ > ----------------------------------------------------------- > > (Updated Jan. 18, 2018, 4:58 p.m.) > > > Review request for hive, Peter Vary and Adam Szita. > > > Bugs: HIVE-18486 > https://issues.apache.org/jira/browse/HIVE-18486 > > > Repository: hive-git > > > Description > ------- > > The following methods of IMetaStoreClient are covered by this test. > - Partition add_partition(Partition) > - int add_partitions(List<Partition>) > - List<Partition> add_partitions(List<Partition>, boolean, boolean) > > > The test covers not just the happy pathes, but the edge cases as well. > > > Diffs > ----- > > > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/65219/diff/1/ > > > Testing > ------- > > Run the tests > > > Thanks, > > Marta Kuczora > >
