xiangfu0 commented on code in PR #18972:
URL: https://github.com/apache/pinot/pull/18972#discussion_r3577536210
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -173,4 +176,21 @@ public static void
initRequestUtilsConfig(PinotConfiguration instanceConfig) {
CommonConstants.Helix.DEFAULT_SSE_LEGACY_LITERAL_UNESCAPING);
RequestUtils.setUseLegacyLiteralUnescaping(useLegacyLiteralUnescaping);
}
+
+ public static void initJsonPathFastPathConfig(PinotConfiguration
instanceConfig) {
+ boolean enabled =
instanceConfig.getProperty(CommonConstants.CONFIG_OF_JSONPATH_FASTPATH_ENABLED,
+ JsonPathFastPathMode.isEnabled());
+ boolean earlyExitEnabled =
+
instanceConfig.getProperty(CommonConstants.CONFIG_OF_JSONPATH_FASTPATH_EARLY_EXIT_ENABLED,
+ JsonPathFastPathMode.isEarlyExitEnabled());
+ JsonPathFastPathMode.setEnabled(enabled);
+ JsonPathFastPathMode.setEarlyExitEnabled(earlyExitEnabled);
+ if (enabled) {
+ LOGGER.info("Enabled the streaming JsonPath fast path, early exit: {}",
earlyExitEnabled);
+ } else if (earlyExitEnabled) {
+ LOGGER.warn("Ignoring {} because {} is not enabled",
+ CommonConstants.CONFIG_OF_JSONPATH_FASTPATH_EARLY_EXIT_ENABLED,
+ CommonConstants.CONFIG_OF_JSONPATH_FASTPATH_ENABLED);
+ }
+ }
Review Comment:
Reworded β the flag really is applied, it is just inert, so the log now says
so: "{earlyExit} is set but has no effect because {fastpath} is disabled". Kept
it applied rather than forcing it false so the stored state still reflects the
operatorβs config.
_π€ Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/JsonFunctions.java:
##########
@@ -100,11 +103,26 @@ public static String jsonFormat(Object object)
@ScalarFunction
public static Object jsonPath(Object object, String jsonPath) {
if (object instanceof String) {
- return PARSE_CONTEXT.parse((String) object).read(jsonPath,
NO_PREDICATES);
+ String json = (String) object;
+ SimpleJsonPath simpleJsonPath = simpleJsonPath(jsonPath);
+ if (simpleJsonPath != null &&
StreamingJsonPathExtractor.canExtract(json)) {
+ return StreamingJsonPathExtractor.extract(json, simpleJsonPath, false,
+ JsonPathFastPathMode.isEarlyExitEnabled());
+ }
+ return PARSE_CONTEXT.parse(json).read(jsonPath, NO_PREDICATES);
}
return PARSE_CONTEXT.parse(object).read(jsonPath, NO_PREDICATES);
}
+ /**
+ * Returns the compiled {@link SimpleJsonPath} when the streaming fast path
is enabled and {@code jsonPath} is a
+ * simple linear chain, and {@code null} when the extraction has to fall
back to Jayway.
+ */
+ @Nullable
+ private static SimpleJsonPath simpleJsonPath(String jsonPath) {
+ return JsonPathFastPathMode.isEnabled() ? SimpleJsonPath.compile(jsonPath)
: null;
+ }
Review Comment:
Fixed β `SimpleJsonPath.compile` now returns `null` for a null path instead
of NPE-ing in the cache, so a null path falls through to Jayway and raises
exactly the exception it raised before. That preserves the core invariant that
enabling the fast path never changes what a caller sees.
_π€ Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-common/src/main/java/org/apache/pinot/common/function/StreamingJsonPathExtractor.java:
##########
@@ -0,0 +1,303 @@
+/**
+ * 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.fasterxml.jackson.core.JsonFactory;
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.core.JsonToken;
+import com.fasterxml.jackson.databind.DeserializationFeature;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.base.Preconditions;
+import com.jayway.jsonpath.InvalidJsonException;
+import java.io.IOException;
+import java.util.List;
+import java.util.Map;
+import javax.annotation.Nullable;
+
+
+/// Resolves one or more [SimpleJsonPath]s against a JSON document in a single
forward pass of a Jackson
+/// [JsonParser], materializing only the addressed values. Subtrees that no
path descends into are consumed
+/// with `skipChildren()`, so no intermediate `Map` / `List` is ever built for
them. This replaces
+/// `JsonPath.parse(json).read(path)`, which builds a full Jackson DOM of the
document and only then walks to
+/// the field.
+///
+/// **Semantics.** The results are byte-for-byte identical to Jayway
configured as Pinot configures it
+/// (`JacksonJsonProvider` + `JacksonMappingProvider` +
`Option.SUPPRESS_EXCEPTIONS`), including:
+/// - a missing key, an out-of-range index, indexing a non-array, descending
into a scalar, and an explicit
+/// JSON `null` all resolve to `null`;
+/// - duplicate object keys resolve to the **last** occurrence, and a later
occurrence discards whatever an
+/// earlier one resolved to, at every depth;
+/// - a container leaf is returned as a `LinkedHashMap` / `ArrayList` with the
same element types;
+/// - numbers become `Integer` / `Long` / `BigInteger` by magnitude, and
`Double` (or `BigDecimal` when
+/// `useBigDecimal` is set) for floating-point literals;
+/// - a document that is malformed anywhere inside its root value raises
[InvalidJsonException], even when the
+/// addressed field was already seen; content *after* the root value is
ignored, exactly as
+/// `ObjectMapper.readValue` ignores it (`FAIL_ON_TRAILING_TOKENS` is off by
default).
+///
+/// **Early exit.** When `earlyExit` is set (from
[JsonPathFastPathMode#isEarlyExitEnabled()]), the pass stops
+/// as soon as every requested path has resolved to a non-null value. That is
much faster when the target
+/// fields appear early in the document, at the cost of two behavior changes,
since the tail is never read:
+/// 1. duplicate keys resolve to the **first** occurrence rather than the last;
+/// 2. a document malformed strictly after the last addressed field yields the
extracted value instead of
+/// raising [InvalidJsonException].
+/// Both inputs are undefined (RFC 8259) or corrupt, and the switch is off by
default.
+///
+/// **Applicability.** Callers must first check [#canExtract]: the document's
first non-whitespace character
+/// has to be `{` or `[`. Bare scalar documents, `null`, empty /
whitespace-only input, a leading byte-order
+/// mark and plain text all take the Jayway path, which reproduces its exact
exceptions for them.
+///
+/// Stateless and thread-safe; the shared [JsonFactory] and [ObjectMapper]s
are safe for concurrent use.
+public final class StreamingJsonPathExtractor {
+ /// The live-path set is tracked as a bitmask, so a single call cannot
resolve more paths than a `long` has
+ /// bits. Callers with more paths must batch them.
+ public static final int MAX_PATHS = Long.SIZE;
+
+ /// Deliberately the plain, unconfigured factory and mappers that Jayway's
`JacksonJsonProvider` builds, so
+ /// that stream-read constraints (nesting depth, string length) and number
handling match exactly.
+ private static final JsonFactory FACTORY = new JsonFactory();
+ private static final ObjectMapper MAPPER = new ObjectMapper();
+ private static final ObjectMapper MAPPER_WITH_BIG_DECIMAL =
+ new
ObjectMapper().configure(DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS,
true);
+
+ private StreamingJsonPathExtractor() {
+ }
+
+ /// Returns `true` when `json`'s first non-whitespace character starts a
JSON object or array, which is the
+ /// only shape this extractor handles. Everything else must go through
Jayway.
+ public static boolean canExtract(@Nullable String json) {
+ if (json == null) {
+ return false;
+ }
+ for (int i = 0, n = json.length(); i < n; i++) {
+ char c = json.charAt(i);
+ if (c == ' ' || c == '\t' || c == '\n' || c == '\r') {
+ continue;
+ }
+ return c == '{' || c == '[';
+ }
+ return false;
+ }
+
+ /// UTF-8 overload of [#canExtract(String)]. A byte-order mark is rejected,
matching the `String` overload.
+ public static boolean canExtract(@Nullable byte[] json) {
+ if (json == null) {
+ return false;
+ }
+ for (byte b : json) {
+ if (b == ' ' || b == '\t' || b == '\n' || b == '\r') {
+ continue;
+ }
+ return b == '{' || b == '[';
+ }
+ return false;
+ }
+
+ // ponytail: the two throwaway arrays cost a few ns against a ~900ns parse,
so the single-path case reuses the
+ // multi-path walk rather than duplicating it. Split out a dedicated
single-path walk only if a profile says so.
+ @Nullable
+ public static Object extract(String json, SimpleJsonPath path, boolean
useBigDecimal, boolean earlyExit) {
+ Object[] result = new Object[1];
+ try (JsonParser parser = FACTORY.createParser(json)) {
+ extract(parser, new SimpleJsonPath[]{path}, result, useBigDecimal,
earlyExit);
+ } catch (IOException e) {
+ throw new InvalidJsonException(e);
+ }
+ return result[0];
+ }
+
+ @Nullable
+ public static Object extract(byte[] json, SimpleJsonPath path, boolean
useBigDecimal, boolean earlyExit) {
+ Object[] result = new Object[1];
+ try (JsonParser parser = FACTORY.createParser(json)) {
+ extract(parser, new SimpleJsonPath[]{path}, result, useBigDecimal,
earlyExit);
+ } catch (IOException e) {
+ throw new InvalidJsonException(e);
+ }
+ return result[0];
+ }
+
+ /// Resolves every path in `paths` in one pass, writing path `i`'s value
into `out[i]`. `out` must be at
+ /// least as long as `paths` and is fully overwritten for the first
`paths.length` entries.
+ public static void extract(String json, SimpleJsonPath[] paths, Object[]
out, boolean useBigDecimal,
+ boolean earlyExit) {
+ try (JsonParser parser = FACTORY.createParser(json)) {
+ extract(parser, paths, out, useBigDecimal, earlyExit);
+ } catch (IOException e) {
+ throw new InvalidJsonException(e);
+ }
+ }
Review Comment:
Added `Preconditions.checkArgument(out.length >= numPaths, ...)` next to the
existing path-count check, so it fails fast with a clear message instead of an
AIOOBE later.
_π€ Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-common/src/test/java/org/apache/pinot/common/function/JsonFunctionsFastPathTest.java:
##########
@@ -0,0 +1,43 @@
+/**
+ * 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 org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+
+
+/**
+ * Re-runs every {@link JsonFunctionsTest} case with the streaming JsonPath
fast path switched on. The fast path
+ * is required to be indistinguishable from Jayway, so the inherited
assertions must hold unchanged.
+ * <p>
+ * {@link StreamingJsonPathExtractorTest} is the exhaustive differential; this
class guarantees the canonical
+ * function tests keep covering the fast path as they grow.
+ */
+public class JsonFunctionsFastPathTest extends JsonFunctionsTest {
+
+ @BeforeClass
+ public void enableFastPath() {
+ JsonPathFastPathMode.setEnabled(true);
+ }
+
+ @AfterClass
+ public void disableFastPath() {
+ JsonPathFastPathMode.setEnabled(false);
+ }
Review Comment:
Fixed β both `@BeforeClass` and `@AfterClass` now also set
`setEarlyExitEnabled(false)`. Worth flagging: early exit is not merely cosmetic
here β it legitimately changes results for duplicate keys and malformed tails,
so a leaked `true` would have failed the inherited assertions for an unrelated
reason.
_π€ Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionFastPathTest.java:
##########
@@ -0,0 +1,47 @@
+/**
+ * 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.core.operator.transform.function;
+
+import org.apache.pinot.common.function.JsonPathFastPathMode;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+
+
+/**
+ * Re-runs every {@link JsonExtractScalarTransformFunctionTest} case with the
streaming JsonPath fast path
+ * switched on. The fast path is required to be indistinguishable from Jayway,
so the inherited assertions must
+ * hold unchanged - including the {@code USE_BIG_DECIMAL_FOR_FLOATS} contexts
used for STRING and BIG_DECIMAL
+ * results.
+ * <p>
+ * The inherited cases only feed STRING / JSON columns. The {@code byte[]}
overloads behind the BYTES-column
+ * branch of {@code getResultExtractor} are covered by {@code
StreamingJsonPathExtractorTest}, which drives them
+ * against Jayway's {@code parseUtf8} over the same corpus.
+ */
+public class JsonExtractScalarTransformFunctionFastPathTest extends
JsonExtractScalarTransformFunctionTest {
+
+ @BeforeClass
+ public void enableFastPath() {
+ JsonPathFastPathMode.setEnabled(true);
+ }
+
+ @AfterClass
+ public void disableFastPath() {
+ JsonPathFastPathMode.setEnabled(false);
+ }
Review Comment:
Fixed β same as `JsonFunctionsFastPathTest`: both flags are now pinned in
`@BeforeClass` and reset in `@AfterClass`.
_π€ Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-common/src/main/java/org/apache/pinot/common/function/StreamingJsonPathExtractor.java:
##########
@@ -0,0 +1,303 @@
+/**
+ * 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.fasterxml.jackson.core.JsonFactory;
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.core.JsonToken;
+import com.fasterxml.jackson.databind.DeserializationFeature;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.base.Preconditions;
+import com.jayway.jsonpath.InvalidJsonException;
+import java.io.IOException;
+import java.util.List;
+import java.util.Map;
+import javax.annotation.Nullable;
+
+
+/// Resolves one or more [SimpleJsonPath]s against a JSON document in a single
forward pass of a Jackson
+/// [JsonParser], materializing only the addressed values. Subtrees that no
path descends into are consumed
+/// with `skipChildren()`, so no intermediate `Map` / `List` is ever built for
them. This replaces
+/// `JsonPath.parse(json).read(path)`, which builds a full Jackson DOM of the
document and only then walks to
+/// the field.
Review Comment:
Not changed, deliberately: `///` (JEP 467) is this project's convention for
new/edited files per its CLAUDE.md, and neighbouring code already uses it (e.g.
the class-level doc on `JsonExtractScalarTransformFunction`). Switching these
three classes to `/** */` would make them the odd ones out. Happy to convert if
the project decides to standardise the other way.
_π€ Addressed by [Claude Code](https://claude.com/claude-code)_
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
