> On Oct. 10, 2019, 9 a.m., Peter Vary wrote: > > Thanks for chasing this down! > > Really appreciate it!
Thanks a lot for the review! > On Oct. 10, 2019, 9 a.m., Peter Vary wrote: > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/LeafFilterFactory.java > > Lines 157 (patched) > > <https://reviews.apache.org/r/71606/diff/1/?file=2168888#file2168888line158> > > > > This is the best way to check this? > > Is this always starts with char? CHAR? or anything else is not possible? It always start with "char", but you are right that it is not the best way to check it. I changed it to use at least the name of the CHAR serde constant. > On Oct. 10, 2019, 9 a.m., Peter Vary wrote: > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/LeafFilterFactory.java > > Lines 181 (patched) > > <https://reviews.apache.org/r/71606/diff/1/?file=2168888#file2168888line182> > > > > I do not like this. > > Either we only aim for space, or we aim for whitespace characters, but > > the check and the replace is different. You are right, thanks for pointing this out. Since the regex will always replace the whitespaces at the end of the string, the check if the string ends with space is not event necessary. If it doesn't end with space, the regex replace will do nothing. - Marta ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71606/#review218175 ----------------------------------------------------------- On Oct. 10, 2019, 11:39 a.m., Marta Kuczora wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71606/ > ----------------------------------------------------------- > > (Updated Oct. 10, 2019, 11:39 a.m.) > > > Review request for hive and Peter Vary. > > > Bugs: HIVE-21407 > https://issues.apache.org/jira/browse/HIVE-21407 > > > Repository: hive-git > > > Description > ------- > > The previous approach didn't solve all use cases. In this new approach the > hive type is sent to the Parquet PPD part and trim the value which is pushed > to the predicate in case of CHAR hive type. > > > Diffs > ----- > > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/FilterPredicateLeafBuilder.java > 5b051dd > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/LeafFilterFactory.java > fc9188f > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/ParquetRecordReaderBase.java > 033e26a > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/read/ParquetFilterPredicateConverter.java > ca5e085 > > ql/src/test/org/apache/hadoop/hive/ql/io/parquet/TestParquetRecordReaderWrapper.java > 0210a0a > > ql/src/test/org/apache/hadoop/hive/ql/io/parquet/read/TestParquetFilterPredicate.java > 7c7c657 > > ql/src/test/org/apache/hadoop/hive/ql/io/sarg/TestConvertAstToSearchArg.java > 4c40908 > ql/src/test/queries/clientpositive/parquet_ppd_char.q 386fb25 > ql/src/test/queries/clientpositive/parquet_ppd_char2.q PRE-CREATION > ql/src/test/results/clientpositive/parquet_ppd_char2.q.out PRE-CREATION > > > Diff: https://reviews.apache.org/r/71606/diff/2/ > > > Testing > ------- > > Added new q test for testing the PPD for char and varchar types. Also > extended the unit tests for the > ParquetFilterPredicateConverter.toFilterPredicate method. > > > The TestParquetRecordReaderWrapper and the TestParquetFilterPredicate are > both testing the same thing, the behavior of the > ParquetFilterPredicateConverter.toFilterPredicate method. It doesn't make > sense to have tests for the same use case in different test classes, so moved > the test cases from the TestParquetRecordReaderWrapper to > TestParquetFilterPredicate. > > > Thanks, > > Marta Kuczora > >
