Joe McDonnell 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 3: (6 comments) 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. IMPALA-7929: blah http://gerrit.cloudera.org:8080/#/c/12213/3//COMMIT_MSG@7 PS3, Line 7: Impala query on HBASE table failing with InternalException. Minor comment about titles: Usually describing what you are changing is more useful down the road than describing the symptom of the JIRA. There are often multiple JIRAs that have similar symptoms but different fixes. There are also times when we merge multiple things for one JIRA, and keeping those separate is useful. It is worth reading through some commits on "git log" to get a sense of the varieties of titles that people use. 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 requires non-null qualifier. The fix : here relaxes this requirement. Please wrap commit messages at about 70-75 characters. "git log" adds about 4 characters before a message body, so it is nice if this fits in a standard 80 character terminal. Take a look at "git log" and follow what other developers are doing. (An exception to this is when there are long names or formatted lines like stack traces.) Separately, a couple notes: I would like the comment to be more specific. What is changing and why does it fix the issue? If someone looked up IMPALA-7929, would they be able to figure out what was wrong and how it was fixed? To me, "relaxes this requirement" doesn't tell me what specifically changed. 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: 2: optional string qualifier Please add a comment describing why this is optional. 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: Thus qualifier is changed from Required to Optional in thrift, : // and the code here is changed accordingly to use the new constructor that : // doesn't require qualifier. A couple notes on this comment: 1. To me, when I read "is changed from X to Y", that tense only really makes sense in the context of this code change / code review. Once this is merged, it doesn't make sense because it is already done and completed. (This tense might be permissible in a commit message, because it is describing what is changing in this commit.) Most code comments describe the existing code in the present tense. "X is optional because blah." Sometimes comments describe what changed, and this uses the past tense. "JIRA-Y changed this to do Z." 2. Comments exist to answer a question. If the code is very clear, developers are not usually asking "What is the code doing?" They are more likely to ask "Why is the code this way?" It is pretty clear that we are constructing the THBaseFilter and setting the qualifier separately. The comment here should answer the question "Why is the qualifier special and set separately?" I would write this comment as: IMPALA-7929: Since the qualifier can be null (e.g. for the key column of an HBase table), the qualifier field must be optional in order to express the null value. Constructors in Thrift do not set optional fields, so the qualifier must be set separately. 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: result=self.client.execute("select * from {0} where k != 'row1'".format(table_name)) : assert(len(result.data) == 3) : result=self.client.execute("select * from {0} where k = 'row1'".format(table_name)) : assert(len(result.data) == 1) : result=self.client.execute("select * from {0} where c != 'c2'".format(table_name)) : assert(len(result.data) == 2) : result=self.client.execute("select * from {0} where c = 'c4'".format(table_name)) : assert(len(result.data) == 1) I would prefer that this use a .test file and run_test_case() to verify the results. -- 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: 3 Gerrit-Owner: Yongjun Zhang <yjzhan...@apache.org> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Paul Rogers <prog...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Yongjun Zhang <yjzhan...@apache.org> Gerrit-Comment-Date: Wed, 23 Jan 2019 21:52:20 +0000 Gerrit-HasComments: Yes
