You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "xiangfu0 (via GitHub)" <gi...@apache.org> on 2025/04/22 04:26:54 UTC
[PR] Adding segment level QueryCache for expensive predicates [pinot]
xiangfu0 opened a new pull request, #15606:
URL: https://github.com/apache/pinot/pull/15606
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding segment level QueryCache for expensive predicates [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #15606:
URL: https://github.com/apache/pinot/pull/15606#issuecomment-2820099649
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/15606?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `12.00000%` with `110 lines` in your changes missing coverage. Please review.
> Project coverage is 33.52%. Comparing base [(`1fdeadd`)](https://app.codecov.io/gh/apache/pinot/commit/1fdeadd6ef9ad3c6ca51f198ac0d4f922e92a0b7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`ed9add8`)](https://app.codecov.io/gh/apache/pinot/commit/ed9add809a1bf5e1bb74fd2e6d1bda168f385ff5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/15606?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ot/core/operator/CachedBitmapDocIdSetOperator.java](https://app.codecov.io/gh/apache/pinot/pull/15606?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Foperator%2FCachedBitmapDocIdSetOperator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9DYWNoZWRCaXRtYXBEb2NJZFNldE9wZXJhdG9yLmphdmE=) | 0.00% | [22 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15606?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...re/operator/filter/CachedBitmapFilterOperator.java](https://app.codecov.io/gh/apache/pinot/pull/15606?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Foperator%2Ffilter%2FCachedBitmapFilterOperator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvQ2FjaGVkQml0bWFwRmlsdGVyT3BlcmF0b3IuamF2YQ==) | 0.00% | [14 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15606?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...a/org/apache/pinot/core/plan/DocIdSetPlanNode.java](https://app.codecov.io/gh/apache/pinot/pull/15606?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fplan%2FDocIdSetPlanNode.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0RvY0lkU2V0UGxhbk5vZGUuamF2YQ==) | 0.00% | [13 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15606?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...org/apache/pinot/spi/config/table/CacheConfig.java](https://app.codecov.io/gh/apache/pinot/pull/15606?src=pr&el=tree&filepath=pinot-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fspi%2Fconfig%2Ftable%2FCacheConfig.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0NhY2hlQ29uZmlnLmphdmE=) | 0.00% | [12 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15606?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...g/apache/pinot/core/operator/DocIdSetOperator.java](https://app.codecov.io/gh/apache/pinot/pull/15606?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Foperator%2FDocIdSetOperator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9Eb2NJZFNldE9wZXJhdG9yLmphdmE=) | 0.00% | [11 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15606?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...apache/pinot/core/query/utils/QueryCacheUtils.java](https://app.codecov.io/gh/apache/pinot/pull/15606?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fquery%2Futils%2FQueryCacheUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS91dGlscy9RdWVyeUNhY2hlVXRpbHMuamF2YQ==) | 0.00% | [9 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15606?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...java/org/apache/pinot/common/cache/QueryCache.java](https://app.codecov.io/gh/apache/pinot/pull/15606?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Fcache%2FQueryCache.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY2FjaGUvUXVlcnlDYWNoZS5qYXZh) | 0.00% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15606?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ava/org/apache/pinot/core/plan/FilterPlanNode.java](https://app.codecov.io/gh/apache/pinot/pull/15606?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fplan%2FFilterPlanNode.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0ZpbHRlclBsYW5Ob2RlLmphdmE=) | 0.00% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15606?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../apache/pinot/common/cache/InMemoryQueryCache.java](https://app.codecov.io/gh/apache/pinot/pull/15606?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Fcache%2FInMemoryQueryCache.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY2FjaGUvSW5NZW1vcnlRdWVyeUNhY2hlLmphdmE=) | 50.00% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15606?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...g/apache/pinot/common/cache/QueryCacheFactory.java](https://app.codecov.io/gh/apache/pinot/pull/15606?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Fcache%2FQueryCacheFactory.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY2FjaGUvUXVlcnlDYWNoZUZhY3RvcnkuamF2YQ==) | 54.54% | [2 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15606?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| ... and [3 more](https://app.codecov.io/gh/apache/pinot/pull/15606?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
> :exclamation: There is a different number of reports uploaded between BASE (1fdeadd) and HEAD (ed9add8). Click for more details.
>
> <details><summary>HEAD has 62 uploads less than BASE</summary>
>
>| Flag | BASE (1fdeadd) | HEAD (ed9add8) |
>|------|------|------|
>|unittests|6|2|
>|unittests1|3|0|
>|java-21|10|2|
>|skip-bytebuffers-false|8|1|
>|temurin|15|2|
>|unittests2|3|2|
>|java-11|5|0|
>|integration|9|0|
>|skip-bytebuffers-true|4|1|
>|integration2|3|0|
>|integration1|3|0|
>|custom-integration1|3|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #15606 +/- ##
=============================================
- Coverage 62.80% 33.52% -29.28%
+ Complexity 1384 690 -694
=============================================
Files 2864 2871 +7
Lines 162708 162827 +119
Branches 24910 24927 +17
=============================================
- Hits 102181 54583 -47598
- Misses 52822 103875 +51053
+ Partials 7705 4369 -3336
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/15606/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/15606/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/15606/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/15606/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/15606/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/15606/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/15606/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.52% <12.00%> (-29.26%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/15606/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.50% <12.00%> (-29.29%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/15606/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.51% <12.00%> (-29.25%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/15606/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.52% <12.00%> (-29.28%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/15606/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.52% <12.00%> (-29.28%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/15606/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/15606/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.52% <12.00%> (-0.02%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/15606?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding segment level QueryCache for expensive predicates [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #15606:
URL: https://github.com/apache/pinot/pull/15606#issuecomment-2923069278
> Have you evaluated the memory overhead of the cache? Since everything is kept in memory, I feel it can easily cause server OOM. E.g. 1000 segments -> 1M bitmaps in memory, where bitmap for a segment could easily go to KBs
We need to be able to configure those and ideally implement multiple cache mechanisms.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding segment level QueryCache for expensive predicates [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #15606:
URL: https://github.com/apache/pinot/pull/15606#discussion_r2065851053
##########
pinot-common/src/main/java/org/apache/pinot/common/cache/QueryCache.java:
##########
@@ -0,0 +1,71 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.cache;
+
+import org.apache.pinot.common.request.context.FilterContext;
+
+
+public interface QueryCache {
+ /**
+ * Put the given key-value pair into the cache.
+ *
+ * @param key The key to be put into the cache
+ * @param value The value to be put into the cache
+ */
+ void put(String key, Object value);
+
+ /**
+ * Get the value for the given key from the cache.
+ *
+ * @param key The key to get from the cache
+ * @return The value for the given key, or null if not found
+ */
+ Object get(String key);
+
+ /**
+ * Remove the given key from the cache.
+ *
+ * @param key The key to remove from the cache
+ */
+ void remove(String key);
+
+ /**
+ * Get the cache key for the given table, segment name, and filter context.
+ * @param segmentName The segment name
+ * @param filterContext The filter context
+ * @return The cache key
+ */
+ static String getCacheKey(String segmentName, FilterContext filterContext) {
+ return segmentName + "_" + filterContext.hashCode();
Review Comment:
Consider using a more robust mechanism to generate cache keys for filter contexts instead of relying solely on filterContext.hashCode(), to mitigate potential hash collisions for complex filters.
```suggestion
return segmentName + "_" + filterContext.toString();
```
##########
pinot-common/src/main/java/org/apache/pinot/common/cache/QueryCacheFactory.java:
##########
@@ -0,0 +1,60 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.cache;
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
+
+public class QueryCacheFactory {
+ private static final Map<String, QueryCache> CACHE_MAP = new ConcurrentHashMap<>();
+
+ private QueryCacheFactory() {
+ }
+
+ /**
+ * Creates a new instance of the QueryCache.
+ *
+ * @return A new instance of the QueryCache
+ */
+ public static QueryCache createQueryCache(String table, String cacheType) {
+ if (CACHE_MAP.get(table) == null) {
+ synchronized (CACHE_MAP) {
+ if (CACHE_MAP.get(table) == null) {
+ CACHE_MAP.put(table, createCache(cacheType));
+ }
+ }
+ }
+ return CACHE_MAP.get(table);
Review Comment:
Consider using ConcurrentHashMap's computeIfAbsent method to simplify the cache creation logic and further ensure thread safety.
```suggestion
return CACHE_MAP.computeIfAbsent(table, key -> createCache(cacheType));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding segment level QueryCache for expensive predicates [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #15606:
URL: https://github.com/apache/pinot/pull/15606#discussion_r2116313020
##########
pinot-common/src/main/java/org/apache/pinot/common/cache/QueryCache.java:
##########
@@ -0,0 +1,71 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.cache;
+
+import org.apache.pinot.common.request.context.FilterContext;
+
+
+public interface QueryCache {
+ /**
+ * Put the given key-value pair into the cache.
+ *
+ * @param key The key to be put into the cache
+ * @param value The value to be put into the cache
+ */
+ void put(String key, Object value);
+
+ /**
+ * Get the value for the given key from the cache.
+ *
+ * @param key The key to get from the cache
+ * @return The value for the given key, or null if not found
+ */
+ Object get(String key);
+
+ /**
+ * Remove the given key from the cache.
+ *
+ * @param key The key to remove from the cache
+ */
+ void remove(String key);
+
+ /**
+ * Get the cache key for the given table, segment name, and filter context.
+ * @param segmentName The segment name
+ * @param filterContext The filter context
+ * @return The cache key
+ */
+ static String getCacheKey(String segmentName, FilterContext filterContext) {
+ return segmentName + "_" + filterContext.hashCode();
Review Comment:
considering the cache expiration, the collision rate is low.
since we keep bitmap per segment, it has to be in the key.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding segment level QueryCache for expensive predicates [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #15606:
URL: https://github.com/apache/pinot/pull/15606#discussion_r2078502594
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/ExpressionContext.java:
##########
@@ -117,7 +117,7 @@ public boolean equals(Object o) {
@Override
public int hashCode() {
- int hash = 31 * 31 * _type.hashCode();
+ int hash = 31 * 31 * _type.toString().hashCode();
Review Comment:
Why? It is adding unnecessary overhead
##########
pinot-common/src/main/java/org/apache/pinot/common/cache/QueryCache.java:
##########
@@ -0,0 +1,71 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.cache;
+
+import org.apache.pinot.common.request.context.FilterContext;
+
+
+public interface QueryCache {
+ /**
+ * Put the given key-value pair into the cache.
+ *
+ * @param key The key to be put into the cache
+ * @param value The value to be put into the cache
+ */
+ void put(String key, Object value);
+
+ /**
+ * Get the value for the given key from the cache.
+ *
+ * @param key The key to get from the cache
+ * @return The value for the given key, or null if not found
+ */
+ Object get(String key);
+
+ /**
+ * Remove the given key from the cache.
+ *
+ * @param key The key to remove from the cache
+ */
+ void remove(String key);
+
+ /**
+ * Get the cache key for the given table, segment name, and filter context.
+ * @param segmentName The segment name
+ * @param filterContext The filter context
+ * @return The cache key
+ */
+ static String getCacheKey(String segmentName, FilterContext filterContext) {
+ return segmentName + "_" + filterContext.hashCode();
Review Comment:
What is the chance of hash collision?
##########
pinot-common/src/main/java/org/apache/pinot/common/cache/QueryCache.java:
##########
@@ -0,0 +1,71 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.cache;
+
+import org.apache.pinot.common.request.context.FilterContext;
+
+
+public interface QueryCache {
+ /**
+ * Put the given key-value pair into the cache.
+ *
+ * @param key The key to be put into the cache
+ * @param value The value to be put into the cache
+ */
+ void put(String key, Object value);
+
+ /**
+ * Get the value for the given key from the cache.
+ *
+ * @param key The key to get from the cache
+ * @return The value for the given key, or null if not found
+ */
+ Object get(String key);
+
+ /**
+ * Remove the given key from the cache.
+ *
+ * @param key The key to remove from the cache
+ */
+ void remove(String key);
+
+ /**
+ * Get the cache key for the given table, segment name, and filter context.
+ * @param segmentName The segment name
+ * @param filterContext The filter context
+ * @return The cache key
+ */
+ static String getCacheKey(String segmentName, FilterContext filterContext) {
+ return segmentName + "_" + filterContext.hashCode();
Review Comment:
Do you need `segmentName` as part of the key?
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/FilterPlanNode.java:
##########
@@ -196,6 +202,15 @@ private boolean canApplyH3IndexForInclusionCheck(Predicate predicate, FunctionCo
* Helper method to build the operator tree from the filter.
*/
private BaseFilterOperator constructPhysicalOperator(FilterContext filter, int numDocs) {
+ if (_queryCache != null) {
Review Comment:
Do we invalidate cache when segment is replaced?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/QueryConfig.java:
##########
@@ -56,13 +56,27 @@ public class QueryConfig extends BaseJsonConfig {
// Indicates the maximum length of the serialized response per server for a query.
private final Long _maxServerResponseSizeBytes;
- @JsonCreator
+ // Table config override for query cache
+ private final CacheConfig _cacheConfig;
+
public QueryConfig(@JsonProperty("timeoutMs") @Nullable Long timeoutMs,
@JsonProperty("disableGroovy") @Nullable Boolean disableGroovy,
@JsonProperty("useApproximateFunction") @Nullable Boolean useApproximateFunction,
@JsonProperty("expressionOverrideMap") @Nullable Map<String, String> expressionOverrideMap,
@JsonProperty("maxQueryResponseSizeBytes") @Nullable Long maxQueryResponseSizeBytes,
@JsonProperty("maxServerResponseSizeBytes") @Nullable Long maxServerResponseSizeBytes) {
+ this(timeoutMs, disableGroovy, useApproximateFunction, expressionOverrideMap, maxQueryResponseSizeBytes,
+ maxServerResponseSizeBytes, null);
+ }
+
+ @JsonCreator
+ public QueryConfig(@JsonProperty("timeoutMs") @Nullable Long timeoutMs,
Review Comment:
(minor) You only need to annotate one constructor. If you decide to keep the other constructor, then no need to change the usage in other classes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding segment level QueryCache for expensive predicates [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #15606:
URL: https://github.com/apache/pinot/pull/15606#discussion_r2116321866
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/QueryConfig.java:
##########
@@ -56,13 +56,27 @@ public class QueryConfig extends BaseJsonConfig {
// Indicates the maximum length of the serialized response per server for a query.
private final Long _maxServerResponseSizeBytes;
- @JsonCreator
+ // Table config override for query cache
+ private final CacheConfig _cacheConfig;
+
public QueryConfig(@JsonProperty("timeoutMs") @Nullable Long timeoutMs,
@JsonProperty("disableGroovy") @Nullable Boolean disableGroovy,
@JsonProperty("useApproximateFunction") @Nullable Boolean useApproximateFunction,
@JsonProperty("expressionOverrideMap") @Nullable Map<String, String> expressionOverrideMap,
@JsonProperty("maxQueryResponseSizeBytes") @Nullable Long maxQueryResponseSizeBytes,
@JsonProperty("maxServerResponseSizeBytes") @Nullable Long maxServerResponseSizeBytes) {
+ this(timeoutMs, disableGroovy, useApproximateFunction, expressionOverrideMap, maxQueryResponseSizeBytes,
+ maxServerResponseSizeBytes, null);
+ }
+
+ @JsonCreator
+ public QueryConfig(@JsonProperty("timeoutMs") @Nullable Long timeoutMs,
Review Comment:
remove the annotation for the above method for backward compatibility check.
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/ExpressionContext.java:
##########
@@ -117,7 +117,7 @@ public boolean equals(Object o) {
@Override
public int hashCode() {
- int hash = 31 * 31 * _type.hashCode();
+ int hash = 31 * 31 * _type.toString().hashCode();
Review Comment:
We might need this hash to be stable to support on-disk based cache if users have nvme ssd etc
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/FilterPlanNode.java:
##########
@@ -196,6 +202,15 @@ private boolean canApplyH3IndexForInclusionCheck(Predicate predicate, FunctionCo
* Helper method to build the operator tree from the filter.
*/
private BaseFilterOperator constructPhysicalOperator(FilterContext filter, int numDocs) {
+ if (_queryCache != null) {
Review Comment:
no, will add this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding segment level QueryCache for expensive predicates [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #15606:
URL: https://github.com/apache/pinot/pull/15606#discussion_r2068765761
##########
pinot-common/src/main/java/org/apache/pinot/common/cache/InMemoryQueryCache.java:
##########
@@ -0,0 +1,56 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.cache;
+
+import org.apache.commons.collections.map.LRUMap;
+
+
+/**
+ * InMemoryQueryCache is a simple implementation of the QueryCache interface that stores
+ * key-value pairs in memory using a ConcurrentHashMap.
+ * The implementation is a LRU based cache, which means that it will evict the least recently used
+ * entries when the cache reaches its maximum size.
+ */
+public class InMemoryQueryCache implements QueryCache {
+ private static final int DEFAULT_CACHE_SIZE = 1000;
+ private final LRUMap _cache;
+
+ public InMemoryQueryCache() {
+ this(DEFAULT_CACHE_SIZE);
+ }
+
+ public InMemoryQueryCache(int size) {
+ _cache = new LRUMap(size);
Review Comment:
LRUMap from Apache Commons Collections is not thread-safe, which might lead to concurrency issues in a production environment; consider using a thread-safe cache implementation instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding segment level QueryCache for expensive predicates [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #15606:
URL: https://github.com/apache/pinot/pull/15606#discussion_r2116316666
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/ExpressionContext.java:
##########
@@ -117,7 +117,7 @@ public boolean equals(Object o) {
@Override
public int hashCode() {
- int hash = 31 * 31 * _type.hashCode();
+ int hash = 31 * 31 * _type.toString().hashCode();
Review Comment:
true, however for enum, the hashcode() method is stable within the JVM process, but not stable across multiple JVMs. to ensure the hashcode is stable, we need to use the string hash.
Maybe we could cache all the hashcode in a Map to speed up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding segment level QueryCache for expensive predicates [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 closed pull request #15606: Adding segment level QueryCache for expensive predicates
URL: https://github.com/apache/pinot/pull/15606
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding segment level QueryCache for expensive predicates [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #15606:
URL: https://github.com/apache/pinot/pull/15606#discussion_r2065864417
##########
pinot-common/src/main/java/org/apache/pinot/common/cache/QueryCache.java:
##########
@@ -0,0 +1,71 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.cache;
+
+import org.apache.pinot.common.request.context.FilterContext;
+
+
+public interface QueryCache {
+ /**
+ * Put the given key-value pair into the cache.
+ *
+ * @param key The key to be put into the cache
+ * @param value The value to be put into the cache
+ */
+ void put(String key, Object value);
+
+ /**
+ * Get the value for the given key from the cache.
+ *
+ * @param key The key to get from the cache
+ * @return The value for the given key, or null if not found
+ */
+ Object get(String key);
+
+ /**
+ * Remove the given key from the cache.
+ *
+ * @param key The key to remove from the cache
+ */
+ void remove(String key);
+
+ /**
+ * Get the cache key for the given table, segment name, and filter context.
+ * @param segmentName The segment name
+ * @param filterContext The filter context
+ * @return The cache key
+ */
+ static String getCacheKey(String segmentName, FilterContext filterContext) {
+ return segmentName + "_" + filterContext.hashCode();
Review Comment:
toString() may get to a very large string
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org