Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/16094 )
Change subject: IMPALA-9691: Support Kudu Timestamp and Date bloom filter ...................................................................... Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/16094/3/be/src/exprs/timestamp-truncation-expr.h File be/src/exprs/timestamp-truncation-expr.h: http://gerrit.cloudera.org:8080/#/c/16094/3/be/src/exprs/timestamp-truncation-expr.h@18 PS3, Line 18: #ifndef IMPALA_EXPRS_TIMESTAMP_TRUNCATION_EXPR_H_ > We prefer to use '#pragma once' for new code Fixed it http://gerrit.cloudera.org:8080/#/c/16094/3/be/src/exprs/timestamp-truncation-expr.h@30 PS3, Line 30: The children of this Expr > Probably worth specifying that this should have exactly one child. Fixed it http://gerrit.cloudera.org:8080/#/c/16094/3/common/thrift/Exprs.thrift File common/thrift/Exprs.thrift: http://gerrit.cloudera.org:8080/#/c/16094/3/common/thrift/Exprs.thrift@151 PS3, Line 151: struct TTimestampTruncationExpr { > Since this doesn't contain anything, its fine to just not include it. removed it. http://gerrit.cloudera.org:8080/#/c/16094/3/fe/src/main/java/org/apache/impala/analysis/TimestampTruncationExpr.java File fe/src/main/java/org/apache/impala/analysis/TimestampTruncationExpr.java: http://gerrit.cloudera.org:8080/#/c/16094/3/fe/src/main/java/org/apache/impala/analysis/TimestampTruncationExpr.java@30 PS3, Line 30: The children > Probably worth specifying that this will have exactly one child. fixed it. http://gerrit.cloudera.org:8080/#/c/16094/3/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java File fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java: http://gerrit.cloudera.org:8080/#/c/16094/3/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@347 PS3, Line 347: // Don't create bloom filter targeting for HDFS scan node if the source : // scan slot is Kudu timestamp slot since the timestamp values stored : // in Kudu table are truncated and maybe different from the values stored : // in the HDFS tables. > I think this needs more thought. There's two things we need to worry about Removed this piece of code to keep current behavior. http://gerrit.cloudera.org:8080/#/c/16094/3/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@658 PS3, Line 658: true > We sometimes specify the names of parameters in a comment when the value is Fixed it. -- To view, visit http://gerrit.cloudera.org:8080/16094 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3c1e9bcc9fd6d79a39f25eaa3396188fc0a52a48 Gerrit-Change-Number: 16094 Gerrit-PatchSet: 3 Gerrit-Owner: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Tue, 23 Jun 2020 18:57:36 +0000 Gerrit-HasComments: Yes
