You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Gabor Kaszab (Code Review)" <ge...@cloudera.org> on 2018/01/25 20:11:57 UTC
[Impala-ASF-CR] IMPALA-6311: Skip row groups with predicates on NULL columns
Gabor Kaszab has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9140
Change subject: IMPALA-6311: Skip row groups with predicates on NULL columns
......................................................................
IMPALA-6311: Skip row groups with predicates on NULL columns
Based on the existing Parquet column chunk level statistics null_count,
Impala's Parquet scanner is enhanced to skip an entire row group if the
null_count statistics indicate that all the values under the predicated
column are NULL as we wouldn't get any result rows from that row group
anyway.
Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/parquet-column-stats.cc
M be/src/exec/parquet-column-stats.h
M testdata/workloads/functional-query/queries/QueryTest/parquet-stats.test
4 files changed, 56 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/40/9140/1
--
To view, visit http://gerrit.cloudera.org:8080/9140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
Gerrit-Change-Number: 9140
Gerrit-PatchSet: 1
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
[Impala-ASF-CR] IMPALA-6113: Skip row groups with predicates on NULL columns
Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
anujphadke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9140 )
Change subject: IMPALA-6113: Skip row groups with predicates on NULL columns
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9140/1/testdata/workloads/functional-query/queries/QueryTest/parquet-stats.test
File testdata/workloads/functional-query/queries/QueryTest/parquet-stats.test:
http://gerrit.cloudera.org:8080/#/c/9140/1/testdata/workloads/functional-query/queries/QueryTest/parquet-stats.test@463
PS1, Line 463: create table table_for_null_count_test (i int, j int) stored as parquet;
> Hey Anuj, thanks for taking a look!
This is to avoid conflicts when running these tests in parallel. Looks like you have already moved it there. Thanks!
I actually tried running these tests from your old patch and it used the unique_db to create the tables without using the $DATABASE. Sorry about the noise.
--
To view, visit http://gerrit.cloudera.org:8080/9140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
Gerrit-Change-Number: 9140
Gerrit-PatchSet: 1
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jan 2018 18:54:19 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6113: Skip row groups with predicates on NULL columns
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9140 )
Change subject: IMPALA-6113: Skip row groups with predicates on NULL columns
......................................................................
Patch Set 7:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1862/
--
To view, visit http://gerrit.cloudera.org:8080/9140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
Gerrit-Change-Number: 9140
Gerrit-PatchSet: 7
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Feb 2018 16:19:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6113: Skip row groups with predicates on NULL columns
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9140 )
Change subject: IMPALA-6113: Skip row groups with predicates on NULL columns
......................................................................
Patch Set 7: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1862/
--
To view, visit http://gerrit.cloudera.org:8080/9140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
Gerrit-Change-Number: 9140
Gerrit-PatchSet: 7
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Feb 2018 19:59:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6113: Skip row groups with predicates on NULL columns
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9140 )
Change subject: IMPALA-6113: Skip row groups with predicates on NULL columns
......................................................................
Patch Set 6: Code-Review+1
LGTM!
--
To view, visit http://gerrit.cloudera.org:8080/9140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
Gerrit-Change-Number: 9140
Gerrit-PatchSet: 6
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Feb 2018 13:28:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6113: Skip row groups with predicates on NULL columns
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Zoltan Borok-Nagy, anujphadke, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9140
to look at the new patch set (#3).
Change subject: IMPALA-6113: Skip row groups with predicates on NULL columns
......................................................................
IMPALA-6113: Skip row groups with predicates on NULL columns
Based on the existing Parquet column chunk level statistics null_count,
Impala's Parquet scanner is enhanced to skip an entire row group if the
null_count statistics indicate that all the values under the predicated
column are NULL as we wouldn't get any result rows from that row group
anyway.
Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/parquet-column-stats.cc
M be/src/exec/parquet-column-stats.h
M testdata/workloads/functional-query/queries/QueryTest/parquet-stats.test
M tests/query_test/test_parquet_stats.py
5 files changed, 56 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/40/9140/3
--
To view, visit http://gerrit.cloudera.org:8080/9140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
Gerrit-Change-Number: 9140
Gerrit-PatchSet: 3
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-6311: Skip row groups with predicates on NULL columns
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/9140 )
Change subject: IMPALA-6311: Skip row groups with predicates on NULL columns
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9140/1/testdata/workloads/functional-query/queries/QueryTest/parquet-stats.test
File testdata/workloads/functional-query/queries/QueryTest/parquet-stats.test:
http://gerrit.cloudera.org:8080/#/c/9140/1/testdata/workloads/functional-query/queries/QueryTest/parquet-stats.test@463
PS1, Line 463: insert into table_for_null_count_test values (1, NULL), (2, NULL), (3, NULL);
> Can we modify these create table statements and move them
Hey Anuj, thanks for taking a look!
Sure, I can move the create table statements to the .py file.
For my benefit, could you please let me know what we gain if we moved these there? Can't I use the $DATABASE variable in the .test file to use the unique_database?
--
To view, visit http://gerrit.cloudera.org:8080/9140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
Gerrit-Change-Number: 9140
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Fri, 26 Jan 2018 11:11:56 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6113: Skip row groups with predicates on NULL columns
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9140 )
Change subject: IMPALA-6113: Skip row groups with predicates on NULL columns
......................................................................
Patch Set 8: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/9140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
Gerrit-Change-Number: 9140
Gerrit-PatchSet: 8
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Sat, 03 Feb 2018 03:24:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6113: Skip row groups with predicates on NULL columns
Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
anujphadke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9140 )
Change subject: IMPALA-6113: Skip row groups with predicates on NULL columns
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9140/3/tests/query_test/test_parquet_stats.py
File tests/query_test/test_parquet_stats.py:
http://gerrit.cloudera.org:8080/#/c/9140/3/tests/query_test/test_parquet_stats.py@49
PS3, Line 49: self.run_test_case('QueryTest/parquet-stats', vector, use_db=unique_database)
> Sorry for the churn. Why do you need to use $DATABASE though anyway? AFAIK
Sorry about the confusion. I thought that adding the create table statement in .test file won't create the table under a unique_db.
I manually tried it later with your first patch and checked the code. It creates a unique db without the $DATABASE suffix. Verified this through the impala-shell while running the test.
--
To view, visit http://gerrit.cloudera.org:8080/9140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
Gerrit-Change-Number: 9140
Gerrit-PatchSet: 1
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Feb 2018 22:40:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6113: Skip row groups with predicates on NULL columns
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/9140 )
Change subject: IMPALA-6113: Skip row groups with predicates on NULL columns
......................................................................
Patch Set 3:
(4 comments)
Thanks for taking a look, Zoli!
http://gerrit.cloudera.org:8080/#/c/9140/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/9140/2//COMMIT_MSG@7
PS2, Line 7: IMPALA-6113
> it's IMPALA-6113
Nice, thx :)
http://gerrit.cloudera.org:8080/#/c/9140/2/be/src/exec/parquet-column-stats.h
File be/src/exec/parquet-column-stats.h:
http://gerrit.cloudera.org:8080/#/c/9140/2/be/src/exec/parquet-column-stats.h@78
PS2, Line 78: returned
> Currently it returns a pointer to a member of ColumnChunk. This means that
In this case the ColumnChunk lives way longer than the code where I use the stats (ColumnChunk is part of the file_metadata_ in HDFSParquetScanner).
I prefer writing a comment to note the future users of this function about this restriction.
http://gerrit.cloudera.org:8080/#/c/9140/2/be/src/exec/parquet-column-stats.cc
File be/src/exec/parquet-column-stats.cc:
http://gerrit.cloudera.org:8080/#/c/9140/2/be/src/exec/parquet-column-stats.cc@132
PS2, Line 132: const int64_t* ColumnStatsBase::ReadNullCountStat(const parquet::ColumnChunk& col_chunk) {
: if (!(col_chunk.__isset.meta_data && col_c
> nit: can fit into one line
Done
http://gerrit.cloudera.org:8080/#/c/9140/2/be/src/exec/parquet-column-stats.cc@139
PS2, Line 139: }
:
: Sta
> nit: can be a one-liner
Done
--
To view, visit http://gerrit.cloudera.org:8080/9140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
Gerrit-Change-Number: 9140
Gerrit-PatchSet: 3
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Fri, 26 Jan 2018 14:53:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6311: Skip row groups with predicates on NULL columns
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Zoltan Borok-Nagy, anujphadke, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9140
to look at the new patch set (#2).
Change subject: IMPALA-6311: Skip row groups with predicates on NULL columns
......................................................................
IMPALA-6311: Skip row groups with predicates on NULL columns
Based on the existing Parquet column chunk level statistics null_count,
Impala's Parquet scanner is enhanced to skip an entire row group if the
null_count statistics indicate that all the values under the predicated
column are NULL as we wouldn't get any result rows from that row group
anyway.
Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/parquet-column-stats.cc
M be/src/exec/parquet-column-stats.h
M testdata/workloads/functional-query/queries/QueryTest/parquet-stats.test
M tests/query_test/test_parquet_stats.py
5 files changed, 58 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/40/9140/2
--
To view, visit http://gerrit.cloudera.org:8080/9140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
Gerrit-Change-Number: 9140
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-6113: Skip row groups with predicates on NULL columns
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9140 )
Change subject: IMPALA-6113: Skip row groups with predicates on NULL columns
......................................................................
IMPALA-6113: Skip row groups with predicates on NULL columns
Based on the existing Parquet column chunk level statistics null_count,
Impala's Parquet scanner is enhanced to skip an entire row group if the
null_count statistics indicate that all the values under the predicated
column are NULL as we wouldn't get any result rows from that row group
anyway.
Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
Reviewed-on: http://gerrit.cloudera.org:8080/9140
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/parquet-column-stats.cc
M be/src/exec/parquet-column-stats.h
M testdata/workloads/functional-query/queries/QueryTest/parquet-stats.test
4 files changed, 59 insertions(+), 0 deletions(-)
Approvals:
Tim Armstrong: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/9140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
Gerrit-Change-Number: 9140
Gerrit-PatchSet: 9
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-6113: Skip row groups with predicates on NULL columns
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9140 )
Change subject: IMPALA-6113: Skip row groups with predicates on NULL columns
......................................................................
Patch Set 8: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
Gerrit-Change-Number: 9140
Gerrit-PatchSet: 8
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Feb 2018 23:31:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6113: Skip row groups with predicates on NULL columns
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/9140 )
Change subject: IMPALA-6113: Skip row groups with predicates on NULL columns
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9140/3/be/src/exec/parquet-column-stats.cc
File be/src/exec/parquet-column-stats.cc:
http://gerrit.cloudera.org:8080/#/c/9140/3/be/src/exec/parquet-column-stats.cc@132
PS3, Line 132: bool ColumnStatsBase::ReadNullCountStat(const parquet::ColumnChunk& col_chunk,
> Why do we need to return a "const int64_t*" to the caller?
Thanks Tim for the explanation. Makes sense.
Done.
http://gerrit.cloudera.org:8080/#/c/9140/3/tests/query_test/test_parquet_stats.py
File tests/query_test/test_parquet_stats.py:
http://gerrit.cloudera.org:8080/#/c/9140/3/tests/query_test/test_parquet_stats.py@49
PS3, Line 49: self.run_test_case('QueryTest/parquet-stats', vector, use_db=unique_database)
> Sorry about the confusion. I thought that adding the create table statement
Thanks Tim, Anuj!
Dropped the $DATABASE prefix.
--
To view, visit http://gerrit.cloudera.org:8080/9140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
Gerrit-Change-Number: 9140
Gerrit-PatchSet: 6
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Feb 2018 09:20:17 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6113: Skip row groups with predicates on NULL columns
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Zoltan Borok-Nagy, anujphadke, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9140
to look at the new patch set (#4).
Change subject: IMPALA-6113: Skip row groups with predicates on NULL columns
......................................................................
IMPALA-6113: Skip row groups with predicates on NULL columns
Based on the existing Parquet column chunk level statistics null_count,
Impala's Parquet scanner is enhanced to skip an entire row group if the
null_count statistics indicate that all the values under the predicated
column are NULL as we wouldn't get any result rows from that row group
anyway.
Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/parquet-column-stats.cc
M be/src/exec/parquet-column-stats.h
M testdata/workloads/functional-query/queries/QueryTest/parquet-stats.test
4 files changed, 61 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/40/9140/4
--
To view, visit http://gerrit.cloudera.org:8080/9140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
Gerrit-Change-Number: 9140
Gerrit-PatchSet: 4
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-6113: Skip row groups with predicates on NULL columns
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9140 )
Change subject: IMPALA-6113: Skip row groups with predicates on NULL columns
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
Gerrit-Change-Number: 9140
Gerrit-PatchSet: 6
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Feb 2018 16:18:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6311: Skip row groups with predicates on NULL columns
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9140 )
Change subject: IMPALA-6311: Skip row groups with predicates on NULL columns
......................................................................
Patch Set 2:
(4 comments)
LGTM, had minor comments
http://gerrit.cloudera.org:8080/#/c/9140/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/9140/2//COMMIT_MSG@7
PS2, Line 7: IMPALA-6311
it's IMPALA-6113
http://gerrit.cloudera.org:8080/#/c/9140/2/be/src/exec/parquet-column-stats.h
File be/src/exec/parquet-column-stats.h:
http://gerrit.cloudera.org:8080/#/c/9140/2/be/src/exec/parquet-column-stats.h@78
PS2, Line 78: int64_t*
Currently it returns a pointer to a member of ColumnChunk. This means that the returned pointer can easily become dangling if the corresponding ColumnChunk object dies.
Maybe you could add a comment about it.
Or, it could return an int64_t, and -1 would indicate that there are no null_count statistics.
http://gerrit.cloudera.org:8080/#/c/9140/2/be/src/exec/parquet-column-stats.cc
File be/src/exec/parquet-column-stats.cc:
http://gerrit.cloudera.org:8080/#/c/9140/2/be/src/exec/parquet-column-stats.cc@132
PS2, Line 132: const int64_t* ColumnStatsBase::ReadNullCountStat(
: const parquet::ColumnChunk& col_chunk) {
nit: can fit into one line
http://gerrit.cloudera.org:8080/#/c/9140/2/be/src/exec/parquet-column-stats.cc@139
PS2, Line 139: if (stats.__isset.null_count) {
: return &stats.null_count;
: }
nit: can be a one-liner
--
To view, visit http://gerrit.cloudera.org:8080/9140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
Gerrit-Change-Number: 9140
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Fri, 26 Jan 2018 11:19:53 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6113: Skip row groups with predicates on NULL columns
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9140 )
Change subject: IMPALA-6113: Skip row groups with predicates on NULL columns
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
Gerrit-Change-Number: 9140
Gerrit-PatchSet: 7
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Feb 2018 16:18:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6113: Skip row groups with predicates on NULL columns
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/9140 )
Change subject: IMPALA-6113: Skip row groups with predicates on NULL columns
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9140/3/be/src/exec/parquet-column-stats.cc
File be/src/exec/parquet-column-stats.cc:
http://gerrit.cloudera.org:8080/#/c/9140/3/be/src/exec/parquet-column-stats.cc@132
PS3, Line 132: bool ColumnStatsBase::ReadNullCountStat(const parquet::ColumnChunk& col_chunk,
> Can we use the same pattern as ReadFromThrift() - i.e. return a bool to ind
Sure, done.
However, in my opinion returning a bool from this function is overkill as the success of the logic can be judged based on the null_count pointer being null or not.
For ReadFromThrift it was a bit different as the slot variable was a pre-allocated memory buffer and outside of the function it wasn't possible to decide whether we can use the content of this buffer or not, hence a bool return value was required.
Where ReadNullCountStat is invoked, if we check the return value we have a false impression that we can use the pointer (however, if some future modification ruins the content of the function it can happen that we still have a null pointer meanwhile returning true), so we might want to check both the return value and the null_count pointer but then the return value has no point.
Could you share your thoughts?
http://gerrit.cloudera.org:8080/#/c/9140/3/tests/query_test/test_parquet_stats.py
File tests/query_test/test_parquet_stats.py:
http://gerrit.cloudera.org:8080/#/c/9140/3/tests/query_test/test_parquet_stats.py@49
PS3, Line 49: self.run_test_case('QueryTest/parquet-stats', vector, use_db=unique_database)
> Can you move these into parquet-stats.test - I don't think there's a reason
Sure. It was suggested by Anuj to move the create statements here. I'll move them back using $DATABASE.
--
To view, visit http://gerrit.cloudera.org:8080/9140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
Gerrit-Change-Number: 9140
Gerrit-PatchSet: 4
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Feb 2018 11:51:02 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6113: Skip row groups with predicates on NULL columns
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Zoltan Borok-Nagy, anujphadke, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9140
to look at the new patch set (#6).
Change subject: IMPALA-6113: Skip row groups with predicates on NULL columns
......................................................................
IMPALA-6113: Skip row groups with predicates on NULL columns
Based on the existing Parquet column chunk level statistics null_count,
Impala's Parquet scanner is enhanced to skip an entire row group if the
null_count statistics indicate that all the values under the predicated
column are NULL as we wouldn't get any result rows from that row group
anyway.
Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/parquet-column-stats.cc
M be/src/exec/parquet-column-stats.h
M testdata/workloads/functional-query/queries/QueryTest/parquet-stats.test
4 files changed, 59 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/40/9140/6
--
To view, visit http://gerrit.cloudera.org:8080/9140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
Gerrit-Change-Number: 9140
Gerrit-PatchSet: 6
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-6113: Skip row groups with predicates on NULL columns
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9140 )
Change subject: IMPALA-6113: Skip row groups with predicates on NULL columns
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9140/3/be/src/exec/parquet-column-stats.cc
File be/src/exec/parquet-column-stats.cc:
http://gerrit.cloudera.org:8080/#/c/9140/3/be/src/exec/parquet-column-stats.cc@132
PS3, Line 132: const int64_t* ColumnStatsBase::ReadNullCountStat(
Can we use the same pattern as ReadFromThrift() - i.e. return a bool to indicate whether the stats were read, and an output parameter for the value?
http://gerrit.cloudera.org:8080/#/c/9140/3/tests/query_test/test_parquet_stats.py
File tests/query_test/test_parquet_stats.py:
http://gerrit.cloudera.org:8080/#/c/9140/3/tests/query_test/test_parquet_stats.py@49
PS3, Line 49: self.client.execute('create table %s.table_for_null_count_test (i int, j int) '
Can you move these into parquet-stats.test - I don't think there's a reason to have these queries in python.
--
To view, visit http://gerrit.cloudera.org:8080/9140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
Gerrit-Change-Number: 9140
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jan 2018 19:17:30 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6311: Skip row groups with predicates on NULL columns
Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
anujphadke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9140 )
Change subject: IMPALA-6311: Skip row groups with predicates on NULL columns
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9140/1/testdata/workloads/functional-query/queries/QueryTest/parquet-stats.test
File testdata/workloads/functional-query/queries/QueryTest/parquet-stats.test:
http://gerrit.cloudera.org:8080/#/c/9140/1/testdata/workloads/functional-query/queries/QueryTest/parquet-stats.test@463
PS1, Line 463: create table table_for_null_count_test (i int, j int) stored as parquet;
Can we modify these create table statements and move them
test_parquet_stats.py and create them using a unique database?
http://gerrit.cloudera.org:8080/#/c/9140/1/testdata/workloads/functional-query/queries/QueryTest/parquet-stats.test@485
PS1, Line 485: create table table_for_null_count_test2 (i int, j int) stored as parquet;
same here.
--
To view, visit http://gerrit.cloudera.org:8080/9140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
Gerrit-Change-Number: 9140
Gerrit-PatchSet: 1
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Thu, 25 Jan 2018 21:48:45 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6113: Skip row groups with predicates on NULL columns
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9140 )
Change subject: IMPALA-6113: Skip row groups with predicates on NULL columns
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9140/3/be/src/exec/parquet-column-stats.cc
File be/src/exec/parquet-column-stats.cc:
http://gerrit.cloudera.org:8080/#/c/9140/3/be/src/exec/parquet-column-stats.cc@132
PS3, Line 132: const int64_t* ColumnStatsBase::ReadNullCountStat(const parquet::ColumnChunk& col_chunk) {
> Sure, done.
Why do we need to return a "const int64_t*" to the caller?
Can't we just have the output argument be an int64_t* so that the this function writes the value to a caller-provided location?
The caller just needs to get an int64_t value + an additional bit of information indicating whether it's valid or not. The NULL-ness of int64_t* pointer did encode that extra bit earlier, but I think it's best to avoid the memory lifetime concerns around returning pointers except when it's necessary.
http://gerrit.cloudera.org:8080/#/c/9140/3/tests/query_test/test_parquet_stats.py
File tests/query_test/test_parquet_stats.py:
http://gerrit.cloudera.org:8080/#/c/9140/3/tests/query_test/test_parquet_stats.py@49
PS3, Line 49: self.client.execute('create table %s.table_for_null_count_test (i int, j int) '
> Sure. It was suggested by Anuj to move the create statements here. I'll mov
Sorry for the churn. Why do you need to use $DATABASE though anyway? AFAIK run_test_case automatically switches to the use_db database so you should be fine dropping the $DATABASE. suffix.
--
To view, visit http://gerrit.cloudera.org:8080/9140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
Gerrit-Change-Number: 9140
Gerrit-PatchSet: 3
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Feb 2018 21:46:40 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6113: Skip row groups with predicates on NULL columns
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9140 )
Change subject: IMPALA-6113: Skip row groups with predicates on NULL columns
......................................................................
Patch Set 8:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1868/
--
To view, visit http://gerrit.cloudera.org:8080/9140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I141317af0e0df30da8f220b29b0bfba364f40ddf
Gerrit-Change-Number: 9140
Gerrit-PatchSet: 8
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Feb 2018 23:31:57 +0000
Gerrit-HasComments: No