Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12213 )
Change subject: IMPALA-7929. Impala query on HBASE table failing with InternalException. ...................................................................... Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/12213/1/common/thrift/PlanNodes.thrift File common/thrift/PlanNodes.thrift: http://gerrit.cloudera.org:8080/#/c/12213/1/common/thrift/PlanNodes.thrift@291 PS1, Line 291: 3: optional i32 op_ordinal : 4: optional string filter_constant > Same question Seems the ordinal is required. How would we identify the column with just a column family? Also, there is a change being made here for a reason. Would be great to add a comment to explain that case for the benefit of future readers. http://gerrit.cloudera.org:8080/#/c/12213/1/fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java File fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java: http://gerrit.cloudera.org:8080/#/c/12213/1/fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java@288 PS1, Line 288: filters_.add(thbf); What is happening in this change? Is it just splitting up the lines for easier debugging? Or, is there some actual functional difference? If a difference, would be great to add a comment to explain why we need this approach rather than the constructor. -- To view, visit http://gerrit.cloudera.org:8080/12213 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I378c2249604481067b5b1c3a3bbb28c30ad4d751 Gerrit-Change-Number: 12213 Gerrit-PatchSet: 1 Gerrit-Owner: Yongjun Zhang <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Yongjun Zhang <[email protected]> Gerrit-Comment-Date: Thu, 10 Jan 2019 22:59:28 +0000 Gerrit-HasComments: Yes
