xiangfu0 commented on code in PR #18972:
URL: https://github.com/apache/pinot/pull/18972#discussion_r3592219159
##########
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);
Review Comment:
Yes — done. The whole node-config fast-path branch here was removed (see the
top-level comment); `jsonPath` is back to pure Jayway. The new opt-in streaming
overloads route through a helper that now wraps the streaming call in try/catch
and falls back to the Jayway implementation on any failure, so an unforeseen
streaming bug can only cost a second parse on that row, never change the
result. Malformed JSON lands there too, and Jayway then raises the same
exception.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-common/src/main/java/org/apache/pinot/common/function/JsonPathFastPathMode.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.function;
+
+import org.apache.pinot.spi.utils.CommonConstants;
+
+
+/// Global runtime switches for the streaming JsonPath fast path (see
[StreamingJsonPathExtractor]).
+///
+/// Both switches default to `false` and are seeded from the corresponding JVM
system property, so tests and
+/// JMH benchmarks can drive them without a running server. Server / broker /
controller / minion startup
+/// overrides them from cluster config via
`ServiceStartableUtils.applyClusterConfig`.
+///
+/// - [#isEnabled()] gates the fast path entirely. When off, every extraction
goes through Jayway.
+/// - [#isEarlyExitEnabled()] additionally lets the extractor stop at the leaf
instead of reading the whole
+/// root value. It trades two documented behavior changes for a large
speedup on documents whose target
+/// field appears early; see [StreamingJsonPathExtractor] for the exact
divergences. It has no effect
+/// unless [#isEnabled()] is also true.
+///
+/// Thread-safe: both fields are `volatile` and only ever flipped at startup
(or from a test).
+public class JsonPathFastPathMode {
+ private static volatile boolean _enabled =
Boolean.getBoolean(CommonConstants.CONFIG_OF_JSONPATH_FASTPATH_ENABLED);
+ private static volatile boolean _earlyExitEnabled =
+
Boolean.getBoolean(CommonConstants.CONFIG_OF_JSONPATH_FASTPATH_EARLY_EXIT_ENABLED);
+
+ private JsonPathFastPathMode() {
Review Comment:
Removed — `JsonPathFastPathMode` is deleted entirely in the redesign (no
more node-level config), so there is no longer a util class here to reorder.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-common/src/main/java/org/apache/pinot/common/function/JsonPathFastPathMode.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.function;
+
+import org.apache.pinot.spi.utils.CommonConstants;
+
+
+/// Global runtime switches for the streaming JsonPath fast path (see
[StreamingJsonPathExtractor]).
+///
+/// Both switches default to `false` and are seeded from the corresponding JVM
system property, so tests and
+/// JMH benchmarks can drive them without a running server. Server / broker /
controller / minion startup
+/// overrides them from cluster config via
`ServiceStartableUtils.applyClusterConfig`.
+///
+/// - [#isEnabled()] gates the fast path entirely. When off, every extraction
goes through Jayway.
+/// - [#isEarlyExitEnabled()] additionally lets the extractor stop at the leaf
instead of reading the whole
+/// root value. It trades two documented behavior changes for a large
speedup on documents whose target
+/// field appears early; see [StreamingJsonPathExtractor] for the exact
divergences. It has no effect
+/// unless [#isEnabled()] is also true.
+///
+/// Thread-safe: both fields are `volatile` and only ever flipped at startup
(or from a test).
+public class JsonPathFastPathMode {
+ private static volatile boolean _enabled =
Boolean.getBoolean(CommonConstants.CONFIG_OF_JSONPATH_FASTPATH_ENABLED);
Review Comment:
Agreed, and removed — the redesign drops the node-level config (and the
`Boolean.getBoolean` system-property seed) completely. The optimization is now
opt-in per expression via new scalar-function overloads instead, so there is
nothing to configure at the node level.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/JsonFunctions.java:
##########
@@ -140,13 +158,12 @@ private static boolean canExtractJsonPath(@Nullable
Object object) {
/**
* Extract object array based on Json path
*/
+ // Routed through jsonPath() rather than PARSE_CONTEXT directly: the two are
equivalent, and this way the
Review Comment:
Fixed — `jsonPathArray` is reverted to its original master form, so the
stray comment between the javadoc and the method is gone.
_🤖 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]
