[Impala-ASF-CR] IMPALA-7929: Allow null qualifier in THBaseFilter
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 8: > Change has been successfully rebased and submitted as > 06d078e76bad6944ad487fd432ff6d0aa9174960 by Tim Armstrong Thanks so much again Joe, Tim and Paul! -- 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: 8 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, 08 Feb 2019 01:17:10 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7929: Allow null qualifier in THBaseFilter
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 7: > Uploaded patch set 7: Commit message was updated. Noticed a duplicate word in the commit message. Submitted the patch again with the duplicate word removed. -- 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: 7 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, 07 Feb 2019 20:20:35 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7929: Allow null qualifier in THBaseFilter
Hello Paul Rogers, Tim Armstrong, Joe McDonnell, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12213 to look at the new patch set (#7). Change subject: IMPALA-7929: Allow null qualifier in THBaseFilter .. IMPALA-7929: Allow null qualifier in THBaseFilter 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 column is null in the mapped table, and Impala required non-null qualifier. The fix here is to relax 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 A testdata/workloads/functional-query/queries/QueryTest/hbase-col-filter.test M tests/query_test/test_hbase_queries.py 4 files changed, 70 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/12213/7 -- 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: 7 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: Allow null qualifier in THBaseFilter
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 6: > Thanks for fixing this! Thank Joe, Tim and Paul a lot for the review! -- 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: 6 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, 07 Feb 2019 19:04:07 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7929: Allow null qualifier in THBaseFilter
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 6: Uploaded patch set 6 that removed an extra empty line. -- 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: 6 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, 07 Feb 2019 17:14:05 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7929: Allow null qualifier in THBaseFilter
Hello Paul Rogers, Tim Armstrong, Joe McDonnell, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12213 to look at the new patch set (#6). Change subject: IMPALA-7929: Allow null qualifier in THBaseFilter .. IMPALA-7929: Allow null qualifier in THBaseFilter 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 qualifier. The fix here is to relax 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 A testdata/workloads/functional-query/queries/QueryTest/hbase-col-filter.test M tests/query_test/test_hbase_queries.py 4 files changed, 70 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/12213/6 -- 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: 6 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: Allow null qualifier in THBaseFilter
Hello Paul Rogers, Tim Armstrong, Joe McDonnell, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12213 to look at the new patch set (#5). Change subject: IMPALA-7929: Allow null qualifier in THBaseFilter .. IMPALA-7929: Allow null qualifier in THBaseFilter 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 qualifier. The fix here is to relax 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 A testdata/workloads/functional-query/queries/QueryTest/hbase-col-filter.test M tests/query_test/test_hbase_queries.py 4 files changed, 71 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/12213/5 -- 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: 5 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: Allow null qualifier in THBaseFilter
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 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, 07 Feb 2019 04:18:28 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7929: Allow null qualifier in THBaseFilter
Hello Paul Rogers, Tim Armstrong, Joe McDonnell, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12213 to look at the new patch set (#4). Change subject: IMPALA-7929: Allow null qualifier in THBaseFilter .. IMPALA-7929: Allow null qualifier in THBaseFilter 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 qualifier. The fix here is to relax 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 A testdata/workloads/functional-query/queries/QueryTest/hbase-col-filter.test M tests/query_test/test_hbase_queries.py 4 files changed, 73 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/12213/4 -- 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: 4 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.
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.
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.
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.
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.
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
[Impala-ASF-CR] IMPALA-6742: Profiles of running queries should include execution summary.
Yongjun Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11591 ) Change subject: IMPALA-6742: Profiles of running queries should include execution summary. .. Patch Set 6: > Change has been successfully rebased and submitted as > e9652a48dd00c3c076ddccaa940d074b6970b7fc by Joe McDonnell Many thanks Joe and Tim for the review and commit! --Yongjun -- To view, visit http://gerrit.cloudera.org:8080/11591 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idc7f714c9427d4b26d4e78cf27ceca2b0b336699 Gerrit-Change-Number: 11591 Gerrit-PatchSet: 6 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Wed, 09 Jan 2019 17:31:53 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6742: Profiles of running queries should include execution summary.
Yongjun Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11591 ) Change subject: IMPALA-6742: Profiles of running queries should include execution summary. .. Patch Set 5: > Patch Set 4: Code-Review+1 Many thanks again for the review Joe. I just updated a new rev that fixes the flake8 issue. -- To view, visit http://gerrit.cloudera.org:8080/11591 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idc7f714c9427d4b26d4e78cf27ceca2b0b336699 Gerrit-Change-Number: 11591 Gerrit-PatchSet: 5 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Fri, 04 Jan 2019 23:19:56 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6742: Profiles of running queries should include execution summary.
Yongjun Zhang has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/11591 ) Change subject: IMPALA-6742: Profiles of running queries should include execution summary. .. IMPALA-6742: Profiles of running queries should include execution summary. Currently execution summary is not included in the profiles of running queries, and it's only reported when the query is finished. This jira makes the execution summary to the profile reported when queries are still running. Testing: Added unit test. Done with real cluster. Change-Id: Idc7f714c9427d4b26d4e78cf27ceca2b0b336699 --- M be/src/service/impala-server.cc M be/src/service/impala-server.h M tests/query_test/test_observability.py 3 files changed, 42 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/91/11591/5 -- To view, visit http://gerrit.cloudera.org:8080/11591 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Idc7f714c9427d4b26d4e78cf27ceca2b0b336699 Gerrit-Change-Number: 11591 Gerrit-PatchSet: 5 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Yongjun Zhang
[Impala-ASF-CR] IMPALA-5474: Adding a trivial subquery turns error into warning
Yongjun Zhang has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/12022 ) Change subject: IMPALA-5474: Adding a trivial subquery turns error into warning .. IMPALA-5474: Adding a trivial subquery turns error into warning After adding a subquery to a query that fails with ERROR, it fails with WARNING. The fix here makes it return ERROR. Testing: Added unit tests; Done real cluster testing with reported cases. Change-Id: Ibedb11dd3d50bcdb21d508f7d21691925491946e --- M shell/impala_client.py M tests/shell/test_shell_commandline.py 2 files changed, 36 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/22/12022/5 -- To view, visit http://gerrit.cloudera.org:8080/12022 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ibedb11dd3d50bcdb21d508f7d21691925491946e Gerrit-Change-Number: 12022 Gerrit-PatchSet: 5 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-5474: Adding a trivial subquery turns error into warning
Yongjun Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/12022 ) Change subject: IMPALA-5474: Adding a trivial subquery turns error into warning .. Patch Set 5: > (1 comment) Thanks Tim. I had some discussions with Joe, and updated the comment in rev5. Would you please take a look? thanks. -- To view, visit http://gerrit.cloudera.org:8080/12022 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibedb11dd3d50bcdb21d508f7d21691925491946e Gerrit-Change-Number: 12022 Gerrit-PatchSet: 5 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, 04 Jan 2019 06:23:49 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5474: Adding a trivial subquery turns error into warning
Yongjun Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/12022 ) Change subject: IMPALA-5474: Adding a trivial subquery turns error into warning .. Patch Set 4: (1 comment) > Please file a bug for the flaky test - we need to investigate that > and fix it (it seems like it could be a product bug). Agree, we can file a jira after the jira is committed, since the failed test is from testing uncommitted code. http://gerrit.cloudera.org:8080/#/c/12022/4/shell/impala_client.py File shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/12022/4/shell/impala_client.py@543 PS4, Line 543: """Returns ERROR message for the last_query_handle. Caller knows that the query has > This comment is misleading now, which is worse than no comment - this retur Hi Tim, thanks for your review and comment. Sorry for my late reply. This method does get error or ERROR when there is, because "True" is passed to the "warn" parameter of get_warn_or_error_log. Maybe we can change the method name from get_error_log to get_error_log_if_there_is? If so, we can change the other places accordingly. Or we can modify the comment to indicate the same thing. e.g., "Returns ERROR message for the last_query_handle when applicable. ...". I prefer the latter. What do you think? thanks. -- To view, visit http://gerrit.cloudera.org:8080/12022 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibedb11dd3d50bcdb21d508f7d21691925491946e Gerrit-Change-Number: 12022 Gerrit-PatchSet: 4 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, 03 Jan 2019 19:55:00 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6742: Profiles of running queries should include execution summary.
Yongjun Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11591 ) Change subject: IMPALA-6742: Profiles of running queries should include execution summary. .. Patch Set 4: BTW, the newly added unit test does fail without my fix. Thanks. -- To view, visit http://gerrit.cloudera.org:8080/11591 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idc7f714c9427d4b26d4e78cf27ceca2b0b336699 Gerrit-Change-Number: 11591 Gerrit-PatchSet: 4 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Thu, 27 Dec 2018 05:43:45 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6742: Profiles of running queries should include execution summary.
Yongjun Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11591 ) Change subject: IMPALA-6742: Profiles of running queries should include execution summary. .. Patch Set 4: > I would like there to be a test that breaks if this change is not > there (or somehow this breaks later). > query_test/test_observability.py is the right place to add this > test. > Here are a couple ways to do this: > 1. Use a sleep in the query. See query_test/test_rows_availability.py: > "select * from functional.alltypestiny where month = 1 and bool_col > = sleep(1000);" will take two seconds. Read that test and > understand why the sleep works. > 2. Execute a query that returns rows, and get the profile before > fetching any of the rows. If we haven't called fetch, then the > client has not called ImpalaServer::UnregisterQuery(). > Of these, I think I prefer #2. See test_exec_summary() for an > example. > Unrelated to your change, I would like you to hand test your change > by putting a sleep between these two lines: > https://github.com/apache/impala/blob/master/be/src/service/client-request-state.cc#L516-L517 > Then fetch the profile and verify it doesn't crash. I have a theory > that this might not work. Hi Joe, Thanks a lot for the review and comments and sorry for getting back to this late. Very good suggestions! I just uploaded a new rev to address it by adding a new unit test per your comment #2. I also did the manual test of adding a sleep call between L516-L517, I did not observe behavior difference. Would you please help taking a look? Thanks. -- To view, visit http://gerrit.cloudera.org:8080/11591 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idc7f714c9427d4b26d4e78cf27ceca2b0b336699 Gerrit-Change-Number: 11591 Gerrit-PatchSet: 4 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Thu, 27 Dec 2018 04:19:35 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6742: Profiles of running queries should include execution summary.
Yongjun Zhang has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/11591 ) Change subject: IMPALA-6742: Profiles of running queries should include execution summary. .. IMPALA-6742: Profiles of running queries should include execution summary. Currently execution summary is not included in the profiles of running queries, and it's only reported when the query is finished. This jira makes the execution summary to the profile reported when queries are still running. Testing: Added unit test. Done with real cluster. Change-Id: Idc7f714c9427d4b26d4e78cf27ceca2b0b336699 --- M be/src/service/impala-server.cc M be/src/service/impala-server.h M tests/query_test/test_observability.py 3 files changed, 41 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/91/11591/4 -- To view, visit http://gerrit.cloudera.org:8080/11591 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Idc7f714c9427d4b26d4e78cf27ceca2b0b336699 Gerrit-Change-Number: 11591 Gerrit-PatchSet: 4 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Yongjun Zhang
[Impala-ASF-CR] IMPALA-5474: Adding a trivial subquery turns error into warning
Yongjun Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/12022 ) Change subject: IMPALA-5474: Adding a trivial subquery turns error into warning .. Patch Set 4: > Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/3561/ The failed test https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/3863/ appears to be not caused by my change. This new rev only has some additional comments added, comparing with last rev that had successful test run. Thanks. -- To view, visit http://gerrit.cloudera.org:8080/12022 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibedb11dd3d50bcdb21d508f7d21691925491946e Gerrit-Change-Number: 12022 Gerrit-PatchSet: 4 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, 13 Dec 2018 04:45:36 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5474: Adding a trivial subquery turns error into warning
Yongjun Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/12022 ) Change subject: IMPALA-5474: Adding a trivial subquery turns error into warning .. Patch Set 4: Thanks much for the review and comments Tim! I added comments to the new methods. -- To view, visit http://gerrit.cloudera.org:8080/12022 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibedb11dd3d50bcdb21d508f7d21691925491946e Gerrit-Change-Number: 12022 Gerrit-PatchSet: 4 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, 12 Dec 2018 23:09:45 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5474: Adding a trivial subquery turns error into warning
Yongjun Zhang has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/12022 ) Change subject: IMPALA-5474: Adding a trivial subquery turns error into warning .. IMPALA-5474: Adding a trivial subquery turns error into warning After adding a subquery to a query that fails with ERROR, it fails with WARNING. The fix here makes it return ERROR. Testing: Added unit tests; Done real cluster testing with reported cases. Change-Id: Ibedb11dd3d50bcdb21d508f7d21691925491946e --- M shell/impala_client.py M tests/shell/test_shell_commandline.py 2 files changed, 33 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/22/12022/4 -- To view, visit http://gerrit.cloudera.org:8080/12022 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ibedb11dd3d50bcdb21d508f7d21691925491946e Gerrit-Change-Number: 12022 Gerrit-PatchSet: 4 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-5474: Adding a trivial subquery turns error into warning
Yongjun Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/12022 ) Change subject: IMPALA-5474: Adding a trivial subquery turns error into warning .. Patch Set 3: > Yeah WARNING isn't a query state, it's just that the error log > mechanism can be used to return warnings that don't fail the query. > > It should be valid to call fetch on any query. Since > https://issues.apache.org/jira/browse/IMPALA-5903 was fixed every > query type has some kind of result set (even if it's just a status > message). impala-shell tries to be a bit more clever and report DML > results and similar in a special way, but other clients just fetch > the result set. > > If you look at ImpalaServer::FetchInternal() if called once the > query is in the EXCEPTION state it will end up raising the query > status as a BeeswaxException. > Yeah WARNING isn't a query state, it's just that the error log > mechanism can be used to return warnings that don't fail the query. > > It should be valid to call fetch on any query. Since > https://issues.apache.org/jira/browse/IMPALA-5903 was fixed every > query type has some kind of result set (even if it's just a status > message). impala-shell tries to be a bit more clever and report DML > results and similar in a special way, but other clients just fetch > the result set. > > If you look at ImpalaServer::FetchInternal() if called once the > query is in the EXCEPTION state it will end up raising the query > status as a BeeswaxException. HI Tim, Thanks a lot for the review and comments. I agree with you that calling fetch() would make the two queries end that the same state. However, adding the logic I mentioned earlier would complicate the wait_to_finish() method quite a bit. My thinking is that, we call get_warning_log() all over the places, even though we know some are ERROR and some are warning. So I propose adding a new method get_error_log() so we have two methods to choose from. To fix the jira here, we simply need to replace the one called when EXCEPTION state is detected with get_error_log. This way, the code looks clear, and we don't need to touch any other testcases I touched with previous rev. Would you please take a look at rev3 I just uploaded? Thanks a lot. -- To view, visit http://gerrit.cloudera.org:8080/12022 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibedb11dd3d50bcdb21d508f7d21691925491946e Gerrit-Change-Number: 12022 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, 12 Dec 2018 00:29:15 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5474: Adding a trivial subquery turns error into warning
Yongjun Zhang has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/12022 ) Change subject: IMPALA-5474: Adding a trivial subquery turns error into warning .. IMPALA-5474: Adding a trivial subquery turns error into warning After adding a subquery to a query that fails with ERROR, it fails with WARNING. The fix here makes it return ERROR. Testing: Added unit tests; Done real cluster testing with reported cases. Change-Id: Ibedb11dd3d50bcdb21d508f7d21691925491946e --- M shell/impala_client.py M tests/shell/test_shell_commandline.py 2 files changed, 27 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/22/12022/3 -- To view, visit http://gerrit.cloudera.org:8080/12022 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ibedb11dd3d50bcdb21d508f7d21691925491946e Gerrit-Change-Number: 12022 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
[Impala-ASF-CR] IMPALA-5474: Adding a trivial subquery turns error into warning
Yongjun Zhang has posted comments on this change. (
http://gerrit.cloudera.org:8080/12022 )
Change subject: IMPALA-5474: Adding a trivial subquery turns error into warning
..
Patch Set 2:
> (1 comment)
Thanks Tim, good points. The experiment you did is exactly what I did to find
out where the WARN/ERROR messages are from.
Internally we only have the following states:
beeswax::QueryState::type ClientRequestState::BeeswaxQueryState() const {
switch (operation_state_) {
case TOperationState::INITIALIZED_STATE: return
beeswax::QueryState::CREATED;
case TOperationState::PENDING_STATE: return beeswax::QueryState::COMPILED;
case TOperationState::RUNNING_STATE: return beeswax::QueryState::RUNNING;
case TOperationState::FINISHED_STATE: return beeswax::QueryState::FINISHED;
case TOperationState::ERROR_STATE: return beeswax::QueryState::EXCEPTION;
default: {
DCHECK(false) << "Add explicit translation for all used TOperationState
values";
return beeswax::QueryState::EXCEPTION;
}
}
}
We can see here that ERROR is treated the same as EXCEPTION in the above code.
If some EXCEPTION is not the same as ERROR, then we want to have a distinct
state to distinguish. For example, we may have EXCEPTION_ERROR, and
EXCEPTION_WARN states.
Due to lack of two distinct states, we get into the situation that either way
the wait_to_finish function can be wrong: if we issue a WARN there, it may be
an ERROR; if we issue an ERROR, it may be just a WARN.
Your suggestion to call fetch() when in EXCEPTION state sounds good, but is it
always valid to try fetch() when in EXCEPTION state? There is some logic/code
between wait_to_finish and fetch() in impala_shell.py. We may need to apply the
same logic inside wait_to_finish when detecting EXCEPTION state.
Thanks.
> (1 comment)
--
To view, visit http://gerrit.cloudera.org:8080/12022
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibedb11dd3d50bcdb21d508f7d21691925491946e
Gerrit-Change-Number: 12022
Gerrit-PatchSet: 2
Gerrit-Owner: Yongjun Zhang
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Paul Rogers
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Yongjun Zhang
Gerrit-Comment-Date: Tue, 04 Dec 2018 06:41:40 +
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5474: Adding a trivial subquery turns error into warning
Yongjun Zhang has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/12022 ) Change subject: IMPALA-5474: Adding a trivial subquery turns error into warning .. IMPALA-5474: Adding a trivial subquery turns error into warning After adding a subquery to a query that fails with ERROR, it fails with WARNING. The fix here makes it return ERROR. Testing: Added unit tests; Done real cluster testing with reported cases. Change-Id: Ibedb11dd3d50bcdb21d508f7d21691925491946e --- M shell/impala_client.py M shell/impala_shell.py M tests/shell/test_shell_commandline.py 3 files changed, 33 insertions(+), 12 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/22/12022/2 -- To view, visit http://gerrit.cloudera.org:8080/12022 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ibedb11dd3d50bcdb21d508f7d21691925491946e Gerrit-Change-Number: 12022 Gerrit-PatchSet: 2 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers
[Impala-ASF-CR] IMPALA-5474: Adding a trivial subquery turns error into warning
Yongjun Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/12022 ) Change subject: IMPALA-5474: Adding a trivial subquery turns error into warning .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/12022/1/tests/shell/test_shell_commandline.py File tests/shell/test_shell_commandline.py: http://gerrit.cloudera.org:8080/#/c/12022/1/tests/shell/test_shell_commandline.py@220 PS1, Line 220: > flake8: E501 line too long (115 > 90 characters) Done http://gerrit.cloudera.org:8080/#/c/12022/1/tests/shell/test_shell_commandline.py@220 PS1, Line 220: > The error message has specific numbers, including the actual number of colu Hi Paul, Thanks a lot for the review and comments. I reused the existing table that I believe was handcrafted to have the 11/10 mismatch. This test is also used in other places in other tests (see ./testdata/workloads/functional-query/queries/QueryTest/parquet-abort-on-error.test). So the 11/10 numbers are essentially hardcoded for this testdata. So I'm not sure whether it's worth more work to parameterize the test. But in general I agreeing that parameterizing stuff is helpful. I just updated new rev to fix the line length, and I also added some comments to the test. Thanks. -- To view, visit http://gerrit.cloudera.org:8080/12022 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibedb11dd3d50bcdb21d508f7d21691925491946e Gerrit-Change-Number: 12022 Gerrit-PatchSet: 2 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Sun, 02 Dec 2018 18:42:42 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5474: Adding a trivial subquery turns error into warning
Yongjun Zhang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12022 Change subject: IMPALA-5474: Adding a trivial subquery turns error into warning .. IMPALA-5474: Adding a trivial subquery turns error into warning After adding a subquery to a query that fails with ERROR, it fails with WARNING. The fix here makes it return ERROR. Testing: Added unit tests; Done real cluster testing with reported cases. Change-Id: Ibedb11dd3d50bcdb21d508f7d21691925491946e --- M shell/impala_client.py M shell/impala_shell.py M tests/shell/test_shell_commandline.py 3 files changed, 24 insertions(+), 12 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/22/12022/1 -- To view, visit http://gerrit.cloudera.org:8080/12022 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ibedb11dd3d50bcdb21d508f7d21691925491946e Gerrit-Change-Number: 12022 Gerrit-PatchSet: 1 Gerrit-Owner: Yongjun Zhang
[Impala-ASF-CR] IMPALA-6742: Profiles of running queries should include execution summary.
Yongjun Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11591 ) Change subject: IMPALA-6742: Profiles of running queries should include execution summary. .. Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/11591/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11591/1//COMMIT_MSG@9 PS1, Line 9: Currently execution summary is not included in the profiles of running : queries, and it's only reported when the query is finished. This jira makes : the execution summary to the profile reported wh > Use shorter lines. My recommendation is to wrap at 70 characters. Thanks, addressed in new rev. http://gerrit.cloudera.org:8080/#/c/11591/1/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/11591/1/be/src/service/impala-server.h@637 PS1, Line 637: void UpdateE > You don't want [[noreturn]] Thanks Joe, my misunderstanding of this annotation. Fixed in new rev. http://gerrit.cloudera.org:8080/#/c/11591/1/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/11591/1/be/src/service/impala-server.cc@1128 PS1, Line 1128:<< PrettyPrinter::Print(cpu_limit_ > I'm thinking this might not hold for some calls to GetRuntimeProfileStr() t Good catch Joe. Indeed. Fixed in new rev by adding a check. I tried to run test_observability both locally and at jenkins, the former had some failures, however, the latter is clean. Looking into why it failed locally. -- To view, visit http://gerrit.cloudera.org:8080/11591 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idc7f714c9427d4b26d4e78cf27ceca2b0b336699 Gerrit-Change-Number: 11591 Gerrit-PatchSet: 2 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Tue, 30 Oct 2018 21:51:06 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6742: Profiles of running queries should include execution summary.
Yongjun Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11591 ) Change subject: IMPALA-6742: Profiles of running queries should include execution summary. .. Patch Set 2: Hi Joe, thanks a lot for the very good review and sorry for late update. I just uploaded a new rev. Interestingly, some tests in test_observability failed locally but all is clean in jenkins. One question about your comment about line 637, I saw other places included [[noreturn]], what's the guideline for having it or not? thanks. -- To view, visit http://gerrit.cloudera.org:8080/11591 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idc7f714c9427d4b26d4e78cf27ceca2b0b336699 Gerrit-Change-Number: 11591 Gerrit-PatchSet: 2 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Tue, 30 Oct 2018 17:36:59 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6742: Profiles of running queries should include execution summary.
Hello Tim Armstrong, Joe McDonnell, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11591 to look at the new patch set (#2). Change subject: IMPALA-6742: Profiles of running queries should include execution summary. .. IMPALA-6742: Profiles of running queries should include execution summary. Currently execution summary is not included in the profiles of running queries, and it's only reported when the query is finished. This jira makes the execution summary to the profile reported when queries are still running. Testing: Done with real cluster. Change-Id: Idc7f714c9427d4b26d4e78cf27ceca2b0b336699 --- M be/src/service/impala-server.cc M be/src/service/impala-server.h 2 files changed, 19 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/91/11591/2 -- To view, visit http://gerrit.cloudera.org:8080/11591 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Idc7f714c9427d4b26d4e78cf27ceca2b0b336699 Gerrit-Change-Number: 11591 Gerrit-PatchSet: 2 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-6742: Profiles of running queries should include execution summary
Yongjun Zhang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11591 Change subject: IMPALA-6742: Profiles of running queries should include execution summary .. IMPALA-6742: Profiles of running queries should include execution summary Currently execution summary is not included in the profiles of running queries, and it's only reported when the query is finished. This jira makes the execution summary to the profile reported when queries are still running. Testing: This is a draft, tests are yet to be done. Change-Id: Idc7f714c9427d4b26d4e78cf27ceca2b0b336699 --- M be/src/service/impala-server.cc M be/src/service/impala-server.h 2 files changed, 15 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/91/11591/1 -- To view, visit http://gerrit.cloudera.org:8080/11591 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Idc7f714c9427d4b26d4e78cf27ceca2b0b336699 Gerrit-Change-Number: 11591 Gerrit-PatchSet: 1 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7166: ExecSummary should be a first class object.
Yongjun Zhang has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/11555 ) Change subject: IMPALA-7166: ExecSummary should be a first class object. .. IMPALA-7166: ExecSummary should be a first class object. Impala RuntimeProfile currently contains "ExecSummary" as a string. We should make it a first class thrift object, so that tools can extract these fields (Est rows etc..). Testing: Modified unit test. Change-Id: I4791237a5579f16c9efda8e57876d48980739e13 --- M be/src/service/impala-server.cc M be/src/util/runtime-profile-test.cc M be/src/util/runtime-profile.cc M be/src/util/runtime-profile.h M common/thrift/RuntimeProfile.thrift 5 files changed, 48 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/11555/3 -- To view, visit http://gerrit.cloudera.org:8080/11555 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4791237a5579f16c9efda8e57876d48980739e13 Gerrit-Change-Number: 11555 Gerrit-PatchSet: 3 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Reviewer: Yongjun Zhang
[Impala-ASF-CR] IMPALA-7166: ExecSummary should be a first class object.
Yongjun Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11555 ) Change subject: IMPALA-7166: ExecSummary should be a first class object. .. Patch Set 3: Thanks for the review Tim. I uploaded new rev to address other comments for completeness here. But I think let me fix IMPALA-6742 first. -- To view, visit http://gerrit.cloudera.org:8080/11555 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4791237a5579f16c9efda8e57876d48980739e13 Gerrit-Change-Number: 11555 Gerrit-PatchSet: 3 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Thu, 04 Oct 2018 23:19:04 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7166: ExecSummary should be a first class object.
Yongjun Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11555 ) Change subject: IMPALA-7166: ExecSummary should be a first class object. .. Patch Set 2: > (1 comment) > > We're having some non-trivial discussions here so let's publish > this code review so it's visible to everyone else. > (1 comment) > > We're having some non-trivial discussions here so let's publish > this code review so it's visible to everyone else. Thanks Tim. > (1 comment) > > We're having some non-trivial discussions here so let's publish > this code review so it's visible to everyone else. > (1 comment) > > We're having some non-trivial discussions here so let's publish > this code review so it's visible to everyone else. Thanks Tim, good info. I will work on IMPALA-6742. -- To view, visit http://gerrit.cloudera.org:8080/11555 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4791237a5579f16c9efda8e57876d48980739e13 Gerrit-Change-Number: 11555 Gerrit-PatchSet: 2 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Thu, 04 Oct 2018 01:11:08 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7166: ExecSummary should be a first class object.
Yongjun Zhang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11555 Change subject: IMPALA-7166: ExecSummary should be a first class object. .. IMPALA-7166: ExecSummary should be a first class object. Impala RuntimeProfile currently contains "ExecSummary" as a string. We should make it a first class thrift object, so that tools can extract these fields (Est rows etc..). Testing: Modified unit test. Change-Id: I4791237a5579f16c9efda8e57876d48980739e13 --- M be/src/service/impala-server.cc M be/src/util/runtime-profile-test.cc M be/src/util/runtime-profile.cc M be/src/util/runtime-profile.h M common/thrift/RuntimeProfile.thrift 5 files changed, 47 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/11555/2 -- To view, visit http://gerrit.cloudera.org:8080/11555 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I4791237a5579f16c9efda8e57876d48980739e13 Gerrit-Change-Number: 11555 Gerrit-PatchSet: 2 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Reviewer: Yongjun Zhang
[Impala-ASF-CR] IMPALA-6202. mod and % should be equivalent.
Yongjun Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11443 ) Change subject: IMPALA-6202. mod and % should be equivalent. .. Patch Set 10: Hi Tim, Vuk and Dan, thanks a lot for the help with this jira. Best. -- To view, visit http://gerrit.cloudera.org:8080/11443 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib0067da04083859ffbf662a29007154461bea2ba Gerrit-Change-Number: 11443 Gerrit-PatchSet: 10 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Thu, 27 Sep 2018 22:05:43 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6202. mod and % should be equivalent.
Yongjun Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11443 ) Change subject: IMPALA-6202. mod and % should be equivalent. .. Patch Set 9: > (7 comments) Thanks for the review Vuk, all the comments here are addressed in rev9. -- To view, visit http://gerrit.cloudera.org:8080/11443 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib0067da04083859ffbf662a29007154461bea2ba Gerrit-Change-Number: 11443 Gerrit-PatchSet: 9 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Thu, 27 Sep 2018 16:38:31 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6202. mod and % should be equivalent.
Yongjun Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11443 ) Change subject: IMPALA-6202. mod and % should be equivalent. .. Patch Set 9: > (2 comments) Thanks for the review Vuk. These two comments are addressed in rev9. I used "this case" instead of "these cases" since it's only one case there. Hope it works for you. -- To view, visit http://gerrit.cloudera.org:8080/11443 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib0067da04083859ffbf662a29007154461bea2ba Gerrit-Change-Number: 11443 Gerrit-PatchSet: 9 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Thu, 27 Sep 2018 16:37:28 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6202. mod and % should be equivalent.
Yongjun Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11443 ) Change subject: IMPALA-6202. mod and % should be equivalent. .. Patch Set 9: > (2 comments) > > Looks good to me, just had a couple of suggestions Thanks for the review Tim, all your comments are addressed in newer rev. -- To view, visit http://gerrit.cloudera.org:8080/11443 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib0067da04083859ffbf662a29007154461bea2ba Gerrit-Change-Number: 11443 Gerrit-PatchSet: 9 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Thu, 27 Sep 2018 16:35:36 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6202. mod and % should be equivalent.
Yongjun Zhang has uploaded a new patch set (#9). ( http://gerrit.cloudera.org:8080/11443 ) Change subject: IMPALA-6202. mod and % should be equivalent. .. IMPALA-6202. mod and % should be equivalent. Currently in DECIMAL V2 mode, typeof(9.9 % 3) is DECIMAL(2,1) and typeof(mod(9.9, 3)) is DECIMAL(4,1), while both are expected to be DECIMAL(2,1). This jira fixes V2 mode by replacing "mod" with "%" at parser stage thus they share the same code path afterwards. Testing: Added unit tests and done real cluster testing. Change-Id: Ib0067da04083859ffbf662a29007154461bea2ba --- M fe/src/main/cup/sql-parser.cup M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java M fe/src/main/java/org/apache/impala/catalog/Type.java M fe/src/main/java/org/apache/impala/catalog/View.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M fe/src/test/java/org/apache/impala/service/JdbcTest.java M testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering-disabled.test M testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering.test M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test 12 files changed, 126 insertions(+), 32 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/11443/9 -- To view, visit http://gerrit.cloudera.org:8080/11443 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib0067da04083859ffbf662a29007154461bea2ba Gerrit-Change-Number: 11443 Gerrit-PatchSet: 9 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Reviewer: Yongjun Zhang
[Impala-ASF-CR] IMPALA-6202. mod and % should be equivalent.
Yongjun Zhang has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/11443 ) Change subject: IMPALA-6202. mod and % should be equivalent. .. IMPALA-6202. mod and % should be equivalent. Currently in DECIMAL V2 mode, typeof(9.9 % 3) is DECIMAL(2,1) and typeof(mod(9.9, 3)) is DECIMAL(4,1), while both are expected to be DECIMAL(2,1). This jira fixes V2 mode by replacing "mod" with "%" at parser stage thus they share the same code path afterwards. Testing: Added unit tests and done real cluster testing. Change-Id: Ib0067da04083859ffbf662a29007154461bea2ba --- M fe/src/main/cup/sql-parser.cup M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java M fe/src/main/java/org/apache/impala/catalog/Type.java M fe/src/main/java/org/apache/impala/catalog/View.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M fe/src/test/java/org/apache/impala/service/JdbcTest.java M testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering-disabled.test M testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering.test M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test 12 files changed, 126 insertions(+), 32 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/11443/8 -- To view, visit http://gerrit.cloudera.org:8080/11443 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib0067da04083859ffbf662a29007154461bea2ba Gerrit-Change-Number: 11443 Gerrit-PatchSet: 8 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Reviewer: Yongjun Zhang
[Impala-ASF-CR] IMPALA-6202. mod and % should be equivalent.
Yongjun Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11443 ) Change subject: IMPALA-6202. mod and % should be equivalent. .. Patch Set 6: Thanks a lot for the review Vuk, just updated new rev to address your comments. -- To view, visit http://gerrit.cloudera.org:8080/11443 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib0067da04083859ffbf662a29007154461bea2ba Gerrit-Change-Number: 11443 Gerrit-PatchSet: 6 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Tue, 25 Sep 2018 21:54:23 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6202. mod and % should be equivalent.
Yongjun Zhang has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/11443 ) Change subject: IMPALA-6202. mod and % should be equivalent. .. IMPALA-6202. mod and % should be equivalent. Currently in DECIMAL V2 mode, typeof(9.9 % 3) is DECIMAL(2,1) and typeof(mod(9.9, 3)) is DECIMAL(4,1), while both are expected to be DECIMAL(2,1). This jira fixes V2 mode by replacing "mod" with "%" at parser stage thus they share the same code path afterwards. Testing: Added unit tests and done real cluster testing. Change-Id: Ib0067da04083859ffbf662a29007154461bea2ba --- M fe/src/main/cup/sql-parser.cup M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering-disabled.test M testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering.test M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test 8 files changed, 120 insertions(+), 31 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/11443/6 -- To view, visit http://gerrit.cloudera.org:8080/11443 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib0067da04083859ffbf662a29007154461bea2ba Gerrit-Change-Number: 11443 Gerrit-PatchSet: 6 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Reviewer: Yongjun Zhang
[Impala-ASF-CR] IMPALA-6202. mod and % should be equivalent.
Yongjun Zhang has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/11443 ) Change subject: IMPALA-6202. mod and % should be equivalent. .. IMPALA-6202. mod and % should be equivalent. Currently in DECIMAL V2 mode, typeof(9.9 % 3) is DECIMAL(2,1) and typeof(mod(9.9, 3)) is DECIMAL(4,1), while both are expected to be DECIMAL(2,1). This jira fixes V2 mode by replacing "mod" with "%" at parser stage thus they share the same code path afterwards. Testing: Added unit tests and done real cluster testing. Change-Id: Ib0067da04083859ffbf662a29007154461bea2ba --- M fe/src/main/cup/sql-parser.cup M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering-disabled.test M testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering.test M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test 8 files changed, 125 insertions(+), 30 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/11443/5 -- To view, visit http://gerrit.cloudera.org:8080/11443 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib0067da04083859ffbf662a29007154461bea2ba Gerrit-Change-Number: 11443 Gerrit-PatchSet: 5 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Reviewer: Yongjun Zhang
[Impala-ASF-CR] IMPALA-6202. mod and % should be equivalent.
Yongjun Zhang has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/11443 ) Change subject: IMPALA-6202. mod and % should be equivalent. .. IMPALA-6202. mod and % should be equivalent. Currently typeof(9.9 % 3) is DECIMAL(2,1) and typeof(mod(9.9, 3)) is DECIMAL(4,1), it's expected that both to be DECIMAL(2,1). This jira fixes it by making mod and % equivalent. The solution is to replace "mod" with "%". Testing: Added unit tests and done real cluster testing. Change-Id: Ib0067da04083859ffbf662a29007154461bea2ba --- M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java M testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering-disabled.test M testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering.test M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test 5 files changed, 91 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/11443/4 -- To view, visit http://gerrit.cloudera.org:8080/11443 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib0067da04083859ffbf662a29007154461bea2ba Gerrit-Change-Number: 11443 Gerrit-PatchSet: 4 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Reviewer: Yongjun Zhang
[Impala-ASF-CR] IMPALA-6202. mod and % are not equivalent.
Yongjun Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11443 ) Change subject: IMPALA-6202. mod and % are not equivalent. .. Patch Set 3: Uploaded a quick v3 to prove concept, it seems to be a much better solution. The reason I'm looking for new solution is, the casting in the previous solution can fail for certain cases. That's the hole I was referring to. I will craft coding and probably add more tests later. One side note, this will change the behavior of "mod" for certain cases, thus may appear incompatible. Thanks. -- To view, visit http://gerrit.cloudera.org:8080/11443 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib0067da04083859ffbf662a29007154461bea2ba Gerrit-Change-Number: 11443 Gerrit-PatchSet: 3 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Sat, 15 Sep 2018 00:14:11 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6202. mod and % are not equivalent.
Yongjun Zhang has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/11443 ) Change subject: IMPALA-6202. mod and % are not equivalent. .. IMPALA-6202. mod and % are not equivalent. Currently typeof(9.9 % 3) is DECIMAL(2,1) and typeof(mod(9.9, 3)) is DECIMAL(4,1), it's expected that both to be DECIMAL(2,1). This jira fixes it by makeing mod and % equivalent. The solution is to replace "mod" with "%". Testing: Added unit tests and done real cluster testing. Change-Id: Ib0067da04083859ffbf662a29007154461bea2ba --- M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test 3 files changed, 79 insertions(+), 15 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/11443/3 -- To view, visit http://gerrit.cloudera.org:8080/11443 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib0067da04083859ffbf662a29007154461bea2ba Gerrit-Change-Number: 11443 Gerrit-PatchSet: 3 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Reviewer: Yongjun Zhang
[Impala-ASF-CR] IMPALA-6202. mod and % are not equivalent.
Yongjun Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11443 ) Change subject: IMPALA-6202. mod and % are not equivalent. .. Patch Set 2: I'm thinking about a different approach now, say, we can probably simply convert a "mod" to "%" when doing the parsing, this way, all code used by "%" would be shared, and thus making them really equivalent. Thanks. -- To view, visit http://gerrit.cloudera.org:8080/11443 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib0067da04083859ffbf662a29007154461bea2ba Gerrit-Change-Number: 11443 Gerrit-PatchSet: 2 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Fri, 14 Sep 2018 23:37:23 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6202. mod and % are not equivalent.
Yongjun Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11443 ) Change subject: IMPALA-6202. mod and % are not equivalent. .. Patch Set 2: > (2 comments) Very good suggestion Vuk, I tried it out, and added some more tests and uncovered a hole in the solution. Thanks. -- To view, visit http://gerrit.cloudera.org:8080/11443 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib0067da04083859ffbf662a29007154461bea2ba Gerrit-Change-Number: 11443 Gerrit-PatchSet: 2 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Fri, 14 Sep 2018 23:20:12 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6202. mod and % are not equivalent.
Yongjun Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11443 ) Change subject: IMPALA-6202. mod and % are not equivalent. .. Patch Set 2: Hi Dan and Vuk, thanks a lot for your review comments. I just uploaded a new rev to address some test failures. I will address your comments in next rev. Thanks. -- To view, visit http://gerrit.cloudera.org:8080/11443 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib0067da04083859ffbf662a29007154461bea2ba Gerrit-Change-Number: 11443 Gerrit-PatchSet: 2 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Fri, 14 Sep 2018 20:11:22 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6202. mod and % are not equivalent.
Yongjun Zhang has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/11443 ) Change subject: IMPALA-6202. mod and % are not equivalent. .. IMPALA-6202. mod and % are not equivalent. Currently typeof(9.9 % 3) is DECIMAL(2,1) and typeof(mod(9.9, 3)) is DECIMAL(4,1), it's expected that both to be DECIMAL(2,1). This jira fixes it for Decimal V2 mode. V1 behavior is left as is since the V1 mode will be dropped soon per IMPALA-4924. Testing: Added unit tests to cover the above mentioned two cases for both Decimal V1 and V2 mode, while the two cases are fixed in V2 mode, they remain different in V1 mode. Change-Id: Ib0067da04083859ffbf662a29007154461bea2ba --- M fe/src/main/java/org/apache/impala/analysis/Expr.java M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test 3 files changed, 72 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/11443/2 -- To view, visit http://gerrit.cloudera.org:8080/11443 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib0067da04083859ffbf662a29007154461bea2ba Gerrit-Change-Number: 11443 Gerrit-PatchSet: 2 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-6202. mod and % are not equivalent.
Yongjun Zhang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11443 Change subject: IMPALA-6202. mod and % are not equivalent. .. IMPALA-6202. mod and % are not equivalent. Currently typeof(9.9 % 3) is DECIMAL(2,1) and typeof(mod(9.9, 3)) is DECIMAL(4,1), it's expected that both to be DECIMAL(2,1). This jira fixes it for Decimal V2 mode. V1 behavior is left as is since the V1 mode will be dropped soon per IMPALA-4924. Testing: Added unit tests to cover the above mentioned two cases for both Decimal V1 and V2 mode, while the two cases are fixed in V2 mode, they remain different in V1 mode. Change-Id: Ib0067da04083859ffbf662a29007154461bea2ba --- M fe/src/main/java/org/apache/impala/analysis/Expr.java M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test 3 files changed, 75 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/11443/1 -- To view, visit http://gerrit.cloudera.org:8080/11443 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ib0067da04083859ffbf662a29007154461bea2ba Gerrit-Change-Number: 11443 Gerrit-PatchSet: 1 Gerrit-Owner: Yongjun Zhang
[Impala-ASF-CR] IMPALA-6442: Misleading file offset reporting in error messages.
Yongjun Zhang has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/11379 ) Change subject: IMPALA-6442: Misleading file offset reporting in error messages. .. IMPALA-6442: Misleading file offset reporting in error messages. The error message described in IMPALA-6442 incorrectly reported the file offset where the Parquet footer starts, as if the offset is counted from the file end instead of from the file beginning. The fix changed the reported file offset to be counted from the beginning of the Parquet file. Testing: Create a small table that contains one row of data with a single column that's bigint and store it as Parquet. Manually changed the footer size field to be 1) smaller than the original footer size by 1, to trigger the error message fixed by this jira to be printed, to verify that the fix functions correctly; 2) bigger than the file size, thus to trigger another related error message to be printed. Change-Id: I35235e99ea9ceb0d31961dd3b8069f7194f5a2de --- M be/src/exec/hdfs-parquet-scanner.cc M testdata/data/README A testdata/data/corrupt_footer_len_decr.parquet A testdata/data/corrupt_footer_len_incr.parquet A testdata/workloads/functional-query/queries/QueryTest/parquet-corrupt-footer-len-decr.test A testdata/workloads/functional-query/queries/QueryTest/parquet-corrupt-footer-len-incr.test M tests/query_test/test_scanners.py 7 files changed, 86 insertions(+), 18 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/11379/6 -- To view, visit http://gerrit.cloudera.org:8080/11379 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I35235e99ea9ceb0d31961dd3b8069f7194f5a2de Gerrit-Change-Number: 11379 Gerrit-PatchSet: 6 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Yongjun Zhang
[Impala-ASF-CR] IMPALA-6442: Misleading file offset reporting in error messages.
Yongjun Zhang has posted comments on this change. (
http://gerrit.cloudera.org:8080/11379 )
Change subject: IMPALA-6442: Misleading file offset reporting in error messages.
..
Patch Set 2:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/11379/1/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
http://gerrit.cloudera.org:8080/#/c/11379/1/be/src/exec/hdfs-parquet-scanner.cc@1165
PS1, Line 1165: sca
> Now that there are two variables storing some kind of length it would be go
Done in new rev.
http://gerrit.cloudera.org:8080/#/c/11379/1/be/src/exec/hdfs-parquet-scanner.cc@1214
PS1, Line 1214: // file_len - 4-byte footer length field - 4-byte version
number field - metadata size
: int64_t metadata_start = file_len - sizeof(int32_t) -
> not your change but can you please fix the line wrapping while you're here?
Done in new rev.
http://gerrit.cloudera.org:8080/#/c/11379/1/be/src/exec/hdfs-parquet-scanner.cc@1228
PS1, Line 1228: const HdfsFileDesc* file_desc =
scan_node_->GetFileDesc(partition_id, filename());
> If both are NULL we won't catch it now. Are we sure that cannot happen? Sho
Thanks a lot for the review Lars.
The assumption is that stream_->file_desc() is not null, if it's null, the call
to get file length before this branch would have failed already. If we want, we
could add a DCHECK in the beginning of the function.
I added the DCHECK here for multiple reasons:
1. only the logic in the existing code here tries to find file_desc.
2. this branch is an "unlikely" branch. Code out side this branch has no need
to find file_desc.
3. the partition_id variable is only used in this branch.
Another version of my code did move these few lines to before this branch, but
I chose to move it here because otherwise it's going to be some additional work
for other code that doesn't need to reach this "unlikely" branch and doesn't
use the partition_id variable.
What do you think?
http://gerrit.cloudera.org:8080/#/c/11379/1/be/src/exec/hdfs-parquet-scanner.cc@1267
PS1, Line 1267: "at file of
> nit: wrap after file_length
done in new rev.
http://gerrit.cloudera.org:8080/#/c/11379/1/testdata/data/corrupt_footer_len_decr.parquet
File testdata/data/corrupt_footer_len_decr.parquet:
http://gerrit.cloudera.org:8080/#/c/11379/1/testdata/data/corrupt_footer_len_decr.parquet@1
PS1, Line 1: PAR1L Ò ,
%c^f&6& 6 (Ò Ò , ,Hschema %c%$ %c^f&6& 6 (Ò Ò , f (Oimpala
version 2.11.0-SNAPSHOT (build
26b2cc85d2084e9e464669b2a67a8015ede173e1) ½ PAR1
> Add a description for both test files to the README file in the same folder
done in new rev.
http://gerrit.cloudera.org:8080/#/c/11379/1/tests/query_test/test_scanners.py
File tests/query_test/test_scanners.py:
http://gerrit.cloudera.org:8080/#/c/11379/1/tests/query_test/test_scanners.py@401
PS1, Line 401: def corrupt_footer_len_common(self, vector, unique_database,
testname_postfix):
> The function should have a comment. Please find a more descriptive name for
Done in new rev.
http://gerrit.cloudera.org:8080/#/c/11379/1/tests/query_test/test_scanners.py@412
PS1, Line 412: create_table_and_copy_files(self.client,
> nit: single line
Done in new rev.
http://gerrit.cloudera.org:8080/#/c/11379/1/tests/query_test/test_scanners.py@420
PS1, Line 420:
> nit: capitalization
Done in new rev.
--
To view, visit http://gerrit.cloudera.org:8080/11379
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I35235e99ea9ceb0d31961dd3b8069f7194f5a2de
Gerrit-Change-Number: 11379
Gerrit-PatchSet: 2
Gerrit-Owner: Yongjun Zhang
Gerrit-Reviewer: Joe McDonnell
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Yongjun Zhang
Gerrit-Comment-Date: Thu, 06 Sep 2018 08:11:06 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6442: Misleading file offset reporting in error messages.
Yongjun Zhang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11379 Change subject: IMPALA-6442: Misleading file offset reporting in error messages. .. IMPALA-6442: Misleading file offset reporting in error messages. The error message described in IMPALA-6442 incorrectly reported the file offset where the Parquet footer starts, as if the offset is counted from the file end instead of from the file beginning. The fix changed the reported file offset to be counted from the beginning of the Parquet file. Testing: Create a small table that contains one row of data with a single column that's bigint and store it as Parquet. Manually changed the footer size field to be 1) smaller than the original footer size by 1, to trigger the error message fixed by this jira to be printed, to verify that the fix functions correctly; 2) bigger than the file size, thus to trigger another related error message to be printed. Change-Id: I35235e99ea9ceb0d31961dd3b8069f7194f5a2de --- M be/src/exec/hdfs-parquet-scanner.cc M testdata/data/README A testdata/data/corrupt_footer_len_decr.parquet A testdata/data/corrupt_footer_len_incr.parquet M tests/query_test/test_scanners.py 5 files changed, 65 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/11379/2 -- To view, visit http://gerrit.cloudera.org:8080/11379 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I35235e99ea9ceb0d31961dd3b8069f7194f5a2de Gerrit-Change-Number: 11379 Gerrit-PatchSet: 2 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-6442: Misleading file offset reporting in error messages.
Yongjun Zhang has abandoned this change. ( http://gerrit.cloudera.org:8080/11336 ) Change subject: IMPALA-6442: Misleading file offset reporting in error messages. .. Abandoned Abandon this one, and will work on a new one. Thanks. -- To view, visit http://gerrit.cloudera.org:8080/11336 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: Ie78ef9b0e11b8505ab7bc965aafdb2729b0027c9 Gerrit-Change-Number: 11336 Gerrit-PatchSet: 1 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Yongjun Zhang
[Impala-ASF-CR] IMPALA-6442: Misleading file offset reporting in error messages.
Yongjun Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11336 ) Change subject: IMPALA-6442: Misleading file offset reporting in error messages. .. Patch Set 1: Thanks a lot for the review Joe. After reading the code more carefully, I agree with your comment. -- To view, visit http://gerrit.cloudera.org:8080/11336 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie78ef9b0e11b8505ab7bc965aafdb2729b0027c9 Gerrit-Change-Number: 11336 Gerrit-PatchSet: 1 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Tue, 28 Aug 2018 03:23:50 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6442: Misleading file offset reporting in error messages.
Yongjun Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11336 ) Change subject: IMPALA-6442: Misleading file offset reporting in error messages. .. Patch Set 1: > Hello, > > This is the Apache Impala project. Not everyone who contributes to > Apache Impala work at Cloudera, which mean not everyone who > contributes can see that link. Please use > https://jenkins.impala.io/job/gerrit-verify-dryrun-external/ > . Thanks Michael, I'm looking into whether I can create a unit test. Will run test there next time. -- To view, visit http://gerrit.cloudera.org:8080/11336 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie78ef9b0e11b8505ab7bc965aafdb2729b0027c9 Gerrit-Change-Number: 11336 Gerrit-PatchSet: 1 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Mon, 27 Aug 2018 20:12:20 + Gerrit-HasComments: No
