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: 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)
…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]> Signed-off-by: Aaron Alvarez <[email protected]>
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.