You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Michael Smith (Code Review)" <ge...@cloudera.org> on 2024/06/06 18:09:23 UTC

[Impala-ASF-CR] IMPALA-12800: Add isTrueWithNullSlots cache

Michael Smith has uploaded this change for review. ( http://gerrit.cloudera.org:8080/21484


Change subject: IMPALA-12800: Add isTrueWithNullSlots cache
......................................................................

IMPALA-12800: Add isTrueWithNullSlots cache

isTrueWithNullSlots can be expensive when it has to query the backend.
Many of the expressions will look similar, especially in large
auto-generated expressions. Adds a cache based on the nullified
expression to avoid querying the backend for expressions with identical
structure.

With DEBUG logging enabled for the Analyzer, computes and logs stats
about the isTrueWithNullSlotsCache.

Adds 'use_null_slots_cache' query option to disable caching. Documents
the new option.

Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
---
M be/src/service/query-options.h
M common/thrift/ImpalaService.thrift
M common/thrift/Query.thrift
M docs/impala.ditamap
A docs/topics/impala_use_null_slots_cache.xml
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
7 files changed, 125 insertions(+), 3 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/21484/1
-- 
To view, visit http://gerrit.cloudera.org:8080/21484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
Gerrit-Change-Number: 21484
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Smith <mi...@cloudera.com>

[Impala-ASF-CR] IMPALA-12800: Add cache for isTrueWithNullSlots() evaluation

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.

Hello Quanlong Huang, Daniel Becker, Riza Suminto, Joe McDonnell, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/21484

to look at the new patch set (#12).

Change subject: IMPALA-12800: Add cache for isTrueWithNullSlots() evaluation
......................................................................

IMPALA-12800: Add cache for isTrueWithNullSlots() evaluation

isTrueWithNullSlots() can be expensive when it has to query the backend.
Many of the expressions will look similar, especially in large
auto-generated expressions. Adds a cache based on the nullified
expression to avoid querying the backend for expressions with identical
structure.

With DEBUG logging enabled for the Analyzer, computes and logs stats
about the null slots cache.

Adds 'use_null_slots_cache' query option to disable caching. Documents
the new option.

Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaService.thrift
M common/thrift/Query.thrift
M docs/impala.ditamap
A docs/topics/impala_use_null_slots_cache.xml
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
8 files changed, 129 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/21484/12
-- 
To view, visit http://gerrit.cloudera.org:8080/21484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
Gerrit-Change-Number: 21484
Gerrit-PatchSet: 12
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>

[Impala-ASF-CR] IMPALA-12800: Add cache for isTrueWithNullSlots() evaluation

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.

Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21484 )

Change subject: IMPALA-12800: Add cache for isTrueWithNullSlots() evaluation
......................................................................


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21484/8/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

http://gerrit.cloudera.org:8080/#/c/21484/8/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2984
PS8, Line 2984:     } catch (ExecutionException e) {
> Sorry, I mean adding an error message for Preconditions.checkState(). Other
Ah, I misunderstood. Done.



-- 
To view, visit http://gerrit.cloudera.org:8080/21484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
Gerrit-Change-Number: 21484
Gerrit-PatchSet: 12
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Jun 2024 22:43:44 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12800: Add null slots cache

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.

Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21484 )

Change subject: IMPALA-12800: Add null slots cache
......................................................................


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21484/8/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:

http://gerrit.cloudera.org:8080/#/c/21484/8/common/thrift/ImpalaService.thrift@947
PS8, Line 947: USE_NULL_SLOTS_CACHE = 179
What do you think of using custom Guava Weigher that weight each entry as 1.
https://www.baeldung.com/guava-cache#2-eviction-by-weight

And then, change this query option to specify maximum weight for nullSlotsCache. Default to some large number (cache all). Setting 0 means not using cache at all.



