[Impala-ASF-CR] IMPALA-7929. Impala query on HBASE table failing with InternalException.
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 3: (1 comment) 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: Well said. -- 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 Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Wed, 23 Jan 2019 22:15:54 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7929. Impala query on HBASE table failing with InternalException.
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))
[Impala-ASF-CR] IMPALA-7929. Impala query on HBASE table failing with InternalException.
Impala Public Jenkins 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: Verified+1 -- 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 Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Sun, 20 Jan 2019 09:56:18 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7929. Impala query on HBASE table failing with InternalException.
Impala Public Jenkins 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: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3656/ DRY_RUN=true -- 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 Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Sun, 20 Jan 2019 06:00:52 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7929. Impala query on HBASE table failing with InternalException.
Yongjun Zhang 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 2: I found that even if I comment out the code at https://github.com/apache/impala/blob/1c94450ca92606fb6b708de2ea07445cc6610dbf/be/src/exec/hbase-table-scanner.cc#L394 and it still works, which seems mysterious. After some study, I found out why: when no filters are passed to hbase, impala get the all rows from hbase, then impala evaluate the predicates itself See https://github.infra.cloudera.com/CDH/Impala/blob/cdh6.x/be/src/exec/hbase-scan-node.cc#L252 This does the filtering for the predicates that involve column that has no qualifier. It's not very efficient, because we get back all rows from hbase scan, and filter out majority. It would be nice to have hbase to filter before returning to impala. Given this understanding, I think We can create a separate jira to address this performance issue, and push the fix forward. Thanks. -- 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: 2 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Sun, 20 Jan 2019 05:33:50 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7929. Impala query on HBASE table failing with InternalException.
Yongjun Zhang 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 2: > > (1 comment) > > HI Joe, thanks for the very good observation. I did some checking, > indeed the code you referred to skip this kind of entries. However, > the fact that the tests I added get the right results. Need to > understand why. Thanks. I figured out that for column mapped to hbase key, the filter is added here https://github.com/apache/impala/blob/1c94450ca92606fb6b708de2ea07445cc6610dbf/be/src/exec/hbase-table-scanner.cc#L394 I think this could explain why it still works for my unit test even though the filters are skipped in the code blocks before this one. -- 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: 2 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Mon, 14 Jan 2019 22:44:26 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7929. Impala query on HBASE table failing with InternalException.
Yongjun Zhang 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 2: > (1 comment) HI Joe, thanks for the very good observation. I did some checking, indeed the code you referred to skip this kind of entries. However, the fact that the tests I added get the right results. Need to understand why. Thanks. -- 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: 2 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Mon, 14 Jan 2019 16:24:17 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7929. Impala query on HBASE table failing with InternalException.
Impala Public Jenkins 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 2: Verified+1 -- 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: 2 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Sat, 12 Jan 2019 02:15:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7929. Impala query on HBASE table failing with InternalException.
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 1: (1 comment) 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@287 PS1, Line 287: 2: optional string qualifier > Thanks Joe, I thought once we make something optional in the middle, the on Ok, so a null in the Java side becomes an empty string in C++. Then when we pass it back to Java via CreateByteArray, it becomes HConstants.EMPTY_START_ROW, which is HConstants.EMPTY_BYTE_ARRAY. It turns out that inside org.apache.hadoop.hbase.client.Scan.addColumn(), it converts qualifier == null to HConstants.EMPTY_BYTE_ARARY. So, that sounds right. This line bugs me: https://github.com/apache/impala/blob/1c94450ca92606fb6b708de2ea07445cc6610dbf/be/src/exec/hbase-table-scanner.cc#L328 -- 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 Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Fri, 11 Jan 2019 23:48:42 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7929. Impala query on HBASE table failing with InternalException.
Impala Public Jenkins 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 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1780/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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: 2 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Fri, 11 Jan 2019 22:43:27 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7929. Impala query on HBASE table failing with InternalException.
Yongjun Zhang 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 2:
(8 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@287
PS1, Line 287: 2: optional string qualifier
> We write this in Java and read it in C++ (hbase-table-scanner.cc). In Java,
Thanks Joe, I thought once we make something optional in the middle, the ones
that follow it need to be too. But it seems I was wrong about that. I changed
it in the new rev, such that only qualifier is optional.
In C++ world, I saw that qualifier is by default constructed as empty string. I
also observed the same from here
https://github.com/gdb/impala/blob/master/be/src/exec/hbase-table-scanner.cc#L315
http://gerrit.cloudera.org:8080/#/c/12213/1/common/thrift/PlanNodes.thrift@291
PS1, Line 291: 3: required i32 op_ordinal
: 4: required string filter_constant
> Do these need to be optional? Is filter_constant ever null?
Thanks Joe, Tim and Paul. I made only qualifier optional in the new rev now. I
originally thought once we make something optional in the middle, the ones that
follow it need to be too. But I was wrong about that.
http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py
File tests/query_test/test_hbase_queries.py:
http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py@28
PS1, Line 28: )
> flake8: E123 closing bracket does not match indentation of opening bracket'
Oops, sorry, I missed this one in the new rev. Will do in next.
http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py@73
PS1, Line 73: def test_hbase_col_name(self, unique_database):
> line has trailing whitespace
Solved in new rev. thanks.
http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py@77
PS1, Line 77: table_name =
"{0}.hbase_col_name_testkeyx".format(unique_database)
> We generally prefer using {0}, {1} and .format() over the % operator in new
Solved in new rev. Thanks.
http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py@78
PS1, Line 78: cr_table = """CREATE TABLE {0} (k STRING, c STRING) STORED BY
:'org.apache.hadoop.hive.hbase.HBaseStorageHandler'
WITH SERDEPROPERTIES
:('hbase.columns.mapping'=':key,cf:c',
'serialization.format'='1')
:TBLPROPERTIES ('hbase.table.name'=\'{1}\',
:
'storage_handler'='org.apache.hadoop.hive.hbase.HBaseStorageHandler')
:""".format(table_name, table_name)
:
> Couple things here:
Solved in new rev. Thanks.
http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py@89
PS1, Line 89: self.run_stmt_in_hive(cr_table)
> We should validate that the results are correct just to make sure that we h
Added data and new queries to do that in the new rev. Since I have to create a
table, then run query, I modified the test I added to do so. Hope it looks
good. Thanks.
http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py@91
PS1, Line 91:
> This is not needed - the unique_database handles deleting the database
When I run the same tests again and again, if I don't have this, it will fail
complaining the hbase already exists. After I added this, then the test can be
run multiple times without issue. Seems there is something for us to understand?
--
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: 2
Gerrit-Owner: Yongjun Zhang
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell
Gerrit-Reviewer: Paul Rogers
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Yongjun Zhang
Gerrit-Comment-Date: Fri, 11 Jan 2019 22:26:05 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7929. Impala query on HBASE table failing with InternalException.
Yongjun Zhang has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/12213 ) Change subject: IMPALA-7929. Impala query on HBASE table failing with InternalException. .. IMPALA-7929. Impala query on HBASE table failing with InternalException. 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. Test: Added unit test. Tested in real cluster. Change-Id: I378c2249604481067b5b1c3a3bbb28c30ad4d751 --- M common/thrift/PlanNodes.thrift M fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java M tests/query_test/test_hbase_queries.py 3 files changed, 54 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/12213/2 -- 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: newpatchset Gerrit-Change-Id: I378c2249604481067b5b1c3a3bbb28c30ad4d751 Gerrit-Change-Number: 12213 Gerrit-PatchSet: 2 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Yongjun Zhang
[Impala-ASF-CR] IMPALA-7929. Impala query on HBASE table failing with InternalException.
Impala Public Jenkins 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 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/12213/2/tests/query_test/test_hbase_queries.py File tests/query_test/test_hbase_queries.py: http://gerrit.cloudera.org:8080/#/c/12213/2/tests/query_test/test_hbase_queries.py@28 PS2, Line 28: ) flake8: E123 closing bracket does not match indentation of opening bracket's line http://gerrit.cloudera.org:8080/#/c/12213/2/tests/query_test/test_hbase_queries.py@94 PS2, Line 94: = flake8: E225 missing whitespace around operator http://gerrit.cloudera.org:8080/#/c/12213/2/tests/query_test/test_hbase_queries.py@96 PS2, Line 96: = flake8: E225 missing whitespace around operator http://gerrit.cloudera.org:8080/#/c/12213/2/tests/query_test/test_hbase_queries.py@98 PS2, Line 98: = flake8: E225 missing whitespace around operator http://gerrit.cloudera.org:8080/#/c/12213/2/tests/query_test/test_hbase_queries.py@100 PS2, Line 100: = flake8: E225 missing whitespace around operator -- 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: 2 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Fri, 11 Jan 2019 22:14:41 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7929. Impala query on HBASE table failing with InternalException.
Impala Public Jenkins 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 2: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3634/ DRY_RUN=true -- 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: 2 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Fri, 11 Jan 2019 22:13:58 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7929. Impala query on HBASE table failing with InternalException.
Impala Public Jenkins 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: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/3630/ -- 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 Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Fri, 11 Jan 2019 01:45:50 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7929. Impala query on HBASE table failing with InternalException.
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 Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Thu, 10 Jan 2019 22:59:28 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7929. Impala query on HBASE table failing with InternalException.
Yongjun Zhang 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: Hi Joe and Tim, thanks a lot for the prompt review, all great comments! I will address them in next rev asap. -- 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 Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Thu, 10 Jan 2019 22:32:06 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7929. Impala query on HBASE table failing with InternalException.
Tim Armstrong 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:
(4 comments)
Joe beat me to it! I think my comments aren't totally redundant.
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
> Do these need to be optional? Is filter_constant ever null?
Same question
http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py
File tests/query_test/test_hbase_queries.py:
http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py@77
PS1, Line 77: table_name = "%s.hbase_col_name_testkeyb" % unique_database
We generally prefer using {0}, {1} and .format() over the % operator in new
code, it would be good to convert over.
http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py@89
PS1, Line 89: self.client.execute("select * from %s where k != 'row1'" %
table_name)
We should validate that the results are correct just to make sure that we
haven't traded one bug for another. The best way to do this is to create a
.test file with the query and expected results.
http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py@91
PS1, Line 91: self.run_stmt_in_hive(del_table)
This is not needed - the unique_database handles deleting the database
--
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
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Yongjun Zhang
Gerrit-Comment-Date: Thu, 10 Jan 2019 21:46:34 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7929. Impala query on HBASE table failing with InternalException.
Impala Public Jenkins 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: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3630/ DRY_RUN=true -- 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 Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Thu, 10 Jan 2019 21:45:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7929. Impala query on HBASE table failing with InternalException.
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 1:
(4 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@287
PS1, Line 287: 2: optional string qualifier
We write this in Java and read it in C++ (hbase-table-scanner.cc). In Java,
optional allows us to leave this as null, which is equivalent to unset. What is
happening on the C++ side? What value does it see? Does that value get conveyed
as null when the C++ side uses JNI to call back into Java?
By the way, you can examine the code that thrift generates by looking in
be/generated-sources/gen-cpp for C++ and fe/generated-sources/gen-java for the
Java.
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
Do these need to be optional? Is filter_constant ever null?
http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py
File tests/query_test/test_hbase_queries.py:
http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py@68
PS1, Line 68: @SkipIfIsilon.hive
> line has trailing whitespace
You are going to want to set up your editor to avoid trailing whitespace and
flag it proactively. For emacs, I think I use this:
(setq-default show-trailing-whitespace t)
http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py@78
PS1, Line 78: cr_table = ("CREATE TABLE %s (k STRING, c STRING) STORED BY "
:
"'org.apache.hadoop.hive.hbase.HBaseStorageHandler' WITH "
:"SERDEPROPERTIES
('hbase.columns.mapping'=':key,cf:c',"
:" 'serialization.format'='1') TBLPROPERTIES "
:"('hbase.table.name'=\'%s\', "
:
"'storage_handler'='org.apache.hadoop.hive.hbase.HBaseStorageHandler')") \
:% (table_name, table_name)
Couple things here:
1. Use a """ multiline python string and make this more readable. See for
example:
https://github.com/apache/impala/blob/master/tests/query_test/test_insert_behaviour.py#L652
2. Use "{0}".format(blah) style replacements.
--
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
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Yongjun Zhang
Gerrit-Comment-Date: Thu, 10 Jan 2019 21:44:22 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7929. Impala query on HBASE table failing with InternalException.
Impala Public Jenkins 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: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1768/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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 Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Thu, 10 Jan 2019 17:18:15 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7929. Impala query on HBASE table failing with InternalException.
Yongjun Zhang 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: Hello Joe and Tim, I just posted a patch. Would you please help taking a look? Thanks a lot. Many thanks to Joe for the discussions about solutions and testing. -- 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 Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Thu, 10 Jan 2019 16:49:50 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7929. Impala query on HBASE table failing with InternalException.
Impala Public Jenkins 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: (13 comments) http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py File tests/query_test/test_hbase_queries.py: http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py@28 PS1, Line 28: ) flake8: E123 closing bracket does not match indentation of opening bracket's line http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py@68 PS1, Line 68: flake8: W291 trailing whitespace http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py@68 PS1, Line 68: @SkipIfIsilon.hive line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py@69 PS1, Line 69: flake8: W291 trailing whitespace http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py@69 PS1, Line 69: @SkipIfS3.hive line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py@70 PS1, Line 70: flake8: W291 trailing whitespace http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py@70 PS1, Line 70: @SkipIfABFS.hive line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py@71 PS1, Line 71: flake8: W291 trailing whitespace http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py@71 PS1, Line 71: @SkipIfADLS.hive line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py@72 PS1, Line 72: flake8: W291 trailing whitespace http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py@72 PS1, Line 72: @SkipIfLocal.hive line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py@73 PS1, Line 73: flake8: W291 trailing whitespace http://gerrit.cloudera.org:8080/#/c/12213/1/tests/query_test/test_hbase_queries.py@73 PS1, Line 73: def test_hbase_col_name(self, unique_database): line has trailing whitespace -- 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 Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 10 Jan 2019 16:48:35 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7929. Impala query on HBASE table failing with InternalException.
Yongjun Zhang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12213 Change subject: IMPALA-7929. Impala query on HBASE table failing with InternalException. .. IMPALA-7929. Impala query on HBASE table failing with InternalException. 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. Test: Added unit test. Tested in real cluster. Change-Id: I378c2249604481067b5b1c3a3bbb28c30ad4d751 --- M common/thrift/PlanNodes.thrift M fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java M tests/query_test/test_hbase_queries.py 3 files changed, 41 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/12213/1 -- 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: newchange Gerrit-Change-Id: I378c2249604481067b5b1c3a3bbb28c30ad4d751 Gerrit-Change-Number: 12213 Gerrit-PatchSet: 1 Gerrit-Owner: Yongjun Zhang
