Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/13189 )
Change subject: IMPALA-7370: DATE: Read/Write to parquet. ...................................................................... Patch Set 2: (5 comments) I did a quick run through on this review. Seems fine just have some minor observations. http://gerrit.cloudera.org:8080/#/c/13189/2/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java: http://gerrit.cloudera.org:8080/#/c/13189/2/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1267 PS2, Line 1267: // Right now, scanning DATE values is only supported for TEXT and PARQUET fileformats. I feel that listing unsupported formats could be done right above findUnsupportedDateFsPartition() instead of here. Leaving it here would make it harder for someone who introduces new fileformat support for date to find all the occurences of comments to re-write. http://gerrit.cloudera.org:8080/#/c/13189/2/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1341 PS2, Line 1341: if (ff != HdfsFileFormat.TEXT && ff != HdfsFileFormat.PARQUET) { : return part; : } nit: can this be merged into one line? http://gerrit.cloudera.org:8080/#/c/13189/2/testdata/workloads/functional-query/queries/QueryTest/date-fileformat-support.test File testdata/workloads/functional-query/queries/QueryTest/date-fileformat-support.test: http://gerrit.cloudera.org:8080/#/c/13189/2/testdata/workloads/functional-query/queries/QueryTest/date-fileformat-support.test@7 PS2, Line 7: ---- QUERY nit: Would it make sense to merge this step with the previous one as it doesn't test anything other than not throwing an error. http://gerrit.cloudera.org:8080/#/c/13189/2/tests/common/impala_connection.py File tests/common/impala_connection.py: http://gerrit.cloudera.org:8080/#/c/13189/2/tests/common/impala_connection.py@285 PS2, Line 285: r = None : try: : r = self.__fetch_results(handle, profile_format=profile_format) : finally: : if r is None: : # Try to close the query handle but ignore any exceptions not to overwrite the : # original exception raised by '__fetch_results'. : try: : self.close_query(handle) : except Exception: : pass : else: : self.close_query(handle) : return r Is this an intentional change for "date on parquet"? Isn't this an unrelated fix? http://gerrit.cloudera.org:8080/#/c/13189/2/tests/query_test/test_insert_parquet.py File tests/query_test/test_insert_parquet.py: http://gerrit.cloudera.org:8080/#/c/13189/2/tests/query_test/test_insert_parquet.py@669 PS2, Line 669: # Expected values for functional.date_tbl nit: for me this comment doesn't add any value -- To view, visit http://gerrit.cloudera.org:8080/13189 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I67da03754531660bc8de3b6935580d46deae1814 Gerrit-Change-Number: 13189 Gerrit-PatchSet: 2 Gerrit-Owner: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Tue, 30 Apr 2019 15:38:20 +0000 Gerrit-HasComments: Yes