-- 
To view, visit http://gerrit.cloudera.org:8080/21484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
Gerrit-Change-Number: 21484
Gerrit-PatchSet: 8
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Mon, 10 Jun 2024 14:17:16 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12800: Add null slots cache

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.

Hello Daniel Becker, Riza Suminto, Joe McDonnell, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/21484

to look at the new patch set (#8).

Change subject: IMPALA-12800: Add null slots cache
......................................................................

IMPALA-12800: Add null slots cache

isTrueWithNullSlots can be expensive when it has to query the backend.
Many of the expressions will look similar, especially in large
auto-generated expressions. Adds a cache based on the nullified
expression to avoid querying the backend for expressions with identical
structure.

With DEBUG logging enabled for the Analyzer, computes and logs stats
about the null slots cache.

Adds 'use_null_slots_cache' query option to disable caching. Documents
the new option.

Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaService.thrift
M common/thrift/Query.thrift
M docs/impala.ditamap
A docs/topics/impala_use_null_slots_cache.xml
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
8 files changed, 126 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/21484/8
-- 
To view, visit http://gerrit.cloudera.org:8080/21484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
Gerrit-Change-Number: 21484
Gerrit-PatchSet: 8
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>

[Impala-ASF-CR] IMPALA-12800: Add null slots cache

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.

Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21484 )

Change subject: IMPALA-12800: Add null slots cache
......................................................................


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21484/8/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

http://gerrit.cloudera.org:8080/#/c/21484/8/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@663
PS8, Line 663: nullSlotsCache =
             :           queryCtx.getClient_request().getQuery_options().use_null_slots_cache ?
             :           CacheBuilder.newBuilder().concurrencyLevel(1).recordStats().build() : null;
nit: I just remember, does Guava has reference-based eviction like Caffeine?
https://github.com/ben-manes/caffeine/wiki/Eviction#reference-based

For example, Caffeine.weakKeys() will let cache entries evicted if the key is not referenced anywhere anymore.



-- 
To view, visit http://gerrit.cloudera.org:8080/21484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
Gerrit-Change-Number: 21484
Gerrit-PatchSet: 8
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 07 Jun 2024 22:37:29 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12800: Add null slots cache

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21484 )

Change subject: IMPALA-12800: Add null slots cache
......................................................................


Patch Set 7: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/21484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
Gerrit-Change-Number: 21484
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Sat, 08 Jun 2024 00:02:23 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12800: Add null slots cache

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.

Hello Daniel Becker, Riza Suminto, Joe McDonnell, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/21484

to look at the new patch set (#6).

Change subject: IMPALA-12800: Add null slots cache
......................................................................

IMPALA-12800: Add null slots cache

isTrueWithNullSlots can be expensive when it has to query the backend.
Many of the expressions will look similar, especially in large
auto-generated expressions. Adds a cache based on the nullified
expression to avoid querying the backend for expressions with identical
structure.

With DEBUG logging enabled for the Analyzer, computes and logs stats
about the null slots cache.

Adds 'use_null_slots_cache' query option to disable caching. Documents
the new option.

Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaService.thrift
M common/thrift/Query.thrift
M docs/impala.ditamap
A docs/topics/impala_use_null_slots_cache.xml
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
8 files changed, 125 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/21484/6
-- 
To view, visit http://gerrit.cloudera.org:8080/21484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
Gerrit-Change-Number: 21484
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>

[Impala-ASF-CR] IMPALA-12800: Add null slots cache

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.

Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21484 )

Change subject: IMPALA-12800: Add null slots cache
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/21484/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/21484/3//COMMIT_MSG@18
PS3, Line 18: Adds 'use_null_slots_cache' query option to disable caching. Documents
            : the new option.
> My thinking was queries fall into 3 categories:
Also, my default would be to use query options for anything that is query-specific. Can always use default_query_options to set a cluster-wide default.


http://gerrit.cloudera.org:8080/#/c/21484/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

