Support profile options for PPL - Part I Implement phases level metrics. #4983
Conversation
Signed-off-by: Peng Huo <[email protected]>
Signed-off-by: Peng Huo <[email protected]>
Signed-off-by: Peng Huo <[email protected]>
📝 WalkthroughWalkthroughAdds per-query profiling: thread-local profile management, metric types and implementations, instrumentation across planning, execution, formatting, propagation through PPL request/transport and async scanners, and inclusion of profiling output in JSON responses. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Factory as PPLQueryRequestFactory
participant Transport as TransportPPLQueryAction
participant Service as QueryService
participant Calcite as CalcitePlanner
participant ES as OpenSearchExecutionEngine
participant Formatter as SimpleJsonResponseFormatter
participant Profiling as QueryProfiling (thread-local)
Client->>Factory: submit PPL request (profile=true, format=jdbc)
Factory->>Profiling: activate(true)
Factory->>Transport: create transport request (profile flag)
Transport->>Service: dispatch request (wrapped listener)
Service->>Profiling: current()
Service->>Calcite: analyze/optimize plan
Calcite-->>Service: plan (Service updates ANALYZE/OPTIMIZE metrics)
Service->>ES: execute plan
ES-->>Service: results (ES updates EXECUTE metric)
Service->>Formatter: format response
Formatter->>Profiling: current().finish()
Formatter-->>Client: JSON response + profile
Transport->>Profiling: clear() (after listener completes)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Peng Huo <[email protected]>
Signed-off-by: Peng Huo <[email protected]>
penghuo
requested review from
GumpacG,
LantaoJin,
MaxKsyunz,
Swiddis,
YANG-DB,
Yury-Fridlyand,
acarbonetto,
anirudha,
dai-chen,
derek-ho,
forestmvey,
joshuali925,
kavithacm,
mengweieric,
ps48,
seankao-az,
vamsimanohar and
ykmr1224
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In
@integ-test/src/yamlRestTest/resources/rest-api-spec/test/api/ppl.profile.yml:
- Around line 52-54: The test is missing assertions for optimization and
formatting profiling metrics; update the ppl.profile.yml assertions to also
check gt: {profile.metrics.OPTIMIZE_TIME_MS: 0.0} and gt:
{profile.metrics.FORMAT_TIME_MS: 0.0} (mirroring the existing ANALYZE_TIME_MS
and OPENSEARCH_TIME_MS checks) so the response validates OPTIMIZE_TIME_MS and
FORMAT_TIME_MS from the profile output (MetricName OPTIMIZE_TIME and
FORMAT_TIME).
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.java (1)
174-177: Apply profiling context propagation consistently.The async call on line 176 doesn't capture and propagate the profiling context like
startScanningdoes (lines 107-111). This creates inconsistent profiling behavior between the initial scan and subsequent batch pre-fetches.🔎 Proposed fix to add profiling context propagation
// Pre-fetch next batch if needed if (!stopIteration && isAsync()) { + ProfileContext ctx = QueryProfiling.current(); nextBatchFuture = - CompletableFuture.supplyAsync(() -> client.search(request), backgroundExecutor); + CompletableFuture.supplyAsync( + () -> QueryProfiling.withCurrentContext(ctx, () -> client.search(request)), + backgroundExecutor); }
🧹 Nitpick comments (6)
core/src/main/java/org/opensearch/sql/monitor/profile/NoopProfileContext.java (1)
24-28: Consider documenting the null return semantics forfinish().Returning
nullfromfinish()indicates no profiling data, but callers must handle this. Consider adding a brief JavaDoc explaining thatnullis the expected return for the no-op context, or returning an emptyQueryProfileto avoid null checks.core/src/main/java/org/opensearch/sql/monitor/profile/QueryProfile.java (1)
35-43: Consider making the metrics map truly immutable.The
metricsfield is described as "Immutable" in the JavaDoc, butLinkedHashMapis mutable. Callers could modify the returned map viagetMetrics().🔎 Suggested fix to wrap with unmodifiableMap
private Map<String, Double> buildMetrics(Map<MetricName, Double> metrics) { Objects.requireNonNull(metrics, "metrics"); Map<String, Double> ordered = new java.util.LinkedHashMap<>(metrics.size()); for (MetricName metricName : MetricName.values()) { Double value = metrics.getOrDefault(metricName, 0d); ordered.put(metricName.name() + "_MS", value); } - return ordered; + return java.util.Collections.unmodifiableMap(ordered); }core/src/main/java/org/opensearch/sql/monitor/profile/DefaultProfileContext.java (1)
13-22: Add JavaDoc to the public class and constructor.Per coding guidelines, all public classes and methods should have proper JavaDoc. The class-level comment is minimal, and the public constructor lacks documentation.
Suggested JavaDoc additions
-/** Default implementation that records profiling metrics. */ +/** + * Default implementation of {@link ProfileContext} that records profiling metrics. + * + * <p>This context tracks timing metrics for query execution stages. It is thread-safe + * for metric recording but should only be finalized once via {@link #finish()}. + */ public class DefaultProfileContext implements ProfileContext { private final long startNanos = System.nanoTime(); private long endNanos; private boolean finished; private final Map<MetricName, DefaultMetricImpl> metrics = new ConcurrentHashMap<>(); private QueryProfile profile; + /** Creates a new profiling context, capturing the start time. */ public DefaultProfileContext() {}core/src/main/java/org/opensearch/sql/monitor/profile/DefaultMetricImpl.java (1)
40-44: Potential race condition inset()method.The
reset()followed byadd()is not atomic. Ifset()is called concurrently withadd()or anotherset(), the result may be incorrect. Ifset()is only called in single-threaded finalization contexts, this is acceptable; otherwise, consider usingAtomicLongwithset()or document the single-threaded constraint.Could you confirm that
set()is only invoked from single-threaded contexts? If concurrent access is possible,AtomicLongwould provide atomicset()while still supportingaddAndGet().core/src/main/java/org/opensearch/sql/monitor/profile/QueryProfiling.java (2)
46-53: Consider logging when overwriting an existing context.
activate()silently overwrites any existing context. For debugging purposes, you might want to log a warning ifCURRENT.get() != nullbefore overwriting, to catch unintended double-activation.
60-73: Consider restoring the previous context instead of clearing.
withCurrentContext()clears the context in thefinallyblock rather than restoring any previously active context. If profiling contexts could be nested (e.g., sub-queries), the outer context would be lost.If nested contexts are not a use case, the current implementation is fine.
Alternative: restore previous context
public static <T> T withCurrentContext(ProfileContext ctx, Supplier<T> action) { + ProfileContext previous = CURRENT.get(); CURRENT.set(Objects.requireNonNull(ctx, "ctx")); try { return action.get(); } finally { - clear(); + if (previous == null) { + CURRENT.remove(); + } else { + CURRENT.set(previous); + } } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
common/src/main/java/org/opensearch/sql/common/utils/QueryContext.javacore/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/monitor/profile/DefaultMetricImpl.javacore/src/main/java/org/opensearch/sql/monitor/profile/DefaultProfileContext.javacore/src/main/java/org/opensearch/sql/monitor/profile/MetricName.javacore/src/main/java/org/opensearch/sql/monitor/profile/NoopProfileContext.javacore/src/main/java/org/opensearch/sql/monitor/profile/NoopProfileMetric.javacore/src/main/java/org/opensearch/sql/monitor/profile/ProfileContext.javacore/src/main/java/org/opensearch/sql/monitor/profile/ProfileMetric.javacore/src/main/java/org/opensearch/sql/monitor/profile/QueryProfile.javacore/src/main/java/org/opensearch/sql/monitor/profile/QueryProfiling.javadocs/user/ppl/interfaces/endpoint.mdinteg-test/src/yamlRestTest/resources/rest-api-spec/api/ppl.jsoninteg-test/src/yamlRestTest/resources/rest-api-spec/test/api/ppl.profile.ymlopensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.javaopensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.javaopensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.javaplugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.javaplugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryAction.javaplugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryRequest.javappl/src/main/java/org/opensearch/sql/ppl/domain/PPLQueryRequest.javaprotocol/src/main/java/org/opensearch/sql/protocol/response/format/SimpleJsonResponseFormatter.java
🧰 Additional context used
📓 Path-based instructions (3)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
core/src/main/java/org/opensearch/sql/monitor/profile/ProfileContext.javaopensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.javacore/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.javappl/src/main/java/org/opensearch/sql/ppl/domain/PPLQueryRequest.javaplugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.javacommon/src/main/java/org/opensearch/sql/common/utils/QueryContext.javaplugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryRequest.javaopensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.javaplugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryAction.javacore/src/main/java/org/opensearch/sql/monitor/profile/ProfileMetric.javacore/src/main/java/org/opensearch/sql/monitor/profile/DefaultProfileContext.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javaopensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.javacore/src/main/java/org/opensearch/sql/monitor/profile/MetricName.javaprotocol/src/main/java/org/opensearch/sql/protocol/response/format/SimpleJsonResponseFormatter.javacore/src/main/java/org/opensearch/sql/monitor/profile/NoopProfileContext.javacore/src/main/java/org/opensearch/sql/monitor/profile/QueryProfile.javacore/src/main/java/org/opensearch/sql/monitor/profile/QueryProfiling.javacore/src/main/java/org/opensearch/sql/monitor/profile/DefaultMetricImpl.javacore/src/main/java/org/opensearch/sql/monitor/profile/NoopProfileMetric.java
⚙️ CodeRabbit configuration file
**/*.java: - Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure methods are under 20 lines with single responsibility
- Verify proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
core/src/main/java/org/opensearch/sql/monitor/profile/ProfileContext.javaopensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.javacore/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.javappl/src/main/java/org/opensearch/sql/ppl/domain/PPLQueryRequest.javaplugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.javacommon/src/main/java/org/opensearch/sql/common/utils/QueryContext.javaplugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryRequest.javaopensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.javaplugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryAction.javacore/src/main/java/org/opensearch/sql/monitor/profile/ProfileMetric.javacore/src/main/java/org/opensearch/sql/monitor/profile/DefaultProfileContext.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javaopensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.javacore/src/main/java/org/opensearch/sql/monitor/profile/MetricName.javaprotocol/src/main/java/org/opensearch/sql/protocol/response/format/SimpleJsonResponseFormatter.javacore/src/main/java/org/opensearch/sql/monitor/profile/NoopProfileContext.javacore/src/main/java/org/opensearch/sql/monitor/profile/QueryProfile.javacore/src/main/java/org/opensearch/sql/monitor/profile/QueryProfiling.javacore/src/main/java/org/opensearch/sql/monitor/profile/DefaultMetricImpl.javacore/src/main/java/org/opensearch/sql/monitor/profile/NoopProfileMetric.java
**/calcite/**/*.java
⚙️ CodeRabbit configuration file
**/calcite/**/*.java: - Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor
- Verify SQL generation and optimization paths
- Document any Calcite-specific workarounds
- Test compatibility with Calcite version constraints
Files:
core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java
**/ppl/**/*.java
⚙️ CodeRabbit configuration file
**/ppl/**/*.java: - For PPL parser changes, verify grammar tests with positive/negative cases
- Check AST generation for new syntax
- Ensure corresponding AST builder classes are updated
- Validate edge cases and boundary conditions
Files:
ppl/src/main/java/org/opensearch/sql/ppl/domain/PPLQueryRequest.java
🧠 Learnings (3)
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.javacore/src/main/java/org/opensearch/sql/executor/QueryService.java
📚 Learning: 2025-12-29T05:32:03.491Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:03.491Z
Learning: For any custom Calcite RelNode class (e.g., ones that extend EnumerableLimitSort or other Calcite RelNode types), always override the copy method. If copy is not overridden, cloning/copy operations may downgrade the instance to the parent class type, losing the custom behavior. In your implementation, ensure copy returns a new instance of the concrete class with all relevant fields and traits preserved, mirroring the current instance state.
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*.java : Maintain Java 11 compatibility when possible for OpenSearch 2.x
Applied to files:
opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java
🧬 Code graph analysis (10)
opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.java (1)
core/src/main/java/org/opensearch/sql/monitor/profile/QueryProfiling.java (1)
QueryProfiling(17-74)
core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java (1)
core/src/main/java/org/opensearch/sql/monitor/profile/QueryProfiling.java (1)
QueryProfiling(17-74)
ppl/src/main/java/org/opensearch/sql/ppl/domain/PPLQueryRequest.java (1)
core/src/main/java/org/opensearch/sql/monitor/profile/QueryProfile.java (1)
Getter(14-44)
plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryRequest.java (1)
core/src/main/java/org/opensearch/sql/monitor/profile/QueryProfile.java (1)
Getter(14-44)
opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java (1)
core/src/main/java/org/opensearch/sql/monitor/profile/QueryProfiling.java (1)
QueryProfiling(17-74)
plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryAction.java (2)
core/src/main/java/org/opensearch/sql/monitor/profile/QueryProfiling.java (1)
QueryProfiling(17-74)common/src/main/java/org/opensearch/sql/common/utils/QueryContext.java (1)
QueryContext(18-87)
core/src/main/java/org/opensearch/sql/executor/QueryService.java (2)
common/src/main/java/org/opensearch/sql/common/utils/QueryContext.java (1)
QueryContext(18-87)core/src/main/java/org/opensearch/sql/monitor/profile/QueryProfiling.java (1)
QueryProfiling(17-74)
opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java (1)
core/src/main/java/org/opensearch/sql/monitor/profile/QueryProfiling.java (1)
QueryProfiling(17-74)
core/src/main/java/org/opensearch/sql/monitor/profile/NoopProfileContext.java (1)
core/src/main/java/org/opensearch/sql/monitor/profile/NoopProfileMetric.java (1)
NoopProfileMetric(9-34)
core/src/main/java/org/opensearch/sql/monitor/profile/QueryProfiling.java (1)
core/src/main/java/org/opensearch/sql/monitor/profile/NoopProfileContext.java (1)
NoopProfileContext(11-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
🔇 Additional comments (39)
integ-test/src/yamlRestTest/resources/rest-api-spec/test/api/ppl.profile.yml (3)
1-26: LGTM! Well-structured test setup.The setup correctly enables the Calcite plugin, creates a minimal test index, and populates it with sample data. Using a single shard with no replicas is appropriate for integration testing.
27-38: LGTM! Proper cleanup.The teardown appropriately mirrors the setup operations and uses
ignore_unavailableto handle cases where the index may not exist.
57-100: LGTM! Comprehensive negative test coverage.The negative test cases appropriately verify that profiling is disabled for:
- Explain API endpoint (where execution doesn't occur)
- Explain queries (embedded explain command)
- Viz format (where profiling data isn't meaningful)
These tests ensure profiling overhead is avoided in scenarios where it's not applicable.
integ-test/src/yamlRestTest/resources/rest-api-spec/api/ppl.json (1)
16-21: LGTM! Format parameter added correctly.The addition of the format parameter to the PPL API spec is well-defined with appropriate type and description.
opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.java (1)
107-111: LGTM! Profiling context properly propagated to async task.The profiling context is correctly captured and passed to the background search operation using
QueryProfiling.withCurrentContext.core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java (1)
345-368: LGTM! OPTIMIZE_TIME profiling correctly implemented.The profiling instrumentation properly captures timing around the
prepareStatementcall. The metric is set only when the operation succeeds, which is the correct behavior.opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java (1)
224-271: LGTM! POST_EXEC_TIME profiling correctly implemented.The profiling instrumentation properly captures timing for the result set building phase. The metric is recorded only on successful completion, which is the correct behavior.
docs/user/ppl/interfaces/endpoint.md (1)
155-190: Format constraint enforcement verified and properly implemented.The code correctly enforces the documented constraints on profiling. The
isProfileSupported()method at PPLQueryRequestFactory.java:125-131 validates all three requirements:
- Rejects explain endpoint paths
- Rejects queries starting with "explain"
- Requires format to be "jdbc" (DEFAULT_RESPONSE_FORMAT)
Profiling is only enabled when all constraints are satisfied, aligning perfectly with the documentation.
core/src/main/java/org/opensearch/sql/monitor/profile/ProfileContext.java (1)
1-24: LGTM! Clean profiling interface.The interface is well-documented with clear contracts for metric collection and profiling finalization. The method signatures are appropriately simple and focused.
core/src/main/java/org/opensearch/sql/monitor/profile/MetricName.java (1)
1-15: LGTM! Well-defined metric names.The enum clearly defines the profiling metrics used across the query lifecycle. Naming conventions are properly followed.
protocol/src/main/java/org/opensearch/sql/protocol/response/format/SimpleJsonResponseFormatter.java (3)
47-48: LGTM! Appropriate timing instrumentation.The FORMAT_TIME metric correctly captures the elapsed time for building the JSON response using
System.nanoTime()for precision.Also applies to: 57-57
76-77: LGTM! Profile field addition.Adding the
profilefield toJsonResponseappropriately exposes profiling data in the response payload.
59-59: The lifecycle is safe—finish()is idempotent.The
finish()method inDefaultProfileContextincludes a guard that prevents re-execution: it checks afinishedflag and returns the cached profile if already invoked. Additionally,buildJsonObject()is only called once per query through the parent class'sformat()method, ensuringfinish()is invoked exactly once per response.core/src/main/java/org/opensearch/sql/monitor/profile/ProfileMetric.java (1)
1-33: LGTM! Well-designed metric interface.The interface provides a clear contract for profiling metrics with both read and write operations. JavaDoc is complete and descriptive.
common/src/main/java/org/opensearch/sql/common/utils/QueryContext.java (1)
23-24: LGTM! Thread-safe profile flag management.The implementation correctly stores and retrieves the profiling flag in ThreadContext. The use of
Boolean.parseBoolean()safely handles the absence of the flag by defaulting tofalse. JavaDoc is clear and complete.Also applies to: 72-86
core/src/main/java/org/opensearch/sql/executor/QueryService.java (3)
43-43: LGTM! Clean import additions for profiling infrastructure.The profiling-related imports are well-organized and align with the new profiling framework.
Also applies to: 47-50
150-150: LGTM! No-op profiling for explain path is appropriate.The explain flow doesn't need actual profiling metrics, so setting a no-op context prevents NPEs when downstream code calls
QueryProfiling.current().
106-117: Profiling context cleanup is properly handled in production, but consider clarifying the guarantee.The profiling context activated at line 107 is correctly cleaned up via
TransportPPLQueryAction.wrapWithProfilingClear(), which wraps all response listeners with finally blocks that callQueryProfiling.clear(). This ensures cleanup occurs whether execution succeeds, fails, or falls back to legacy (line 122). SinceTransportPPLQueryActionis the sole production entry point for PPL queries, the profiling context cleanup is guaranteed.However, if
QueryService.executeWithCalcite()orexplainWithCalcite()are ever invoked from other code paths outside the transport layer (currently only in tests), cleanup would not be guaranteed. Consider documenting this assumption or adding cleanup withinQueryServiceitself as a defensive measure.core/src/main/java/org/opensearch/sql/monitor/profile/NoopProfileContext.java (1)
10-28: Solid no-op implementation with proper singleton pattern.The singleton pattern with private constructor and public INSTANCE is correctly implemented. Input validation on
getOrCreateMetricis good defensive practice.ppl/src/main/java/org/opensearch/sql/ppl/domain/PPLQueryRequest.java (2)
38-41: LGTM! Profile field follows existing patterns.The
profilefield correctly mirrors the style ofsanitizeandstylefields with fluent accessors. Default value offalseensures profiling is opt-in.
47-59: LGTM! Constructor chaining correctly propagates the new parameter.The constructor delegation pattern maintains backward compatibility while adding the new profile capability.
plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryAction.java (2)
112-123: LGTM! Profiling lifecycle management is well-structured.The profile flag is correctly propagated via
QueryContext.setProfile(), and the clearing listener ensures cleanup regardless of success/failure. Good separation between storing the intent (QueryContext) and activation (QueryService).
208-230: Clean profiling cleanup wrapper with proper try-finally semantics.The
wrapWithProfilingClearmethod correctly ensuresQueryProfiling.clear()is called in both success and failure paths. This prevents thread-local leaks in thread pools.One minor consideration: if
delegate.onResponse()ordelegate.onFailure()throws an exception, thefinallyblock still executes, which is correct behavior.opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java (3)
24-26: LGTM! Explicit imports are cleaner than wildcards.Good refactoring to replace the wildcard import with explicit imports for
SearchRequest,SearchResponse, andSearchScrollRequest.
212-246: LGTM! Correct timing instrumentation for non-PIT search.The
metric.set()call appropriately records the complete duration for a single-shot search. The timing captures the entire search execution including response processing.
258-302: Good use ofadd()for PIT-based pagination accumulation.Using
metric.add()instead ofset()correctly accumulates timing across multiple PIT search iterations during cursor-based pagination. This provides an accurate total forOPENSEARCH_TIMEeven when results span multiple batches.core/src/main/java/org/opensearch/sql/monitor/profile/NoopProfileMetric.java (1)
8-34: LGTM! Clean Null Object pattern implementation.The package-private visibility is appropriate, and the singleton with private constructor is correctly implemented. All methods safely do nothing or return neutral values, ensuring no side effects when profiling is disabled.
core/src/main/java/org/opensearch/sql/monitor/profile/QueryProfile.java (1)
13-33: LGTM! Well-structured immutable snapshot class.The use of
@SerializedNamefor JSON serialization andLinkedHashMapfor consistent metric ordering is appropriate. The design correctly defaults missing metrics to 0.plugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.java (2)
63-73: LGTM! Profile flag parsing and propagation looks correct.The logic correctly:
- Reads the optional
profileboolean from JSON payload- Validates profiling is only enabled when the path, format, and query support it
- Passes the computed
enableProfileto thePPLQueryRequestconstructor
125-131: Clean implementation of profile support validation.The method correctly restricts profiling to non-explain requests with JDBC format. The null checks are appropriately defensive.
core/src/main/java/org/opensearch/sql/monitor/profile/DefaultProfileContext.java (3)
24-29: LGTM!The
getOrCreateMetricimplementation is thread-safe usingConcurrentHashMap.computeIfAbsentand properly validates the input.
31-48: LGTM!The
finish()method correctly handles:
- Single finalization via synchronized and
finishedflag- Capturing end timestamp
- Building an ordered snapshot with defaults for missing metrics
- Returning cached profile on subsequent calls
50-52: LGTM!Standard pattern for converting nanoseconds to milliseconds with two-decimal precision.
core/src/main/java/org/opensearch/sql/monitor/profile/DefaultMetricImpl.java (1)
10-38: LGTM!The class is well-designed:
- Package-private visibility appropriately restricts access
LongAdderprovides excellent performance for concurrentadd()operations- Proper JavaDoc on the constructor
plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryRequest.java (3)
48-51: LGTM!The
profilefield follows the same pattern as other boolean fields (sanitize) with fluent accessors and a sensible default offalse.
137-144: LGTM!The
toPPLQueryRequest()method correctly propagates theprofileflag to the reconstructedPPLQueryRequest.
64-75: Serialization pattern is consistent across fields; verify rolling upgrade strategy.The
profilefield is added to serialization without version checks, but this matches the pattern used for thestylefield added previously. If rolling upgrades are supported, both fields would need versioned serialization. However, the codebase does not currently use version-aware serialization in the transport layer. Clarify whether:
- Rolling upgrades requiring serialization compatibility are actually supported
- Version checks are necessary for this field (and should have been used for
styleas well)If version-aware serialization is required, both
styleandprofileneed to be wrapped with version checks using a similar pattern.core/src/main/java/org/opensearch/sql/monitor/profile/QueryProfiling.java (2)
17-29: Well-designed thread-local profiling holder.Good use of:
- Private constructor to enforce static-only access
- Null-object pattern returning
NoopProfileContext.INSTANCEinstead of nullThreadLocalfor per-thread isolation
55-58: LGTM!Proper use of
ThreadLocal.remove()to prevent memory leaks, especially important when threads are pooled.
integ-test/src/yamlRestTest/resources/rest-api-spec/test/api/ppl.profile.yml
Outdated
Show resolved
Hide resolved
Signed-off-by: Peng Huo <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
Fix all issues with AI Agents 🤖
In
@core/src/main/java/org/opensearch/sql/monitor/profile/DefaultProfileContext.java:
- Around line 23-28: The public method
DefaultProfileContext.getOrCreateMetric(MetricName name) lacks a comprehensive
JavaDoc; add a JavaDoc block above the method that describes its behavior
(returns an existing ProfileMetric for the given MetricName or creates and
stores a new DefaultMetricImpl using key.name()), documents the parameter (name)
and return value (ProfileMetric), notes that it throws NullPointerException if
name is null, and describes thread-safety (uses metrics.computeIfAbsent for
atomic create-if-absent semantics and is safe for concurrent access to the
metrics map).
- Around line 13-21: Replace the single-line comment above DefaultProfileContext
with a proper JavaDoc block for the public class DefaultProfileContext: describe
the class purpose (records profiling metrics for a query), outline thread-safety
guarantees (e.g., internally uses ConcurrentHashMap and is safe for concurrent
metric updates; indicate which methods/fields are safe or not), summarize
typical usage (how to create, record metrics, and finish the profile to obtain
QueryProfile), and include any relevant tags/notes (e.g., @see QueryProfile,
@since, and a brief example snippet reference). Ensure the JavaDoc sits
immediately above the class declaration and covers purpose, thread-safety, and
usage.
- Around line 30-47: The public synchronized method finish() in
DefaultProfileContext lacks JavaDoc; add a comprehensive JavaDoc block above the
finish() method describing its purpose (marking the profile as finished and
returning a QueryProfile snapshot), its thread-safety (synchronized) and
idempotence (returns existing profile if already finished), the return value
(QueryProfile containing totalMillis and per-metric snapshot), and any important
side effects (sets finished flag, computes end time from System.nanoTime(),
rounds metrics). Mention thrown exceptions if any (none) and any concurrency
notes so callers understand semantics.
🧹 Nitpick comments (1)
core/src/main/java/org/opensearch/sql/monitor/profile/DefaultProfileContext.java (1)
39-39: Consider cachingMetricName.values()for marginal optimization.The
MetricName.values()call creates a new array on each invocation. While the performance impact is negligible (finish() is called once per query), caching the array as a static final field would eliminate the allocation.🔎 Optional optimization
+ private static final MetricName[] ALL_METRICS = MetricName.values(); + ... @Override public synchronized QueryProfile finish() { ... - for (MetricName metricName : MetricName.values()) { + for (MetricName metricName : ALL_METRICS) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/main/java/org/opensearch/sql/monitor/profile/DefaultProfileContext.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
core/src/main/java/org/opensearch/sql/monitor/profile/DefaultProfileContext.java
⚙️ CodeRabbit configuration file
**/*.java: - Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure methods are under 20 lines with single responsibility
- Verify proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
core/src/main/java/org/opensearch/sql/monitor/profile/DefaultProfileContext.java
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: For PPL command PRs, refer docs/dev/ppl-commands.md and verify the PR satisfies the checklist
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (25, integration)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (21, unit)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (25, doc)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: security-it-linux (21)
- GitHub Check: security-it-linux (25)
- GitHub Check: test-sql-cli-integration (21)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: Update draft release notes
core/src/main/java/org/opensearch/sql/monitor/profile/DefaultProfileContext.java
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/monitor/profile/DefaultProfileContext.java
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/monitor/profile/DefaultProfileContext.java
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/monitor/profile/DefaultMetricImpl.java
Show resolved
Hide resolved
opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java
Show resolved
Hide resolved
Signed-off-by: Peng Huo <[email protected]>
| searchDone = true; | ||
| } | ||
| needClean = searchDone; | ||
| metric.set(System.nanoTime() - engineStartTime); |
There was a problem hiding this comment.
Should use add here since we support pagination for aggregate and this code will be called multiplet-times?
| package org.opensearch.sql.monitor.profile; | ||
|
|
||
| /** Named metrics used by query profiling. */ | ||
| public enum MetricName { |
There was a problem hiding this comment.
Do we need PARSE_TIME, though it should be tiny?
There was a problem hiding this comment.
I ignore it for now, should be minial.
qianheng-aws
left a comment
qianheng-aws
left a comment
There was a problem hiding this comment.
@penghuo Is this profile metric safe for parallelism updating? For OPENSEARCH_TIME, it may be updated by multiple thread if there is join or union.
| final class DefaultMetricImpl implements ProfileMetric { | ||
|
|
||
| private final String name; | ||
| private long value = 0; |
There was a problem hiding this comment.
Should be AtomicLong? We have multi-thread usage in opensearch_time
docs/user/ppl/interfaces/endpoint.md
Outdated
| "ANALYZE_TIME_MS": 5.77, | ||
| "OPTIMIZE_TIME_MS": 13.51, | ||
| "OPENSEARCH_TIME_MS": 4.31, | ||
| "POST_EXEC_TIME_MS": 0.77, | ||
| "FORMAT_TIME_MS": 0.04 |
There was a problem hiding this comment.
I'd like make all the metrics lower case which to align with OpenSearch DSL profile
| */ | ||
| public final class QueryProfiling { | ||
|
|
||
| private static final ThreadLocal<ProfileContext> CURRENT = new ThreadLocal<>(); |
There was a problem hiding this comment.
We search with multiple threads on JOIN/Subsearch queries. We the total time should sum them.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @docs/user/ppl/interfaces/endpoint.md:
- Around line 155-191: Replace the inconsistent code-fence marker "```bash ppl
ignore" in the Profile example with "```bash ppl" so it matches other curl
examples; update the opening fence string (search for the literal "```bash ppl
ignore" in the Profile (Experimental) example) to remove the "ignore" token
while leaving the surrounding curl block unchanged.
🧹 Nitpick comments (3)
docs/user/ppl/interfaces/endpoint.md (1)
172-185: Consider documenting what each metric represents.The expected output shows five timing metrics but doesn't explain what each one measures (e.g., what stages do analyze, optimize, opensearch, post_exec, and format steps correspond to). Adding brief descriptions would help users understand where time is spent and diagnose performance bottlenecks.
Since this is marked "Experimental," this can be deferred if metric definitions are documented in a user manual or if the metric names are self-evident to your target audience.
core/src/main/java/org/opensearch/sql/monitor/profile/DefaultMetricImpl.java (2)
21-23: Consider adding null validation for defensive coding.The constructor accepts a
nameparameter without validation. Adding a null check would prevent potential NPEs if the caller inadvertently passes null.🛡️ Proposed defensive validation
DefaultMetricImpl(String name) { + if (name == null) { + throw new IllegalArgumentException("Metric name cannot be null"); + } this.name = name; }
25-33: Add JavaDoc to public methods for better documentation.The coding guidelines require JavaDoc on all public methods. While these methods implement the
ProfileMetricinterface (which likely has documentation), adding brief JavaDoc here improves discoverability and maintains consistency with project standards.📝 Example JavaDoc additions
+ /** + * Returns the name of this metric. + * + * @return metric name + */ @Override public String name() { return name; } + /** + * Returns the current value of this metric. + * + * @return sum of all increments + */ @Override public long value() { return value.sum(); }As per coding guidelines, all public methods require proper JavaDoc.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/src/main/java/org/opensearch/sql/monitor/profile/DefaultMetricImpl.javacore/src/main/java/org/opensearch/sql/monitor/profile/QueryProfile.javadocs/user/ppl/interfaces/endpoint.mdinteg-test/src/yamlRestTest/resources/rest-api-spec/test/api/ppl.profile.ymlopensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.javaopensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.java
🚧 Files skipped from review as they are similar to previous changes (3)
- opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java
- core/src/main/java/org/opensearch/sql/monitor/profile/QueryProfile.java
- integ-test/src/yamlRestTest/resources/rest-api-spec/test/api/ppl.profile.yml
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.javacore/src/main/java/org/opensearch/sql/monitor/profile/DefaultMetricImpl.java
⚙️ CodeRabbit configuration file
**/*.java: - Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure methods are under 20 lines with single responsibility
- Verify proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.javacore/src/main/java/org/opensearch/sql/monitor/profile/DefaultMetricImpl.java
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: For PPL command PRs, refer docs/dev/ppl-commands.md and verify the PR satisfies the checklist
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Development requires JDK 21 for the OpenSearch SQL project
Applied to files:
core/src/main/java/org/opensearch/sql/monitor/profile/DefaultMetricImpl.java
🧬 Code graph analysis (1)
opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.java (1)
core/src/main/java/org/opensearch/sql/monitor/profile/QueryProfiling.java (1)
QueryProfiling(17-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (21, unit)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: build-linux (25, integration)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (25, doc)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: security-it-linux (21)
- GitHub Check: security-it-linux (25)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: test-sql-cli-integration (21)
🔇 Additional comments (5)
core/src/main/java/org/opensearch/sql/monitor/profile/DefaultMetricImpl.java (2)
10-14: Excellent choice of LongAdder for concurrent metrics.The use of
LongAdderis well-suited for profiling metrics that may be updated concurrently from multiple threads. It provides better performance thanAtomicLongunder contention by maintaining a set of variables internally.
40-44: Correct implementation of set() for LongAdder.The
set()method correctly usesreset()followed byadd()to establish a new value. This is the proper pattern sinceLongAdderdoesn't provide a directset()operation.opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.java (3)
20-21: LGTM!The imports are correctly added and necessary for the profiling context propagation functionality.
107-111: Profiling context correctly propagated to async task.The implementation properly captures the profiling context from the calling thread and propagates it to the background executor. The context capture timing is correct (before the async task is submitted), and
withCurrentContextensures the context is set on the background thread, used during execution, and cleaned up afterward.
175-179: Profiling context consistently propagated for batch pre-fetching.The profiling context propagation follows the same correct pattern as
startScanning. The context is captured when the next batch pre-fetch is initiated, ensuring profiling metrics are collected for all async background searches.
docs/user/ppl/interfaces/endpoint.md
Outdated
| ## Profile (Experimental) | ||
| You can enable profiling on the PPL endpoint to capture per-stage timings in milliseconds. Profiling is returned only for regular query execution (not explain) and only when using the default `format=jdbc`. | ||
|
|
||
| ### Example | ||
|
|
||
| ```bash ppl ignore | ||
| curl -sS -H 'Content-Type: application/json' \ | ||
| -X POST localhost:9200/_plugins/_ppl \ | ||
| -d '{ | ||
| "profile": true, | ||
| "query" : "source=accounts | fields firstname, lastname" | ||
| }' | ||
| ``` | ||
|
|
||
| Expected output (trimmed): | ||
|
|
||
| ```json | ||
| { | ||
| "profile": { | ||
| "total_ms": 25.77, | ||
| "metrics": { | ||
| "analyze_time_ms": 5.77, | ||
| "optimize_time_ms": 13.51, | ||
| "opensearch_time_ms": 4.31, | ||
| "post_exec_time_ms": 0.77, | ||
| "format_time_ms": 0.04 | ||
| } | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ### Notes | ||
|
|
||
| - Profile output is only returned when the query finishes successfully. | ||
| - Profiling runs only when Calcite is enabled. | ||
| - For parallel work (for example, joins), `opensearch_time_ms` is cumulative across requests and can exceed `total_ms`. |
There was a problem hiding this comment.
Remove inconsistent ignore keyword from code fence marker.
Line 161's code fence uses bash ppl ignore while all other curl examples in the file use only bash ppl (see lines 13, 69, 91, 112, 136). The ignore keyword appears to be an artifact and should be removed for consistency.
📝 Proposed fix
- ```bash ppl ignore
+ ```bash ppl
curl -sS -H 'Content-Type: application/json' \🤖 Prompt for AI Agents
In @docs/user/ppl/interfaces/endpoint.md around lines 155 - 191, Replace the
inconsistent code-fence marker "```bash ppl ignore" in the Profile example with
"```bash ppl" so it matches other curl examples; update the opening fence string
(search for the literal "```bash ppl ignore" in the Profile (Experimental)
example) to remove the "ignore" token while leaving the surrounding curl block
unchanged.
Signed-off-by: Peng Huo <[email protected]>
penghuo
force-pushed
the
issues/4294-v1
branch
from
662e582 to
7ff2bb2
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @core/src/main/java/org/opensearch/sql/monitor/profile/QueryProfile.java:
- Around line 23-32: The Javadoc for QueryProfile(QueryProfile) claims
totalTimeMillis is "rounded to two decimals" but the constructor currently
passes totalTimeMillis directly into new Summary(totalTimeMillis) without
rounding; update the constructor to round totalTimeMillis to two decimal places
(e.g., via BigDecimal setScale(2, RoundingMode.HALF_UP) or
Math.round(totalTimeMillis * 100.0) / 100.0) before creating Summary, and ensure
any other uses in buildPhases(...) or Summary(...) that rely on that value use
the rounded value so the documented contract is enforced.
- Around line 15-42: QueryProfile claims to be an immutable snapshot but stores
a mutable Map and doesn't defensively handle null metric values; update the
constructor and buildPhases so they validate not-null inputs, replace null
metric values with 0.0, create a new LinkedHashMap of String->Phase using
MetricName.values() order, wrap it with Collections.unmodifiableMap before
assigning to the final phases field, and ensure Summary and Phase are
constructed with immutable/primitive data (or defensively copied) so that
QueryProfile, its summary, and the phases map are truly immutable and safe to
expose via the generated getters.
In
@opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.java:
- Around line 105-112: The initial background search in startScanning captures
and propagates the profiling context but fetchNextBatch still schedules its
prefetch without QueryProfiling.withCurrentContext, so later async batches don't
carry profiling info; fix by capturing the current ProfileContext
(QueryProfiling.current()) before scheduling and wrap the
CompletableFuture.supplyAsync call in fetchNextBatch with
QueryProfiling.withCurrentContext(ctx, () -> client.search(request)) (use the
same pattern used in startScanning), ensuring nextBatchFuture is created with
backgroundExecutor so every async batch includes the profiling context.
🧹 Nitpick comments (5)
integ-test/src/yamlRestTest/resources/rest-api-spec/test/api/ppl.profile.yml (1)
40-57: Test coverage for positive profiling case looks good.The test correctly verifies that profiling metrics are captured for all four phases (analyze, optimize, execute, format) plus the total time, using appropriate
gtassertions for timing values. This aligns with the PR's objective to capture per-stage profiling metrics.Optional: Consider adding assertions to verify profile structure
You could add assertions to verify that the profile structure is complete and that phase times don't exceed total time, though this may be overkill for an integration test:
# Example additional assertions - is_true: profile.summary - is_true: profile.phases - lte: {profile.phases.analyze.time_ms: $profile.summary.total_time_ms}However, the current assertions are sufficient for validating the profiling functionality.
opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java (1)
218-272: Record timing in afinallyso failures don’t silently drop EXECUTE metrics.Proposed fix
private void buildResultSet( ResultSet resultSet, RelDataType rowTypes, Integer querySizeLimit, ResponseListener<QueryResponse> listener) throws SQLException { - ProfileMetric metric = QueryProfiling.current().getOrCreateMetric(MetricName.EXECUTE); - long execTime = System.nanoTime(); + ProfileMetric metric = QueryProfiling.current().getOrCreateMetric(MetricName.EXECUTE); + long execTime = System.nanoTime(); + try { // Get the ResultSet metadata to know about columns ResultSetMetaData metaData = resultSet.getMetaData(); @@ Schema schema = new Schema(columns); QueryResponse response = new QueryResponse(schema, values, null); - metric.add(System.nanoTime() - execTime); - listener.onResponse(response); + listener.onResponse(response); + } finally { + metric.add(System.nanoTime() - execTime); + } }opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java (2)
205-249: Ensure EXECUTE metric is recorded even whensearchAction.apply()fails.Proposed fix
} else { ProfileMetric metric = QueryProfiling.current().getOrCreateMetric(MetricName.EXECUTE); long executionStartTime = System.nanoTime(); - // Set afterKey to request, null for first round (afterKey is null in the beginning). - if (this.sourceBuilder.aggregations() != null) { - this.sourceBuilder.aggregations().getAggregatorFactories().stream() - .filter(a -> a instanceof CompositeAggregationBuilder) - .forEach(c -> ((CompositeAggregationBuilder) c).aggregateAfter(afterKey)); - if (LOG.isDebugEnabled()) { - LOG.debug(sourceBuilder); - } - } - - SearchRequest searchRequest = - new SearchRequest().indices(indexName.getIndexNames()).source(this.sourceBuilder); - this.searchResponse = searchAction.apply(searchRequest); - - openSearchResponse = - new OpenSearchResponse( - this.searchResponse, exprValueFactory, includes, isCountAggRequest()); - - // Update afterKey from response - if (openSearchResponse.isAggregationResponse()) { - openSearchResponse.getAggregations().asList().stream() - .filter(a -> a instanceof InternalComposite) - .forEach(c -> afterKey = ((InternalComposite) c).afterKey()); - } - if (afterKey != null) { - // For composite aggregation, searchDone is determined by response result. - searchDone = openSearchResponse.isEmpty(); - } else { - // Direct set searchDone to true for non-composite aggregation. - searchDone = true; - } - needClean = searchDone; - metric.add(System.nanoTime() - executionStartTime); + try { + // Set afterKey to request, null for first round (afterKey is null in the beginning). + if (this.sourceBuilder.aggregations() != null) { + this.sourceBuilder.aggregations().getAggregatorFactories().stream() + .filter(a -> a instanceof CompositeAggregationBuilder) + .forEach(c -> ((CompositeAggregationBuilder) c).aggregateAfter(afterKey)); + if (LOG.isDebugEnabled()) { + LOG.debug(sourceBuilder); + } + } + + SearchRequest searchRequest = + new SearchRequest().indices(indexName.getIndexNames()).source(this.sourceBuilder); + this.searchResponse = searchAction.apply(searchRequest); + + openSearchResponse = + new OpenSearchResponse( + this.searchResponse, exprValueFactory, includes, isCountAggRequest()); + + // Update afterKey from response + if (openSearchResponse.isAggregationResponse()) { + openSearchResponse.getAggregations().asList().stream() + .filter(a -> a instanceof InternalComposite) + .forEach(c -> afterKey = ((InternalComposite) c).afterKey()); + } + if (afterKey != null) { + // For composite aggregation, searchDone is determined by response result. + searchDone = openSearchResponse.isEmpty(); + } else { + // Direct set searchDone to true for non-composite aggregation. + searchDone = true; + } + needClean = searchDone; + } finally { + metric.add(System.nanoTime() - executionStartTime); + } }
251-305: Same: PIT path should record timing infinally.protocol/src/main/java/org/opensearch/sql/protocol/response/format/SimpleJsonResponseFormatter.java (1)
46-60: Cache the profiling context to avoid redundant thread-local lookups.The code correctly handles the profiling lifecycle—
finish()is idempotent and safe to call multiple times. However, the two calls toQueryProfiling.current()(lines 47 and 59) can be consolidated for clarity:@Override public Object buildJsonObject(QueryResult response) { - ProfileMetric formatMetric = QueryProfiling.current().getOrCreateMetric(MetricName.FORMAT); + var ctx = QueryProfiling.current(); + ProfileMetric formatMetric = ctx.getOrCreateMetric(MetricName.FORMAT); long formatTime = System.nanoTime(); JsonResponse.JsonResponseBuilder json = JsonResponse.builder(); json.total(response.size()).size(response.size()); response.columnNameTypes().forEach((name, type) -> json.column(new Column(name, type))); json.datarows(fetchDataRows(response)); formatMetric.set(System.nanoTime() - formatTime); - json.profile(QueryProfiling.current().finish()); + json.profile(ctx.finish()); return json.build(); }The try-finally wrapper is unnecessary since
finish()does not throw exceptions and the timing logic withset()is correct for single measurement per invocation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/monitor/profile/MetricName.javacore/src/main/java/org/opensearch/sql/monitor/profile/QueryProfile.javadocs/user/ppl/interfaces/endpoint.mdinteg-test/src/yamlRestTest/resources/rest-api-spec/test/api/ppl.profile.ymlopensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.javaopensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.javaopensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.javaprotocol/src/main/java/org/opensearch/sql/protocol/response/format/SimpleJsonResponseFormatter.java
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/main/java/org/opensearch/sql/executor/QueryService.java
- docs/user/ppl/interfaces/endpoint.md
🧰 Additional context used
📓 Path-based instructions (2)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.javacore/src/main/java/org/opensearch/sql/monitor/profile/MetricName.javacore/src/main/java/org/opensearch/sql/monitor/profile/QueryProfile.javaprotocol/src/main/java/org/opensearch/sql/protocol/response/format/SimpleJsonResponseFormatter.javaopensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.javaopensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.javacore/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java
⚙️ CodeRabbit configuration file
**/*.java: - Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure methods are under 20 lines with single responsibility
- Verify proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.javacore/src/main/java/org/opensearch/sql/monitor/profile/MetricName.javacore/src/main/java/org/opensearch/sql/monitor/profile/QueryProfile.javaprotocol/src/main/java/org/opensearch/sql/protocol/response/format/SimpleJsonResponseFormatter.javaopensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.javaopensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.javacore/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java
**/calcite/**/*.java
⚙️ CodeRabbit configuration file
**/calcite/**/*.java: - Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor
- Verify SQL generation and optimization paths
- Document any Calcite-specific workarounds
- Test compatibility with Calcite version constraints
Files:
core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: For PPL command PRs, refer docs/dev/ppl-commands.md and verify the PR satisfies the checklist
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*.java : Maintain Java 11 compatibility when possible for OpenSearch 2.x
Applied to files:
opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.javainteg-test/src/yamlRestTest/resources/rest-api-spec/test/api/ppl.profile.yml
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*.java : Document Calcite-specific workarounds in code
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java
📚 Learning: 2025-12-29T05:32:03.491Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:03.491Z
Learning: For any custom Calcite RelNode class (e.g., ones that extend EnumerableLimitSort or other Calcite RelNode types), always override the copy method. If copy is not overridden, cloning/copy operations may downgrade the instance to the parent class type, losing the custom behavior. In your implementation, ensure copy returns a new instance of the concrete class with all relevant fields and traits preserved, mirroring the current instance state.
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL
Applied to files:
integ-test/src/yamlRestTest/resources/rest-api-spec/test/api/ppl.profile.yml
🧬 Code graph analysis (5)
opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java (1)
core/src/main/java/org/opensearch/sql/monitor/profile/QueryProfiling.java (1)
QueryProfiling(17-74)
protocol/src/main/java/org/opensearch/sql/protocol/response/format/SimpleJsonResponseFormatter.java (1)
core/src/main/java/org/opensearch/sql/monitor/profile/QueryProfiling.java (1)
QueryProfiling(17-74)
opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java (1)
core/src/main/java/org/opensearch/sql/monitor/profile/QueryProfiling.java (1)
QueryProfiling(17-74)
opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.java (1)
core/src/main/java/org/opensearch/sql/monitor/profile/QueryProfiling.java (1)
QueryProfiling(17-74)
core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java (1)
core/src/main/java/org/opensearch/sql/monitor/profile/QueryProfiling.java (1)
QueryProfiling(17-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: security-it-linux (25)
- GitHub Check: security-it-linux (21)
- GitHub Check: build-linux (21, unit)
- GitHub Check: build-linux (25, doc)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (25, integration)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: test-sql-cli-integration (21)
- GitHub Check: CodeQL-Scan (java)
🔇 Additional comments (8)
core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java (2)
31-31: LGTM: Profiling imports are correctly added.The static import of
OPTIMIZEand the regular imports for profiling classes are appropriate for the instrumentation added below.Also applies to: 95-96
346-347: LGTM: Clean profiling instrumentation for the OPTIMIZE phase.The changes correctly measure the time taken to prepare the Calcite statement:
- Uses
System.nanoTime()for precise timing- Updates the metric only after successful preparation (failures are excluded from profiling, which is appropriate)
- Preserves the original control flow and exception handling
- Non-invasive implementation with minimal overhead
Based on learnings: Test that the profiling instrumentation doesn't introduce measurable overhead in the SQL generation and optimization paths.
Also applies to: 366-368
integ-test/src/yamlRestTest/resources/rest-api-spec/test/api/ppl.profile.yml (3)
1-26: Setup section looks well-structured.The setup correctly enables the Calcite plugin, creates a minimal test index with appropriate settings (1 shard, 0 replicas for performance), and indexes test data with immediate refresh. This provides a clean foundation for the profiling tests.
27-38: Proper cleanup with defensive handling.The teardown correctly reverses the setup changes and uses
ignore_unavailable: truefor defensive cleanup. This ensures tests don't leave artifacts or modified cluster settings.
59-102: Comprehensive negative test coverage.The three test cases properly verify that profiling is ignored for explain API, explain queries, and viz format, aligning with the PR's objective to enforce profiling only for supported formats and request paths. The assertions correctly check for
profile: nullin each scenario.Based on the PR objectives, these tests validate the enforcement of supported formats and request paths for profiling.
core/src/main/java/org/opensearch/sql/monitor/profile/MetricName.java (1)
8-14: LGTM: clear shared phase keys for profiling.protocol/src/main/java/org/opensearch/sql/protocol/response/format/SimpleJsonResponseFormatter.java (1)
73-85: Addingprofileto the response payload looks good.opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.java (1)
107-111: Thread-safety concern is unfounded; ProfileContext is designed to be safely shared across threads.DefaultProfileContext explicitly uses
ConcurrentHashMapfor the metrics map,LongAdderfor thread-safe metric values (which support concurrent increments), and asynchronizedfinish()method to safely snapshot state. ThegetOrCreateMetric()method, though unsynchronized, is safe becauseConcurrentHashMap.computeIfAbsent()is atomic. Multiple threads can concurrently add to metrics and callfinish()without race conditions or corruption.Likely an incorrect or invalid review comment.
...search/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.java
Show resolved
Hide resolved
Signed-off-by: Peng Huo <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @core/src/main/java/org/opensearch/sql/executor/QueryService.java:
- Line 150: The code in QueryService currently calls QueryProfiling.noop() but
never clears the thread-local ProfileContext; wrap the profiling setup so that
QueryProfiling.clear() is invoked in a finally block (e.g., around the method or
the try that contains QueryProfiling.noop()) to ensure the thread-local is
removed even on exceptions; update the method in QueryService where
QueryProfiling.noop() is called to call QueryProfiling.clear() in a finally
clause to prevent leaks in pooled threads.
- Around line 106-117: The activated ProfileContext (created by
QueryProfiling.activate(...) and stored in profileContext) must be cleared to
avoid ThreadLocal leaks: wrap the profiling lifecycle (from ProfileContext
profileContext = QueryProfiling.activate(...) through using analyzeMetric and
any subsequent profiling calls like analyzeMetric.set(...)) in a try-finally and
call QueryProfiling.clear() in the finally block so the ThreadLocal is removed
even on exceptions; update the method in QueryService that contains this snippet
to perform the activate/use/clear pattern.
🧹 Nitpick comments (4)
core/src/main/java/org/opensearch/sql/monitor/profile/DefaultMetricImpl.java (2)
25-33: Consider adding JavaDoc to public methods.While the methods are straightforward, the coding guidelines require JavaDoc on public methods. Consider adding brief documentation for
name()andvalue().As per coding guidelines, public methods should have JavaDoc.
📝 Suggested JavaDoc additions
+ /** @return the name of this metric */ @Override public String name() { return name; } + /** @return the current accumulated value in nanoseconds */ @Override public long value() { return value.sum(); }
35-44: Consider adding JavaDoc to public methods.Similar to the getters, these mutation methods would benefit from JavaDoc documentation per coding guidelines.
As per coding guidelines, public methods should have JavaDoc.
📝 Suggested JavaDoc additions
+ /** Add a delta to the current metric value. Thread-safe for concurrent calls. + * @param delta the value to add (in nanoseconds) + */ @Override public void add(long delta) { value.add(delta); } + /** Set the metric to a specific value by resetting and adding. + * @param value the new value (in nanoseconds) + */ @Override public void set(long value) { this.value.reset(); this.value.add(value); }core/src/main/java/org/opensearch/sql/monitor/profile/QueryProfile.java (2)
15-21: Consider wrapping the phases map in an unmodifiable wrapper for stronger immutability guarantees.The
phasesfield is final and initialized with a newLinkedHashMap, but the Lombok@Getterexposes it directly. Callers could mutate the map viaqueryProfile.getPhases().put(...). Consider wrapping it withCollections.unmodifiableMap()in the constructor or getter.🔒 Suggested immutability improvement
Option 1 - Wrap in constructor:
public QueryProfile(double totalTimeMillis, Map<MetricName, Double> phases) { this.summary = new Summary(totalTimeMillis); - this.phases = buildPhases(phases); + this.phases = Collections.unmodifiableMap(buildPhases(phases)); }Option 2 - Use
Map.copyOf()for immutable map:public QueryProfile(double totalTimeMillis, Map<MetricName, Double> phases) { this.summary = new Summary(totalTimeMillis); - this.phases = buildPhases(phases); + this.phases = Map.copyOf(buildPhases(phases)); }
44-64: Consider adding JavaDoc to public inner classes.The
SummaryandPhaseclasses are well-designed with appropriate visibility and immutability. However, coding guidelines require JavaDoc on public classes. Consider adding brief class-level documentation explaining their role in the profiling output.As per coding guidelines, public classes should have JavaDoc.
📝 Suggested JavaDoc additions
@Getter + /** Overall timing summary for the profiled query. */ public static final class Summary { @SerializedName("total_time_ms") private final double totalTimeMillis; private Summary(double totalTimeMillis) { this.totalTimeMillis = totalTimeMillis; } } @Getter + /** Timing information for a single execution phase. */ public static final class Phase { @SerializedName("time_ms") private final double timeMillis; private Phase(double timeMillis) { this.timeMillis = timeMillis; } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/monitor/profile/DefaultMetricImpl.javacore/src/main/java/org/opensearch/sql/monitor/profile/MetricName.javacore/src/main/java/org/opensearch/sql/monitor/profile/QueryProfile.javadocs/user/ppl/interfaces/endpoint.mdinteg-test/src/yamlRestTest/resources/rest-api-spec/test/api/ppl.profile.ymlopensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.javaopensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.javaprotocol/src/main/java/org/opensearch/sql/protocol/response/format/SimpleJsonResponseFormatter.java
🚧 Files skipped from review as they are similar to previous changes (3)
- core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java
- opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java
- protocol/src/main/java/org/opensearch/sql/protocol/response/format/SimpleJsonResponseFormatter.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
core/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/monitor/profile/MetricName.javacore/src/main/java/org/opensearch/sql/monitor/profile/DefaultMetricImpl.javacore/src/main/java/org/opensearch/sql/monitor/profile/QueryProfile.javaopensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java
⚙️ CodeRabbit configuration file
**/*.java: - Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure methods are under 20 lines with single responsibility
- Verify proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
core/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/monitor/profile/MetricName.javacore/src/main/java/org/opensearch/sql/monitor/profile/DefaultMetricImpl.javacore/src/main/java/org/opensearch/sql/monitor/profile/QueryProfile.javaopensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java
🧠 Learnings (3)
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
integ-test/src/yamlRestTest/resources/rest-api-spec/test/api/ppl.profile.ymlcore/src/main/java/org/opensearch/sql/executor/QueryService.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL
Applied to files:
integ-test/src/yamlRestTest/resources/rest-api-spec/test/api/ppl.profile.yml
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Development requires JDK 21 for the OpenSearch SQL project
Applied to files:
core/src/main/java/org/opensearch/sql/monitor/profile/DefaultMetricImpl.java
🧬 Code graph analysis (2)
core/src/main/java/org/opensearch/sql/executor/QueryService.java (2)
common/src/main/java/org/opensearch/sql/common/utils/QueryContext.java (1)
QueryContext(18-87)core/src/main/java/org/opensearch/sql/monitor/profile/QueryProfiling.java (1)
QueryProfiling(17-74)
opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java (1)
core/src/main/java/org/opensearch/sql/monitor/profile/QueryProfiling.java (1)
QueryProfiling(17-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (24)
- GitHub Check: security-it-linux (21)
- GitHub Check: security-it-linux (25)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (21, unit)
- GitHub Check: build-linux (25, doc)
- GitHub Check: build-linux (21, doc)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: build-linux (25, integration)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: test-sql-cli-integration (21)
- GitHub Check: CodeQL-Scan (java)
🔇 Additional comments (12)
integ-test/src/yamlRestTest/resources/rest-api-spec/test/api/ppl.profile.yml (6)
1-26: LGTM! Well-structured test setup.The setup block properly enables the Calcite plugin, creates a minimal test index with appropriate settings for integration testing (single shard, no replicas), and indexes sample data with refresh enabled.
28-37: LGTM! Proper cleanup with error handling.The teardown correctly removes the test index with
ignore_unavailable: trueto prevent failures if the index doesn't exist, and properly disables the Calcite plugin.
40-56: LGTM! Comprehensive profiling metrics validation.The test appropriately validates that profiling returns non-zero timing metrics across all phases (analyze, optimize, execute, format) and the total summary. Using
gt: 0.0assertions is correct for non-deterministic timing values.
59-71: LGTM! Correctly validates profiling is disabled for explain API.The test appropriately verifies that the
profileflag is ignored when using theppl.explainendpoint, ensuring profiling is only enabled for supported request paths.
74-86: LGTM! Validates profiling is disabled for explain queries.The test correctly verifies that profiling is ignored when using the
explainkeyword in the query itself (as opposed to the explain API endpoint), ensuring consistent behavior across different explain invocation methods.
89-102: Theformatparameter at line 101 is uniquely placed and inconsistent with all other PPL endpoint calls in the test suite.All other ppl endpoint usages across the entire test suite only include a
bodyparameter at this level. The format parameter as a sibling to body at line 101 is the only such occurrence, suggesting it should be verified against the actual PPL API endpoint specification to confirm whether format is a valid query parameter or if it should be included inside the body.docs/user/ppl/interfaces/endpoint.md (1)
155-191: LGTM! Clear and well-structured profiling documentation.The new Profile section effectively documents the experimental profiling feature with a clear example and appropriate caveats. The JSON output structure aligns with the QueryProfile implementation.
Minor observation: Line 161 uses
bash ppl ignoreas the code fence language identifier, which is unconventional but may be intentional for documentation tooling.opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java (2)
54-56: LGTM! Appropriate profiling imports.The new imports for profiling infrastructure are necessary and correctly scoped.
224-225: LGTM! Correct profiling instrumentation.The execution timing is properly captured using
System.nanoTime()before ResultSet processing and recorded after QueryResponse construction. The use ofMetricName.EXECUTEaccurately represents this phase.Also applies to: 270-270
core/src/main/java/org/opensearch/sql/monitor/profile/MetricName.java (1)
1-14: LGTM! Clean enum design.The
MetricNameenum clearly defines the four profiling phases (ANALYZE, OPTIMIZE, EXECUTE, FORMAT) with proper naming conventions and JavaDoc.core/src/main/java/org/opensearch/sql/monitor/profile/DefaultMetricImpl.java (1)
11-23: LGTM! Solid class structure with appropriate thread-safety primitives.The package-private final class uses
LongAdderfor thread-safe metric accumulation, and the constructor is properly documented.core/src/main/java/org/opensearch/sql/monitor/profile/QueryProfile.java (1)
23-42: LGTM! Solid constructor and phase-building logic.The constructor properly initializes the snapshot with appropriate null checking. The
buildPhasesmethod ensures all metric phases are present in a consistent order, defaulting missing values to 0.0, and correctly usesLocale.ROOTfor case conversion.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/user/ppl/interfaces/endpoint.md (1)
155-191: Enhance clarity on endpoint and format constraints.The section correctly documents key constraints (no explain, default format=jdbc, Calcite enabled), but could be clearer about endpoint specificity. Consider explicitly stating: "Profiling is supported on the
/_plugins/_pplendpoint only (not/_plugins/_ppl/_explain), with the defaultformat=jdbc." This eliminates ambiguity for users. Additionally, consider documenting the behavior if users attempt to enable profiling with a non-default format (silent ignored, error, etc.).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/user/ppl/interfaces/endpoint.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (25, integration)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (21, unit)
- GitHub Check: build-linux (25, doc)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: security-it-linux (21)
- GitHub Check: security-it-linux (25)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: test-sql-cli-integration (21)
🔇 Additional comments (2)
docs/user/ppl/interfaces/endpoint.md (2)
161-161: Clarify theignoreflag in the code block.The bash code block uses
ignoreas a language modifier:\``bash ppl ignore`. This is non-standard syntax. Please confirm whether this is intentional (e.g., to skip example validation/execution), and if so, consider clarifying this convention in the documentation or using a more standard approach.
155-191: Documentation structure and examples are well-presented.The Profile section follows the established pattern (description → example → notes), includes a clear curl command, shows realistic output, and documents important constraints. The "Experimental" label and the note about Calcite dependency are helpful. The trimmed output example appropriately signals that full responses include schema/datarows in addition to profiling metrics.
Agree. I sepereate into 2 PRs. |
|
The backport to To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/sql/backport-2.19-dev 2.19-dev
# Navigate to the new working tree
pushd ../.worktrees/sql/backport-2.19-dev
# Create a new branch
git switch --create backport/backport-4983-to-2.19-dev
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 47709a05afa0419fccd4c37e6ec5cc8066dd8292
# Push it to GitHub
git push --set-upstream origin backport/backport-4983-to-2.19-dev
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/sql/backport-2.19-devThen, create a pull request where the |
…cs. (opensearch-project#4983) * Init Signed-off-by: Peng Huo <[email protected]> * Cleanup ThreadLocal Signed-off-by: Peng Huo <[email protected]> * Update doc Signed-off-by: Peng Huo <[email protected]> * Update Doc Signed-off-by: Peng Huo <[email protected]> * Refactor Code Signed-off-by: Peng Huo <[email protected]> * Remove unused code Signed-off-by: Peng Huo <[email protected]> * Address comments Signed-off-by: Peng Huo <[email protected]> * Add Task 1 - Add Phases level metrics Signed-off-by: Peng Huo <[email protected]> * Reformat doc Signed-off-by: Peng Huo <[email protected]> --------- Signed-off-by: Peng Huo <[email protected]> (cherry picked from commit 47709a0)
…cs. (opensearch-project#4983) * Init Signed-off-by: Peng Huo <[email protected]> * Cleanup ThreadLocal Signed-off-by: Peng Huo <[email protected]> * Update doc Signed-off-by: Peng Huo <[email protected]> * Update Doc Signed-off-by: Peng Huo <[email protected]> * Refactor Code Signed-off-by: Peng Huo <[email protected]> * Remove unused code Signed-off-by: Peng Huo <[email protected]> * Address comments Signed-off-by: Peng Huo <[email protected]> * Add Task 1 - Add Phases level metrics Signed-off-by: Peng Huo <[email protected]> * Reformat doc Signed-off-by: Peng Huo <[email protected]> --------- Signed-off-by: Peng Huo <[email protected]> (cherry picked from commit 47709a0)
…cs. (opensearch-project#4983) * Init Signed-off-by: Peng Huo <[email protected]> * Cleanup ThreadLocal Signed-off-by: Peng Huo <[email protected]> * Update doc Signed-off-by: Peng Huo <[email protected]> * Update Doc Signed-off-by: Peng Huo <[email protected]> * Refactor Code Signed-off-by: Peng Huo <[email protected]> * Remove unused code Signed-off-by: Peng Huo <[email protected]> * Address comments Signed-off-by: Peng Huo <[email protected]> * Add Task 1 - Add Phases level metrics Signed-off-by: Peng Huo <[email protected]> * Reformat doc Signed-off-by: Peng Huo <[email protected]> --------- Signed-off-by: Peng Huo <[email protected]> (cherry picked from commit 47709a0)
4 participants
Description
profileflag through transport and request parsing, enforce supported formats/paths, and document profiling usage.Related Issues
#4294
Check List
--signoffor-s.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.