Alex Behm has posted comments on this change. Change subject: IMPALA-2716: Hive/Impala incompatibility for timestamp data in Parquet ......................................................................
Patch Set 5: (10 comments) I'm pretty happy with the code! Moving on to the tests. http://gerrit.cloudera.org:8080/#/c/5939/5/be/src/exec/hdfs-scan-node-base.h File be/src/exec/hdfs-scan-node-base.h: Line 305: // parquet-mr. Comment that the FE guarantees that this is a valid timezone. http://gerrit.cloudera.org:8080/#/c/5939/5/be/src/exec/parquet-column-readers.cc File be/src/exec/parquet-column-readers.cc: Line 246: DCHECK((is_timestamp_dependent_timezone_ && timezone_ == NULL) || simplify condition: is_timestamp_dependent_timezone_ == (timezone_ == NULL) Line 549: /// Used to cache the timezone object corresponding to "parquet.mr.int96.write.zone" the "parquet.mr.int96.write.zone" table property Line 550: /// table property to avoid repeated calls to 'TimezoneDatabase::FindTimezone()'. no need to single-quote here http://gerrit.cloudera.org:8080/#/c/5939/5/be/src/exprs/timestamp-functions.cc File be/src/exprs/timestamp-functions.cc: Line 190: // See if they specified a zone id Who is "they"? Suggest rephrasing Line 202: return time_zone_ptr(); return NULL? http://gerrit.cloudera.org:8080/#/c/5939/5/common/thrift/BackendGflags.thrift File common/thrift/BackendGflags.thrift: Line 58: // If true, all newly created Parquet tables will have the parquet.mr.int96.write.zone ... all newly created HDFS tables regardless of format ... http://gerrit.cloudera.org:8080/#/c/5939/5/common/thrift/PlanNodes.thrift File common/thrift/PlanNodes.thrift: Line 218: 9: optional string parquet_mr_write_zone; remove trailing semicolon for consistency http://gerrit.cloudera.org:8080/#/c/5939/5/fe/src/main/java/org/apache/impala/analysis/BaseTableRef.java File fe/src/main/java/org/apache/impala/analysis/BaseTableRef.java: Line 112: throw new AnalysisException("Invalid Time Zone: " + timezone); Mention that the timezone is in the table property 'parquet.mr.int96.write.zone'. Otherwise, the user might have no clue where this timezone comes from. http://gerrit.cloudera.org:8080/#/c/5939/5/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java: Line 186: "Only HDFS tables can use '%s' table property.", the '%s' table property -- To view, visit http://gerrit.cloudera.org:8080/5939 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3f24525ef45a2814f476bdee76655b30081079d6 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Attila Jeges <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Zoltan Ivanfi <[email protected]> Gerrit-HasComments: Yes
