Copilot commented on code in PR #18972:
URL: https://github.com/apache/pinot/pull/18972#discussion_r3577338400
##########
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:
`SimpleJsonPath.compile(jsonPath)` will throw if `jsonPath` is null. With
the fast path enabled, that can change behavior compared to the Jayway fallback
path. If null paths are possible for this scalar function, guard here (return
null and let the existing Jayway call path handle/throw consistently).
##########
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:
The log says the early-exit flag is being ignored when the main flag is
disabled, but the code still applies it via
`JsonPathFastPathMode.setEarlyExitEnabled(earlyExitEnabled)`. Either (a) change
the warning to reflect reality (e.g., ‘early exit enabled but has no effect
unless fast path is enabled’) or (b) actually ignore it by forcing
`earlyExitEnabled=false` when `enabled` is false.
##########
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:
The method documents that `out` must be at least as long as `paths`, but
doesn’t validate it. As a public API, it’s better to fail fast with a clear
`IllegalArgumentException` (e.g. via `Preconditions.checkArgument(out.length >=
paths.length, ...)`) rather than an `ArrayIndexOutOfBoundsException` later.
##########
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:
Same global-state concern as `JsonFunctionsFastPathTest`: this class only
toggles `_enabled`. To make the suite robust to test ordering, explicitly reset
`JsonPathFastPathMode.setEarlyExitEnabled(false)` as well.
##########
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:
This test mutates global static state but only resets `_enabled`. To avoid
order-dependent failures/flakes (e.g., if another test left early-exit
enabled), also explicitly set `JsonPathFastPathMode.setEarlyExitEnabled(false)`
in both setup/teardown (or at least teardown).
##########
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:
These are `//`-style comments (triple-slash is still a line comment in
Java), so they won’t be picked up by Javadoc tooling and the `[...]` link
syntax isn’t Javadoc. Since this is a new public utility class, consider
converting the class-level doc to a proper `/** ... */` Javadoc and using
`{@link SimpleJsonPath}` / `{@link JsonParser}` etc. so the documentation is
generated and links resolve.
--
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]
