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 <[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: Fri, 11 Jan 2019 22:26:05 +0000 Gerrit-HasComments: Yes