http://gerrit.cloudera.org:8080/#/c/21484/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@541
PS3, Line 541: // Null slots cache - for expressions with slots replaced by nulls - to reduce
             :     // backend expression evaluation when query contains many similar expressions.
             :     private final Map<Expr, Boolean> isTrueWithNullSlotsCache = new HashMap<>();
             : 
             :     // Track null slots cache hits.
             :     private int isTrueWithNullSlotsCacheHits = 0;
             : 
             :     // Track null slots cache misses.
             :     private int isTrueWithNullSlotsCacheMisses = 0;
> Sure, I can wrap this up in a private class. I thought about mirroring http
Hmm, guava Cache does cover all the features I want though. My worry is that calculating size for every entry could be expensive (seems to add ~30 ms to the test query), so not sure I want to do it for everything. So we can either default it to unlimited with an option to set a limit, or just set a default limit and eat the extra cost.



-- 
To view, visit http://gerrit.cloudera.org:8080/21484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
Gerrit-Change-Number: 21484
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 06 Jun 2024 21:24:16 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12800: Add null slots cache

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.

Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21484 )

Change subject: IMPALA-12800: Add null slots cache
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21484/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

http://gerrit.cloudera.org:8080/#/c/21484/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@541
PS3, Line 541: // Null slots cache - for expressions with slots replaced by nulls - to reduce
             :     // backend expression evaluation when query contains many similar expressions.
             :     private final Map<Expr, Boolean> isTrueWithNullSlotsCache = new HashMap<>();
             : 
             :     // Track null slots cache hits.
             :     private int isTrueWithNullSlotsCacheHits = 0;
             : 
             :     // Track null slots cache misses.
             :     private int isTrueWithNullSlotsCacheMisses = 0;
> cache_ in CatalogMetaProvider is heavyweight, system-wide, and long lived.
Switched to Guava cache, I don't see any performance loss compared to what I had before.

I didn't add a maximumSize because limiting it to less than all the expressions showed a measurable performance loss, and I don't think there's a universal maximum that makes sense. Open to future improvements here.



-- 
To view, visit http://gerrit.cloudera.org:8080/21484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
Gerrit-Change-Number: 21484
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 06 Jun 2024 22:28:04 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12800: Add null slots cache

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.

Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21484 )

Change subject: IMPALA-12800: Add null slots cache
......................................................................


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21484/8/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:

http://gerrit.cloudera.org:8080/#/c/21484/8/common/thrift/ImpalaService.thrift@947
PS8, Line 947: USE_NULL_SLOTS_CACHE = 179
> What do you think of using custom Guava Weigher that weight each entry as 1
If they all have the same weight, might as well use maxSize.

We could make this an option to configure maxSize. It's a short-lived cache that scales based on number of expressions, so I don't expect the overall cost to be that high. I lean towards keeping the configuration simpler, but open to changing it to a query option that sets the cache max size.



-- 
To view, visit http://gerrit.cloudera.org:8080/21484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
Gerrit-Change-Number: 21484
Gerrit-PatchSet: 8
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Mon, 10 Jun 2024 18:05:15 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12800: Add cache for isTrueWithNullSlots() evaluation

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.

Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21484 )

Change subject: IMPALA-12800: Add cache for isTrueWithNullSlots() evaluation
......................................................................


Patch Set 10:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/21484/9//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/21484/9//COMMIT_MSG@7
PS9, Line 7: IMPALA-12800: Add null slots cache
> nit: Can we use a better title, e.g. "Add cache for isTrueWithNullSlots() e
Done


http://gerrit.cloudera.org:8080/#/c/21484/8/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

