Yongjun Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/12213 )
Change subject: IMPALA-7929: Allow null qualifier in THBaseFilter ...................................................................... Patch Set 5: (6 comments) HI Joe and Paul, Thanks a lot for the review. Sorry for late response. I just uploaded new rev to address all of them. Added another new rev to address two cosmetic thing (indentation and extra empty line). Hope it looks good. Would you please take a look at the new rev? thanks. http://gerrit.cloudera.org:8080/#/c/12213/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12213/3//COMMIT_MSG@7 PS3, Line 7: IMPALA-7929: > We always use ":" after the JIRA in our commit titles. i.e. Thanks Joe. Addressed in new rev. http://gerrit.cloudera.org:8080/#/c/12213/3//COMMIT_MSG@9 PS3, Line 9: Impala query failed with "IntenalException: Required field 'qualifier' : was not present!" on table created via hive and mapped to HBase, because : the qualifier of the HBase key key column is null in the mapped table, : and Impala required non-null q > Please wrap commit messages at about 70-75 characters. "git log" adds about Addressed in new rev. http://gerrit.cloudera.org:8080/#/c/12213/3/common/thrift/PlanNodes.thrift File common/thrift/PlanNodes.thrift: http://gerrit.cloudera.org:8080/#/c/12213/3/common/thrift/PlanNodes.thrift@287 PS3, Line 287: // The qualifier for HBase K > Please add a comment describing why this is optional. Addressed in new rev. http://gerrit.cloudera.org:8080/#/c/12213/3/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/3/fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java@285 PS3, Line 285: nalyzer); : FeHBaseTable tbl = (FeHBaseTable) desc_.getTable(); : > A couple notes on this comment: Addressed in new rev. http://gerrit.cloudera.org:8080/#/c/12213/3/fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java@285 PS3, Line 285: nalyzer); : FeHBaseTable tbl = (FeHBaseTable) desc_.getTable(); : > Well said. Agreed. http://gerrit.cloudera.org:8080/#/c/12213/3/tests/query_test/test_hbase_queries.py File tests/query_test/test_hbase_queries.py: http://gerrit.cloudera.org:8080/#/c/12213/3/tests/query_test/test_hbase_queries.py@94 PS3, Line 94: self.run_stmt_in_hive(del_table) : : : : : : : > I would prefer that this use a .test file and run_test_case() to verify the Addressed in new rev. -- 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: 5 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, 07 Feb 2019 04:18:28 +0000 Gerrit-HasComments: Yes
