From 8df4b6adf3e817df47616ac4e3efd7d6a2b6e4dc Mon Sep 17 00:00:00 2001 From: wentjin Date: Wed, 8 Sep 2021 17:22:30 +0800 Subject: [PATCH 1/2] Fix JSONPath cache inefficient issue Addressed comments to improve unit test Addressed review comments - Remove Constants class and configure inlined into JsonPathMapCache - Remove useless ConcurrentHashMap parameters Addressed review comments Co-authored-by: Xiaotian (Jackie) Jiang <17555551+Jackie-Jiang@users.noreply.github.com> Change cache maximum size to reduce memory usage use Guava cache for json path add json path cache integration test --- .../common/function/FunctionRegistry.java | 4 ++ .../pinot/common/function/JsonPathCache.java | 46 +++++++++++++++++++ .../tests/JsonPathClusterIntegrationTest.java | 12 +++++ 3 files changed, 62 insertions(+) create mode 100644 pinot-common/src/main/java/org/apache/pinot/common/function/JsonPathCache.java diff --git a/pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java b/pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java index 2d71654a073..a6f4b6766bc 100644 --- a/pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java +++ b/pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java @@ -19,6 +19,7 @@ package org.apache.pinot.common.function; import com.google.common.base.Preconditions; +import com.jayway.jsonpath.spi.cache.CacheProvider; import java.lang.reflect.Method; import java.util.HashMap; import java.util.Map; @@ -70,6 +71,9 @@ private FunctionRegistry() { } LOGGER.info("Initialized FunctionRegistry with {} functions: {} in {}ms", FUNCTION_INFO_MAP.size(), FUNCTION_INFO_MAP.keySet(), System.currentTimeMillis() - startTimeMs); + + // set JsonPath cache before the cache is accessed + CacheProvider.setCache(new JsonPathCache()); } /** diff --git a/pinot-common/src/main/java/org/apache/pinot/common/function/JsonPathCache.java b/pinot-common/src/main/java/org/apache/pinot/common/function/JsonPathCache.java new file mode 100644 index 00000000000..991e790ad0a --- /dev/null +++ b/pinot-common/src/main/java/org/apache/pinot/common/function/JsonPathCache.java @@ -0,0 +1,46 @@ +/** + * 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.function; + +import com.google.common.annotations.VisibleForTesting; +import com.google.common.cache.CacheBuilder; +import com.jayway.jsonpath.JsonPath; +import com.jayway.jsonpath.spi.cache.Cache; + +public class JsonPathCache implements Cache { + private static final long DEFAULT_CACHE_MAXIMUM_SIZE = 20000; + private final com.google.common.cache.Cache _jsonPathCache = CacheBuilder.newBuilder() + .maximumSize(DEFAULT_CACHE_MAXIMUM_SIZE) + .build(); + + @Override + public JsonPath get(String key) { + return _jsonPathCache.getIfPresent(key); + } + + @Override + public void put(String key, JsonPath value) { + _jsonPathCache.put(key, value); + } + + @VisibleForTesting + public long size() { + return _jsonPathCache.size(); + } +} \ No newline at end of file diff --git a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/JsonPathClusterIntegrationTest.java b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/JsonPathClusterIntegrationTest.java index 9121bb21ff3..789fd62c32e 100644 --- a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/JsonPathClusterIntegrationTest.java +++ b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/JsonPathClusterIntegrationTest.java @@ -22,6 +22,8 @@ import com.fasterxml.jackson.databind.node.ArrayNode; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; +import com.jayway.jsonpath.spi.cache.Cache; +import com.jayway.jsonpath.spi.cache.CacheProvider; import java.io.File; import java.util.ArrayList; import java.util.Arrays; @@ -35,6 +37,7 @@ import org.apache.avro.generic.GenericData; import org.apache.avro.generic.GenericDatumWriter; import org.apache.commons.io.FileUtils; +import org.apache.pinot.common.function.JsonPathCache; import org.apache.pinot.spi.config.table.TableConfig; import org.apache.pinot.spi.config.table.TableType; import org.apache.pinot.spi.config.table.ingestion.IngestionConfig; @@ -471,6 +474,15 @@ void testFailedSqlQuery() Assert.assertEquals(pinotResponse.get("totalDocs").asInt(), 0); } + @Test + public void testJSONPathCache() { + Cache cache = CacheProvider.getCache(); + // check cache type + Assert.assertEquals(cache.getClass(), JsonPathCache.class); + // check cache used + Assert.assertTrue(((JsonPathCache) cache).size() > 0); + } + @AfterClass public void tearDown() throws Exception { From fd7aed02495c5839277e44fc63acfa7905ebbce7 Mon Sep 17 00:00:00 2001 From: "Xiaotian (Jackie) Jiang" Date: Tue, 2 Nov 2021 12:26:46 -0700 Subject: [PATCH 2/2] Rebase, cleanup and fix test Fix the issue of setting different default configurations Do not access the JsonPath cache in the transform function --- .../common/function/FunctionRegistry.java | 4 -- .../pinot/common/function/JsonPathCache.java | 35 +++++----- .../common/function/scalar/JsonFunctions.java | 42 ++++-------- .../JsonExtractKeyTransformFunction.java | 64 ++++++------------- .../JsonExtractScalarTransformFunction.java | 49 ++++---------- .../tests/JsonPathClusterIntegrationTest.java | 6 +- 6 files changed, 66 insertions(+), 134 deletions(-) diff --git a/pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java b/pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java index a6f4b6766bc..2d71654a073 100644 --- a/pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java +++ b/pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java @@ -19,7 +19,6 @@ package org.apache.pinot.common.function; import com.google.common.base.Preconditions; -import com.jayway.jsonpath.spi.cache.CacheProvider; import java.lang.reflect.Method; import java.util.HashMap; import java.util.Map; @@ -71,9 +70,6 @@ private FunctionRegistry() { } LOGGER.info("Initialized FunctionRegistry with {} functions: {} in {}ms", FUNCTION_INFO_MAP.size(), FUNCTION_INFO_MAP.keySet(), System.currentTimeMillis() - startTimeMs); - - // set JsonPath cache before the cache is accessed - CacheProvider.setCache(new JsonPathCache()); } /** diff --git a/pinot-common/src/main/java/org/apache/pinot/common/function/JsonPathCache.java b/pinot-common/src/main/java/org/apache/pinot/common/function/JsonPathCache.java index 991e790ad0a..3f012f410da 100644 --- a/pinot-common/src/main/java/org/apache/pinot/common/function/JsonPathCache.java +++ b/pinot-common/src/main/java/org/apache/pinot/common/function/JsonPathCache.java @@ -23,24 +23,25 @@ import com.jayway.jsonpath.JsonPath; import com.jayway.jsonpath.spi.cache.Cache; + public class JsonPathCache implements Cache { - private static final long DEFAULT_CACHE_MAXIMUM_SIZE = 20000; - private final com.google.common.cache.Cache _jsonPathCache = CacheBuilder.newBuilder() - .maximumSize(DEFAULT_CACHE_MAXIMUM_SIZE) - .build(); + private static final long DEFAULT_CACHE_MAXIMUM_SIZE = 10000; + + private final com.google.common.cache.Cache _jsonPathCache = + CacheBuilder.newBuilder().maximumSize(DEFAULT_CACHE_MAXIMUM_SIZE).build(); - @Override - public JsonPath get(String key) { - return _jsonPathCache.getIfPresent(key); - } + @Override + public JsonPath get(String key) { + return _jsonPathCache.getIfPresent(key); + } - @Override - public void put(String key, JsonPath value) { - _jsonPathCache.put(key, value); - } + @Override + public void put(String key, JsonPath value) { + _jsonPathCache.put(key, value); + } - @VisibleForTesting - public long size() { - return _jsonPathCache.size(); - } -} \ No newline at end of file + @VisibleForTesting + public long size() { + return _jsonPathCache.size(); + } +} diff --git a/pinot-common/src/main/java/org/apache/pinot/common/function/scalar/JsonFunctions.java b/pinot-common/src/main/java/org/apache/pinot/common/function/scalar/JsonFunctions.java index acb3a379cdb..0285f7b1bea 100644 --- a/pinot-common/src/main/java/org/apache/pinot/common/function/scalar/JsonFunctions.java +++ b/pinot-common/src/main/java/org/apache/pinot/common/function/scalar/JsonFunctions.java @@ -21,19 +21,16 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.google.common.annotations.VisibleForTesting; import com.jayway.jsonpath.Configuration; -import com.jayway.jsonpath.Option; +import com.jayway.jsonpath.JsonPath; import com.jayway.jsonpath.ParseContext; import com.jayway.jsonpath.Predicate; -import com.jayway.jsonpath.internal.ParseContextImpl; +import com.jayway.jsonpath.spi.cache.CacheProvider; import com.jayway.jsonpath.spi.json.JacksonJsonProvider; -import com.jayway.jsonpath.spi.json.JsonProvider; import com.jayway.jsonpath.spi.mapper.JacksonMappingProvider; -import com.jayway.jsonpath.spi.mapper.MappingProvider; import java.util.Arrays; -import java.util.EnumSet; import java.util.List; import java.util.Map; -import java.util.Set; +import org.apache.pinot.common.function.JsonPathCache; import org.apache.pinot.spi.annotations.ScalarFunction; import org.apache.pinot.spi.utils.JsonUtils; @@ -50,32 +47,17 @@ * */ public class JsonFunctions { - private static final ParseContext PARSE_CONTEXT; - private static final Predicate[] NO_PREDICATES = new Predicate[0]; - static { - Configuration.setDefaults(new Configuration.Defaults() { - private final JsonProvider _jsonProvider = new ArrayAwareJacksonJsonProvider(); - private final MappingProvider _mappingProvider = new JacksonMappingProvider(); - - @Override - public JsonProvider jsonProvider() { - return _jsonProvider; - } - - @Override - public MappingProvider mappingProvider() { - return _mappingProvider; - } - - @Override - public Set