http://gerrit.cloudera.org:8080/#/c/21484/8/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2981
PS8, Line 2981:       return globalState_.nullSlotsCache.get(nullTuplePred,
> I think we can improve the hash key (maybe in a follow-up JIRA). Currently,
https://gerrit.cloudera.org/c/21483/6 implemented appropriate hashCodes for all Exprs.


http://gerrit.cloudera.org:8080/#/c/21484/8/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2982
PS8, Line 2982: .booleanValue()
> nit: Unnecessary unboxing. Need 4 spaces indent.
Done


http://gerrit.cloudera.org:8080/#/c/21484/8/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2984
PS8, Line 2984:       Preconditions.checkState(e.getCause() instanceof InternalException);
> nit: can we add an error message for this?
I'd rather not. This is preserving the original error message.

guava Cache#get wraps any Throwable thrown by the loader function in an ExecutionException. I'm unwrapping it to throw the exception we originally threw.



-- 
To view, visit http://gerrit.cloudera.org:8080/21484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
Gerrit-Change-Number: 21484
Gerrit-PatchSet: 10
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Jun 2024 16:29:11 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12800: Add cache for isTrueWithNullSlots() evaluation

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21484 )

Change subject: IMPALA-12800: Add cache for isTrueWithNullSlots() evaluation
......................................................................


Patch Set 12: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/21484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
Gerrit-Change-Number: 21484
Gerrit-PatchSet: 12
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 12 Jun 2024 12:27:04 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12800: Add null slots cache

Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.

Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/21484 )

Change subject: IMPALA-12800: Add null slots cache
......................................................................


Patch Set 8: Code-Review+1

(3 comments)

http://gerrit.cloudera.org:8080/#/c/21484/8/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

http://gerrit.cloudera.org:8080/#/c/21484/8/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2981
PS8, Line 2981:       return globalState_.nullSlotsCache.get(nullTuplePred,
I think we can improve the hash key (maybe in a follow-up JIRA). Currently, for non-literal exprs, it's just the expr id:
https://github.com/apache/impala/blob/825900fa6c3a51941b7b90edb8af6f7dba5e5fe8/fe/src/main/java/org/apache/impala/analysis/Expr.java#L1078
It's ok since Expr.clone() copies the id:
https://github.com/apache/impala/blob/825900fa6c3a51941b7b90edb8af6f7dba5e5fe8/fe/src/main/java/org/apache/impala/analysis/Expr.java#L447

Ideally, if two exprs have the same structure, they can share the results. E.g. if a view is used several times in a query, its expressions in different occurences can share the same eval results.


http://gerrit.cloudera.org:8080/#/c/21484/8/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2982
PS8, Line 2982: .booleanValue()
nit: Unnecessary unboxing. Need 4 spaces indent.


http://gerrit.cloudera.org:8080/#/c/21484/8/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2984
PS8, Line 2984:       Preconditions.checkState(e.getCause() instanceof InternalException);
nit: can we add an error message for this?



-- 
To view, visit http://gerrit.cloudera.org:8080/21484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
Gerrit-Change-Number: 21484
Gerrit-PatchSet: 8
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Jun 2024 00:25:11 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12800: Add null slots cache

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.

Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21484 )

Change subject: IMPALA-12800: Add null slots cache
......................................................................


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21484/8/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

http://gerrit.cloudera.org:8080/#/c/21484/8/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@663
PS8, Line 663: nullSlotsCache =
             :           queryCtx.getClient_request().getQuery_options().use_null_slots_cache ?
             :           CacheBuilder.newBuilder().concurrencyLevel(1).recordStats().build() : null;
> nit: I just remember, does Guava has reference-based eviction like Caffeine
Seems not, or we wouldn't see any cache hits.



-- 
To view, visit http://gerrit.cloudera.org:8080/21484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
Gerrit-Change-Number: 21484
Gerrit-PatchSet: 8
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 07 Jun 2024 22:40:04 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12800: Add null slots cache

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.

Hello Daniel Becker, Riza Suminto, Joe McDonnell, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/21484

