You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2018/06/22 17:17:17 UTC
[Impala-ASF-CR] IMPALA-7140 (part 6): fetch column stats for LocalTable
Hello Tianyi Wang, Vuk Ercegovac,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/10797
to review the following change.
Change subject: IMPALA-7140 (part 6): fetch column stats for LocalTable
......................................................................
IMPALA-7140 (part 6): fetch column stats for LocalTable
This adds fetching of column statistics for LocalTable. Currently, all
column stats are fetched when the table is loaded, even for simple
statements like 'DESCRIBE' where they aren't necessary. This is because
I couldn't find a convenient spot during analysis at which time the set
of necessary columns are known. I left a TODO for this potential
improvement.
With this change I can see that 'SHOW COLUMN STATS' shows the expected
results for functional.alltypes. A new simple unit test verifies this.
Planner tests still don't pass due to some NullPointerExceptions related
to loading functions from the builtins DB -- most of the tests seem to
rely on simple built-ins like COUNT and CAST.
Change-Id: Ib6403c2bedf4ee29c5e6f90e947382cb44f46e0c
---
M fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
M fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java
7 files changed, 114 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/10797/1
--
To view, visit http://gerrit.cloudera.org:8080/10797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib6403c2bedf4ee29c5e6f90e947382cb44f46e0c
Gerrit-Change-Number: 10797
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-7140 (part 6): fetch column stats for LocalTable
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10797 )
Change subject: IMPALA-7140 (part 6): fetch column stats for LocalTable
......................................................................
IMPALA-7140 (part 6): fetch column stats for LocalTable
This adds fetching of column statistics for LocalTable. Currently, all
column stats are fetched when the table is loaded, even for simple
statements like 'DESCRIBE' where they aren't necessary. This is because
I couldn't find a convenient spot during analysis at which time the set
of necessary columns are known. I left a TODO for this potential
improvement.
With this change I can see that 'SHOW COLUMN STATS' shows the expected
results for functional.alltypes. A new simple unit test verifies this.
Planner tests still don't pass due to some NullPointerExceptions related
to loading functions from the builtins DB -- most of the tests seem to
rely on simple built-ins like COUNT and CAST.
Change-Id: Ib6403c2bedf4ee29c5e6f90e947382cb44f46e0c
Reviewed-on: http://gerrit.cloudera.org:8080/10797
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
M fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java
7 files changed, 114 insertions(+), 20 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Todd Lipcon: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/10797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib6403c2bedf4ee29c5e6f90e947382cb44f46e0c
Gerrit-Change-Number: 10797
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-7140 (part 6): fetch column stats for LocalTable
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Tianyi Wang, Vuk Ercegovac,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10797
to look at the new patch set (#2).
Change subject: IMPALA-7140 (part 6): fetch column stats for LocalTable
......................................................................
IMPALA-7140 (part 6): fetch column stats for LocalTable
This adds fetching of column statistics for LocalTable. Currently, all
column stats are fetched when the table is loaded, even for simple
statements like 'DESCRIBE' where they aren't necessary. This is because
I couldn't find a convenient spot during analysis at which time the set
of necessary columns are known. I left a TODO for this potential
improvement.
With this change I can see that 'SHOW COLUMN STATS' shows the expected
results for functional.alltypes. A new simple unit test verifies this.
Planner tests still don't pass due to some NullPointerExceptions related
to loading functions from the builtins DB -- most of the tests seem to
rely on simple built-ins like COUNT and CAST.
Change-Id: Ib6403c2bedf4ee29c5e6f90e947382cb44f46e0c
---
M fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
M fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java
7 files changed, 114 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/10797/2
--
To view, visit http://gerrit.cloudera.org:8080/10797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib6403c2bedf4ee29c5e6f90e947382cb44f46e0c
Gerrit-Change-Number: 10797
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-7140 (part 6): fetch column stats for LocalTable
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10797 )
Change subject: IMPALA-7140 (part 6): fetch column stats for LocalTable
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2793/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/10797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6403c2bedf4ee29c5e6f90e947382cb44f46e0c
Gerrit-Change-Number: 10797
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Jul 2018 18:55:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7140 (part 6): fetch column stats for LocalTable
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10797 )
Change subject: IMPALA-7140 (part 6): fetch column stats for LocalTable
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/10797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6403c2bedf4ee29c5e6f90e947382cb44f46e0c
Gerrit-Change-Number: 10797
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Jul 2018 22:09:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7140 (part 6): fetch column stats for LocalTable
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10797 )
Change subject: IMPALA-7140 (part 6): fetch column stats for LocalTable
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10797/1/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java:
http://gerrit.cloudera.org:8080/#/c/10797/1/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java@395
PS1, Line 395: // TODO(todd): this calculation ends up setting the num_nulls stat
> hmm, that seems odd. I'll look into it more and file a jira. thx for pointi
Looked into this one. Partitioning keys are handled differently for stats: compute stats queries skip them and as you've pointed out, they're maintained differently. Given the number of rows in a partition, we should use this for the number of nulls per partition and can sum up over all partitions with a null column. I was in the process of filing a jira, but decided to wait on it to first confirm the behavior.
--
To view, visit http://gerrit.cloudera.org:8080/10797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6403c2bedf4ee29c5e6f90e947382cb44f46e0c
Gerrit-Change-Number: 10797
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 06 Jul 2018 18:33:23 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7140 (part 6): fetch column stats for LocalTable
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10797 )
Change subject: IMPALA-7140 (part 6): fetch column stats for LocalTable
......................................................................
Patch Set 1: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10797/1/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java:
http://gerrit.cloudera.org:8080/#/c/10797/1/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java@395
PS1, Line 395: // TODO(todd): this calculation ends up setting the num_nulls stat
hmm, that seems odd. I'll look into it more and file a jira. thx for pointing it out.
http://gerrit.cloudera.org:8080/#/c/10797/1/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java:
http://gerrit.cloudera.org:8080/#/c/10797/1/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java@57
PS1, Line 57: private static final Logger LOG = Logger.getLogger(Table.class);
LocalTable.class
--
To view, visit http://gerrit.cloudera.org:8080/10797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6403c2bedf4ee29c5e6f90e947382cb44f46e0c
Gerrit-Change-Number: 10797
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Jun 2018 18:03:06 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7140 (part 6): fetch column stats for LocalTable
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10797 )
Change subject: IMPALA-7140 (part 6): fetch column stats for LocalTable
......................................................................
Patch Set 3: Code-Review+2
Forwarding +2
--
To view, visit http://gerrit.cloudera.org:8080/10797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6403c2bedf4ee29c5e6f90e947382cb44f46e0c
Gerrit-Change-Number: 10797
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Jul 2018 02:35:22 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7140 (part 6): fetch column stats for LocalTable
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10797 )
Change subject: IMPALA-7140 (part 6): fetch column stats for LocalTable
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10797/1/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java:
http://gerrit.cloudera.org:8080/#/c/10797/1/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java@57
PS1, Line 57: private static final Logger LOG = Logger.getLogger(Table.class);
> LocalTable.class
Done
--
To view, visit http://gerrit.cloudera.org:8080/10797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6403c2bedf4ee29c5e6f90e947382cb44f46e0c
Gerrit-Change-Number: 10797
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 06 Jul 2018 17:22:35 +0000
Gerrit-HasComments: Yes