----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71016/#review216884 -----------------------------------------------------------
common/src/java/org/apache/hadoop/hive/common/format/datetime/HiveSqlDateTimeFormatter.java Lines 546 (patched) <https://reviews.apache.org/r/71016/#comment304127> I think it would be better to split this method to two: one for checking only fm and one for checking only fx. Returning a boolean and setting an other one in the background can be a bit confusing for the caller. common/src/test/org/apache/hadoop/hive/common/format/datetime/TestHiveSqlDateTimeFormatter.java Lines 252 (patched) <https://reviews.apache.org/r/71016/#comment304128> I think this test could be split up to have separate tests for fm, fx and fm-fx cases. It is just a nit, but I think it is a good idea to focus on one use-case per test. - Marta Kuczora On July 5, 2019, 7:51 a.m., Karen Coppage wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71016/ > ----------------------------------------------------------- > > (Updated July 5, 2019, 7:51 a.m.) > > > Review request for hive and Marta Kuczora. > > > Bugs: HIVE-21578 > https://issues.apache.org/jira/browse/HIVE-21578 > > > Repository: hive-git > > > Description > ------- > > Enable Hive to parse the following datetime formats when any combination or > subset of these or previously implemented formats is provided in one string. > > "text" (nested strings) > FM > FX > > > Diffs > ----- > > > common/src/java/org/apache/hadoop/hive/common/format/datetime/HiveSqlDateTimeFormatter.java > 998e5a2f6a > > common/src/test/org/apache/hadoop/hive/common/format/datetime/TestHiveSqlDateTimeFormatter.java > 4e822d53f9 > ql/src/test/queries/clientpositive/cast_datetime_with_sql_2016_format.q > 5a2a6d7894 > ql/src/test/results/clientpositive/cast_datetime_with_sql_2016_format.q.out > e1fd341050 > > > Diff: https://reviews.apache.org/r/71016/diff/2/ > > > Testing > ------- > > > Thanks, > > Karen Coppage > >