to look at the new patch set (#2).

Change subject: IMPALA-12800: Add null slots cache
......................................................................

IMPALA-12800: Add null slots cache

isTrueWithNullSlots can be expensive when it has to query the backend.
Many of the expressions will look similar, especially in large
auto-generated expressions. Adds a cache based on the nullified
expression to avoid querying the backend for expressions with identical
structure.

With DEBUG logging enabled for the Analyzer, computes and logs stats
about the null slots cache.

Adds 'use_null_slots_cache' query option to disable caching. Documents
the new option.

Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
---
M be/src/service/query-options.h
M common/thrift/ImpalaService.thrift
M common/thrift/Query.thrift
M docs/impala.ditamap
A docs/topics/impala_use_null_slots_cache.xml
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
7 files changed, 133 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/21484/2
-- 
To view, visit http://gerrit.cloudera.org:8080/21484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
Gerrit-Change-Number: 21484
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>

[Impala-ASF-CR] IMPALA-12800: Add null slots cache

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.

Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21484 )

Change subject: IMPALA-12800: Add null slots cache
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/21484/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/21484/3//COMMIT_MSG@18
PS3, Line 18: Adds 'use_null_slots_cache' query option to disable caching. Documents
            : the new option.
> Also, my default would be to use query options for anything that is query-s
Ack


http://gerrit.cloudera.org:8080/#/c/21484/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

http://gerrit.cloudera.org:8080/#/c/21484/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@541
PS3, Line 541: // Null slots cache - for expressions with slots replaced by nulls - to reduce
             :     // backend expression evaluation when query contains many similar expressions.
             :     private final Map<Expr, Boolean> isTrueWithNullSlotsCache = new HashMap<>();
             : 
             :     // Track null slots cache hits.
             :     private int isTrueWithNullSlotsCacheHits = 0;
             : 
             :     // Track null slots cache misses.
             :     private int isTrueWithNullSlotsCacheMisses = 0;
> Hmm, guava Cache does cover all the features I want though. My worry is tha
cache_ in CatalogMetaProvider is heavyweight, system-wide, and long lived.

Since this is local within query's Analyzer, simple Guava cache with fixed maximumSize is probably good enough.
See also other Cache lib that Quanlong mention in IMPALA-12682.



-- 
To view, visit http://gerrit.cloudera.org:8080/21484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
Gerrit-Change-Number: 21484
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 06 Jun 2024 21:31:07 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12800: Add null slots cache

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.

Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21484 )

Change subject: IMPALA-12800: Add null slots cache
......................................................................


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21484/8/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

http://gerrit.cloudera.org:8080/#/c/21484/8/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@663
PS8, Line 663: nullSlotsCache =
             :           queryCtx.getClient_request().getQuery_options().use_null_slots_cache ?
             :           CacheBuilder.newBuilder().concurrencyLevel(1).recordStats().build() : null;
> Seems not, or we wouldn't see any cache hits.
Oh, right. Two expression can be equal, but from different instance/reference.



-- 
To view, visit http://gerrit.cloudera.org:8080/21484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
Gerrit-Change-Number: 21484
Gerrit-PatchSet: 8
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 07 Jun 2024 22:44:06 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12800: Add cache for isTrueWithNullSlots() evaluation

Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.

Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/21484 )

Change subject: IMPALA-12800: Add cache for isTrueWithNullSlots() evaluation
......................................................................


Patch Set 11:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/21484/8/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

http://gerrit.cloudera.org:8080/#/c/21484/8/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2981
PS8, Line 2981:       return globalState_.nullSlotsCache.get(nullTuplePred,
> https://gerrit.cloudera.org/c/21483/6 implemented appropriate hashCodes for
Ack


http://gerrit.cloudera.org:8080/#/c/21484/8/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2984
PS8, Line 2984:       Preconditions.checkState(e.getCause() instanceof InternalException);
> I'd rather not. This is preserving the original error message.
Sorry, I mean adding an error message for Preconditions.checkState(). Otherwise, when it fails, clients just get an error of "IllegalStateException: null".



-- 
To view, visit http://gerrit.cloudera.org:8080/21484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
Gerrit-Change-Number: 21484
Gerrit-PatchSet: 11
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Jun 2024 22:05:49 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12800: Add cache for isTrueWithNullSlots() evaluation

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/21484 )

Change subject: IMPALA-12800: Add cache for isTrueWithNullSlots() evaluation
......................................................................

IMPALA-12800: Add cache for isTrueWithNullSlots() evaluation

isTrueWithNullSlots() can be expensive when it has to query the backend.
Many of the expressions will look similar, especially in large
auto-generated expressions. Adds a cache based on the nullified
expression to avoid querying the backend for expressions with identical
structure.

With DEBUG logging enabled for the Analyzer, computes and logs stats
about the null slots cache.

Adds 'use_null_slots_cache' query option to disable caching. Documents
the new option.

Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
Reviewed-on: http://gerrit.cloudera.org:8080/21484
Reviewed-by: Quanlong Huang <hu...@gmail.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaService.thrift
M common/thrift/Query.thrift
M docs/impala.ditamap
A docs/topics/impala_use_null_slots_cache.xml
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
8 files changed, 129 insertions(+), 3 deletions(-)

Approvals:
  Quanlong Huang: Looks good to me, approved
  Impala Public Jenkins: Verified

-- 
To view, visit http://gerrit.cloudera.org:8080/21484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
Gerrit-Change-Number: 21484
Gerrit-PatchSet: 13
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>

[Impala-ASF-CR] IMPALA-12800: Add null slots cache

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.

Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21484 )

Change subject: IMPALA-12800: Add null slots cache
......................................................................


Patch Set 7:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/21484/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

http://gerrit.cloudera.org:8080/#/c/21484/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@541
PS3, Line 541: private final Set<PrivilegeRequest> privilegeReqs = new LinkedHashSet<>();
             : 
             :     // List of PrivilegeRequest to custom authorization failure error message.
             :     // Tracks all privilege requests on catalog objects that need a custom
             :     // error message returned to avoid exposing existence of catalog objects.
             :     private final List<Pair<PrivilegeRequest, String>> maskedPrivilegeReqs =
             :         new ArrayList<>();
             : 
             :     // accesses to catalog objects
> Switched to Guava cache, I don't see any performance loss compared to what 
Done


http://gerrit.cloudera.org:8080/#/c/21484/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

http://gerrit.cloudera.org:8080/#/c/21484/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2979
PS7, Line 2979: getIfPresent(nullTuplePred);
you can use get(K key, Callable<? extends V> loader) here.


http://gerrit.cloudera.org:8080/#/c/21484/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2992
PS7, Line 2992: if (LOG.isDebugEnabled() && globalState_.nullSlotsCache != null)
nit: can return early here, saving indentation.

if (!LOG.isDebugEnabled() || globalState_.nullSlotsCache == null) return;


http://gerrit.cloudera.org:8080/#/c/21484/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2994
PS7, Line 2994: MemoryMeter meter = MemoryMeter.builder().build();
Just a note:

We probably should log WARNING if any of GlobalState field member's size grows prohibitively large. I'm not sure if it is possible to meter everything though.



-- 
To view, visit http://gerrit.cloudera.org:8080/21484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
Gerrit-Change-Number: 21484
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 07 Jun 2024 21:48:12 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12800: Add null slots cache

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.

Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21484 )

Change subject: IMPALA-12800: Add null slots cache
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21484/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

http://gerrit.cloudera.org:8080/#/c/21484/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3004
PS6, Line 3004: 99th perce
> Shouldn't we include "percentile" here? I first thought it was the value of
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/21484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib63f5553284f21f775d2097b6c5d6bbb63699acd
Gerrit-Change-Number: 21484
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 07 Jun 2024 18:48:33 +0000
Gerrit-HasComments: Yes