Support profile option for PPL - Part II Implement operator level metrics #5044
Conversation
…rics Signed-off-by: Peng Huo <[email protected]>
📝 WalkthroughWalkthroughThis pull request introduces plan-node profiling capabilities to the query execution framework. It adds a Changes
Sequence DiagramsequenceDiagram
participant CalciteTool as CalciteToolsHelper
participant PlanBuilder as PlanProfileBuilder
participant RelNode as Wrapped RelNode Tree
participant Enumerable as ProfileEnumerableRel
participant Enumerator as ProfileEnumerator
participant Metrics as ProfilePlanNodeMetrics
CalciteTool->>PlanBuilder: profile(root RelNode)
PlanBuilder->>PlanBuilder: traverse & wrap RelNodes recursively
PlanBuilder->>RelNode: wrap with ProfileEnumerableRel/ProfileScannableRel
PlanBuilder-->>CalciteTool: return ProfilePlan (wrapped rel + plan root)
CalciteTool->>RelNode: execute query
RelNode->>Enumerable: produce Enumerable
Enumerable->>Enumerable: wrapBlock to inject profiling
Note over Enumerable: if metrics present:
Enumerable->>Enumerator: create with timing/row tracking
Enumerator->>Metrics: addTimeNanos() on each move
Enumerator->>Metrics: incrementRows() on successful iteration
Enumerator-->>Enumerable: return results
Enumerable-->>RelNode: return profiled Enumerable
RelNode-->>CalciteTool: execution complete
CalciteTool->>Metrics: snapshot() to build QueryProfile.PlanNode
Metrics-->>CalciteTool: hierarchical plan with metrics
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
penghuo
added
PPL
penghuo
requested review from
GumpacG,
LantaoJin,
MaxKsyunz,
RyanL1997,
Swiddis,
YANG-DB,
Yury-Fridlyand,
acarbonetto,
anirudha,
dai-chen,
derek-ho,
forestmvey,
joshuali925,
kavithacm,
mengweieric,
noCharger,
ps48,
qianheng-aws,
seankao-az,
vamsimanohar,
ykmr1224 and
yuancu
as code owners
|
@dai-chen @LantaoJin @qianheng-aws Please review code. Is it follow up of #4983. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/calcite/profile/ProfileScannableRel.java:
- Around line 17-30: ProfileScannableRel is missing an override of the copy(...)
method so copy operations can recreate a ProfileScannableRel (and retain the
Scannable behavior) instead of falling back to ProfileEnumerableRel; add a
public ProfileScannableRel copy(RelTraitSet traitSet, List<RelNode> inputs)
override that calls the superclass constructor with the incoming delegate (cast
to EnumerableRel), passes inputs and planNode, and sets the scannable field by
casting the delegate to Scannable so the new instance preserves the Scannable
implementation and metrics.
🧹 Nitpick comments (13)
integ-test/src/yamlRestTest/resources/rest-api-spec/test/api/ppl.profile.yml (1)
57-58: LGTM! New plan profiling assertions are well-aligned with the test setup.The assertions correctly verify that:
- Plan execution time is captured (
time_ms > 0)- Row count matches the 2 documents inserted in setup
Consider also asserting on
profile.plan.nodeto verify the operator name is returned (e.g., usingis_trueormatch), which would provide additional coverage for the new plan structure described in the PR objectives.core/src/main/java/org/opensearch/sql/monitor/profile/NoopProfileContext.java (1)
17-20: Consider adding{@inheritDoc}for consistency.The existing methods
getOrCreateMetricandfinishuse/** {@inheritDoc} */comments, but these new override methods omit them. For consistency with the existing code pattern in this file, consider adding the same annotation.♻️ Suggested fix
+ /** {@inheritDoc} */ @Override public boolean isEnabled() { return false; }+ /** {@inheritDoc} */ @Override public void setPlanRoot(ProfilePlanNode planRoot) {}Also applies to: 29-30
core/src/test/java/org/opensearch/sql/calcite/profile/ProfileEnumerableRelTest.java (2)
18-42: Good test coverage for core profile behavior.The tests properly verify the key contract: null metrics returns the original enumerable unchanged, and non-null metrics correctly tracks rows and time.
Consider adding an edge case test for an empty enumerable (size=0) to verify that
rows()returns 0 andtimeNanos()is still recorded for the enumeration setup/teardown.
62-66: Consider the timing sensitivity of sleep-based tests.Using
Thread.sleep(1)makes the test timing-dependent. While this is necessary to ensure measurable time passes, in high-load CI environments, the 1ms sleep might occasionally be too short to reliably measure due to timer granularity.The current approach is acceptable since you only assert
timeNanos() > 0, which is a low bar. Just be aware this could be a source of flakiness if the assertion becomes more specific.core/src/main/java/org/opensearch/sql/monitor/profile/ProfilePlanNodeMetrics.java (2)
10-11: Add JavaDoc for public methods.Per coding guidelines, public methods should have proper JavaDoc with
@param,@return, and@throwsannotations. Consider adding brief documentation foraddTimeNanos,incrementRows,timeNanos(), androws().
18-22: Silent discard of non-positive time values.The
if (nanos > 0)check silently ignores zero and negative values. While negative nanosecond deltas should be rare (clock drift/adjustment), silently discarding them could make debugging timing anomalies harder.Consider logging a warning for negative values, or document this behavior in the method's JavaDoc if the silent discard is intentional.
core/src/main/java/org/opensearch/sql/monitor/profile/ProfilePlanNode.java (1)
12-13: Add JavaDoc for public class and methods.Per coding guidelines, public classes and methods should have proper JavaDoc. The class-level comment is brief; consider expanding it and adding documentation for the constructor,
metrics(), andsnapshot()methods.core/src/main/java/org/opensearch/sql/calcite/profile/PlanProfileBuilder.java (1)
17-18: Add JavaDoc for public API.Per coding guidelines, public classes and methods should have proper JavaDoc. The
profile(RelNode)method is the main entry point and should document its purpose, parameters, and return value.Proposed JavaDoc
/** Builds a profiled EnumerableRel plan tree and matching plan node structure. */ public final class PlanProfileBuilder { private PlanProfileBuilder() {} + /** + * Wraps a RelNode tree with profiling instrumentation. + * + * @param root the root of the plan tree to profile + * @return a ProfilePlan containing the wrapped tree and its corresponding plan node hierarchy + */ public static ProfilePlan profile(RelNode root) {Also applies to: 22-25
core/src/main/java/org/opensearch/sql/monitor/profile/DefaultProfileContext.java (1)
24-41: Add JavaDoc for new public methods.Per coding guidelines, public methods should have JavaDoc with
@param,@return, and@throws. TheisEnabled()andsetPlanRoot()methods are missing documentation.📝 Suggested documentation
+ /** {@inheritDoc} */ @Override public boolean isEnabled() { return true; } + /** + * Set the plan root for profiling. Only the first call takes effect. + * + * @param planRoot the root of the profiled plan tree + */ @Override public synchronized void setPlanRoot(ProfilePlanNode planRoot) { if (this.planRoot == null) { this.planRoot = planRoot; } }core/src/test/java/org/opensearch/sql/calcite/profile/PlanProfileBuilderTest.java (1)
89-99: Consider adding edge case tests.The current tests cover the happy path well. Consider adding tests for:
- Input list with multiple children to verify all inputs are wrapped
- Nested tree structure to verify recursive profiling
core/src/main/java/org/opensearch/sql/calcite/profile/ProfileEnumerableRel.java (3)
48-56: Return defensive copy fromgetInputs()to prevent external modification.The method returns the internal mutable
inputslist directly. External callers could inadvertently modify this list, leading to inconsistent state. Consider returning an unmodifiable view.🔧 Suggested fix
@Override public List<RelNode> getInputs() { - return inputs; + return List.copyOf(inputs); }
38-46: Minor: Avoid redundant null check on delegate.
Objects.requireNonNull(delegate, "delegate")is called twice in the super constructor call.🔧 Suggested fix
public ProfileEnumerableRel( EnumerableRel delegate, List<RelNode> inputs, ProfilePlanNode planNode) { + Objects.requireNonNull(delegate, "delegate"); super( - Objects.requireNonNull(delegate, "delegate").getCluster(), - Objects.requireNonNull(delegate, "delegate").getTraitSet()); + delegate.getCluster(), + delegate.getTraitSet()); this.delegate = delegate; this.inputs = new ArrayList<>(Objects.requireNonNull(inputs, "inputs")); this.planNode = Objects.requireNonNull(planNode, "planNode"); }
138-152: Add JavaDoc for the publicprofile()method.Per coding guidelines, public methods should have JavaDoc with
@param,@return, and@throwsdocumentation.📝 Suggested documentation
+ /** + * Wrap an enumerable with profiling instrumentation. + * + * @param enumerable the source enumerable to profile + * @param metrics metrics collector, or null to skip profiling + * @param <T> element type + * @return profiled enumerable, or the original if metrics is null + */ public static <T> Enumerable<T> profile( Enumerable<T> enumerable, ProfilePlanNodeMetrics metrics) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
core/src/main/java/org/opensearch/sql/calcite/profile/PlanProfileBuilder.javacore/src/main/java/org/opensearch/sql/calcite/profile/ProfileEnumerableRel.javacore/src/main/java/org/opensearch/sql/calcite/profile/ProfileScannableRel.javacore/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.javacore/src/main/java/org/opensearch/sql/monitor/profile/DefaultProfileContext.javacore/src/main/java/org/opensearch/sql/monitor/profile/NoopProfileContext.javacore/src/main/java/org/opensearch/sql/monitor/profile/ProfileContext.javacore/src/main/java/org/opensearch/sql/monitor/profile/ProfilePlanNode.javacore/src/main/java/org/opensearch/sql/monitor/profile/ProfilePlanNodeMetrics.javacore/src/main/java/org/opensearch/sql/monitor/profile/ProfileUtils.javacore/src/main/java/org/opensearch/sql/monitor/profile/QueryProfile.javacore/src/test/java/org/opensearch/sql/calcite/profile/PlanProfileBuilderTest.javacore/src/test/java/org/opensearch/sql/calcite/profile/ProfileEnumerableRelTest.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.java
💤 Files with no reviewable changes (1)
- opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java
🧰 Additional context used
📓 Path-based instructions (5)
**/*.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/calcite/utils/CalciteToolsHelper.javacore/src/main/java/org/opensearch/sql/monitor/profile/ProfileUtils.javacore/src/main/java/org/opensearch/sql/calcite/profile/PlanProfileBuilder.javacore/src/main/java/org/opensearch/sql/monitor/profile/NoopProfileContext.javacore/src/main/java/org/opensearch/sql/calcite/profile/ProfileEnumerableRel.javacore/src/main/java/org/opensearch/sql/monitor/profile/ProfileContext.javacore/src/main/java/org/opensearch/sql/monitor/profile/ProfilePlanNodeMetrics.javacore/src/main/java/org/opensearch/sql/monitor/profile/ProfilePlanNode.javacore/src/main/java/org/opensearch/sql/calcite/profile/ProfileScannableRel.javacore/src/test/java/org/opensearch/sql/calcite/profile/PlanProfileBuilderTest.javaopensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.javacore/src/main/java/org/opensearch/sql/monitor/profile/QueryProfile.javacore/src/test/java/org/opensearch/sql/calcite/profile/ProfileEnumerableRelTest.javacore/src/main/java/org/opensearch/sql/monitor/profile/DefaultProfileContext.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- 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 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/calcite/utils/CalciteToolsHelper.javacore/src/main/java/org/opensearch/sql/monitor/profile/ProfileUtils.javacore/src/main/java/org/opensearch/sql/calcite/profile/PlanProfileBuilder.javacore/src/main/java/org/opensearch/sql/monitor/profile/NoopProfileContext.javacore/src/main/java/org/opensearch/sql/calcite/profile/ProfileEnumerableRel.javacore/src/main/java/org/opensearch/sql/monitor/profile/ProfileContext.javacore/src/main/java/org/opensearch/sql/monitor/profile/ProfilePlanNodeMetrics.javacore/src/main/java/org/opensearch/sql/monitor/profile/ProfilePlanNode.javacore/src/main/java/org/opensearch/sql/calcite/profile/ProfileScannableRel.javacore/src/test/java/org/opensearch/sql/calcite/profile/PlanProfileBuilderTest.javaopensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.javacore/src/main/java/org/opensearch/sql/monitor/profile/QueryProfile.javacore/src/test/java/org/opensearch/sql/calcite/profile/ProfileEnumerableRelTest.javacore/src/main/java/org/opensearch/sql/monitor/profile/DefaultProfileContext.java
core/src/main/java/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/**/*.java: - New functions MUST have unit tests in the same commit
Files:
core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.javacore/src/main/java/org/opensearch/sql/monitor/profile/ProfileUtils.javacore/src/main/java/org/opensearch/sql/calcite/profile/PlanProfileBuilder.javacore/src/main/java/org/opensearch/sql/monitor/profile/NoopProfileContext.javacore/src/main/java/org/opensearch/sql/calcite/profile/ProfileEnumerableRel.javacore/src/main/java/org/opensearch/sql/monitor/profile/ProfileContext.javacore/src/main/java/org/opensearch/sql/monitor/profile/ProfilePlanNodeMetrics.javacore/src/main/java/org/opensearch/sql/monitor/profile/ProfilePlanNode.javacore/src/main/java/org/opensearch/sql/calcite/profile/ProfileScannableRel.javacore/src/main/java/org/opensearch/sql/monitor/profile/QueryProfile.javacore/src/main/java/org/opensearch/sql/monitor/profile/DefaultProfileContext.java
**/calcite/**/*.java
⚙️ CodeRabbit configuration file
**/calcite/**/*.java: - Follow existing Calcite integration patterns
- Verify RelNode visitor implementations are complete
- Check RexNode handling follows project conventions
- Validate SQL generation is correct and optimized
- Ensure Calcite version compatibility
- Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor
- Document any Calcite-specific workarounds
- Test compatibility with Calcite version constraints
Files:
core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.javacore/src/main/java/org/opensearch/sql/calcite/profile/PlanProfileBuilder.javacore/src/main/java/org/opensearch/sql/calcite/profile/ProfileEnumerableRel.javacore/src/main/java/org/opensearch/sql/calcite/profile/ProfileScannableRel.javacore/src/test/java/org/opensearch/sql/calcite/profile/PlanProfileBuilderTest.javacore/src/test/java/org/opensearch/sql/calcite/profile/ProfileEnumerableRelTest.java
**/*Test.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*Test.java: All new business logic requires unit tests
Name unit tests with*Test.javasuffix in OpenSearch SQL
Files:
core/src/test/java/org/opensearch/sql/calcite/profile/PlanProfileBuilderTest.javacore/src/test/java/org/opensearch/sql/calcite/profile/ProfileEnumerableRelTest.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
core/src/test/java/org/opensearch/sql/calcite/profile/PlanProfileBuilderTest.javacore/src/test/java/org/opensearch/sql/calcite/profile/ProfileEnumerableRelTest.java
🧠 Learnings (5)
📚 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.javadocs/user/ppl/interfaces/endpoint.mdcore/src/test/java/org/opensearch/sql/calcite/profile/PlanProfileBuilderTest.javaopensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.javacore/src/test/java/org/opensearch/sql/calcite/profile/ProfileEnumerableRelTest.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 : Document Calcite-specific workarounds in code
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: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration
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-29T05:32:11.893Z
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:11.893Z
Learning: In opensearch-project/sql, when creating custom Calcite RelNode classes that extend EnumerableLimitSort or other Calcite RelNode types, always override the `copy` method. Without overriding copy, the class will downgrade to its parent class type during copy operations, losing the custom implementation.
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/profile/ProfileEnumerableRel.javacore/src/main/java/org/opensearch/sql/calcite/profile/ProfileScannableRel.java
🧬 Code graph analysis (7)
core/src/main/java/org/opensearch/sql/calcite/profile/PlanProfileBuilder.java (1)
core/src/main/java/org/opensearch/sql/monitor/profile/ProfilePlanNode.java (1)
ProfilePlanNode(13-38)
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/calcite/profile/ProfileEnumerableRel.java (2)
core/src/main/java/org/opensearch/sql/monitor/profile/ProfilePlanNode.java (1)
ProfilePlanNode(13-38)core/src/main/java/org/opensearch/sql/monitor/profile/ProfilePlanNodeMetrics.java (1)
ProfilePlanNodeMetrics(11-35)
core/src/main/java/org/opensearch/sql/monitor/profile/ProfilePlanNode.java (1)
core/src/main/java/org/opensearch/sql/monitor/profile/ProfileUtils.java (1)
ProfileUtils(9-22)
core/src/main/java/org/opensearch/sql/calcite/profile/ProfileScannableRel.java (1)
core/src/main/java/org/opensearch/sql/monitor/profile/ProfilePlanNode.java (1)
ProfilePlanNode(13-38)
core/src/test/java/org/opensearch/sql/calcite/profile/ProfileEnumerableRelTest.java (2)
core/src/main/java/org/opensearch/sql/monitor/profile/ProfilePlanNodeMetrics.java (1)
ProfilePlanNodeMetrics(11-35)core/src/main/java/org/opensearch/sql/calcite/profile/ProfileEnumerableRel.java (1)
ProfileEnumerableRel(32-197)
core/src/main/java/org/opensearch/sql/monitor/profile/DefaultProfileContext.java (1)
core/src/main/java/org/opensearch/sql/monitor/profile/ProfileUtils.java (1)
ProfileUtils(9-22)
🔇 Additional comments (16)
core/src/main/java/org/opensearch/sql/monitor/profile/ProfileUtils.java (1)
1-22: LGTM!Clean utility class following best practices: private constructor prevents instantiation, proper JavaDoc, and the rounding logic correctly converts nanoseconds to milliseconds with two decimal precision. The implementation is stateless and thread-safe.
docs/user/ppl/interfaces/endpoint.md (1)
183-202: LGTM!The documentation updates clearly explain the new plan profiling feature. The example JSON structure is well-formed, and the notes accurately describe the inclusive timing semantics and potential discrepancies users might observe. This will help users correctly interpret the profiling output.
core/src/main/java/org/opensearch/sql/monitor/profile/ProfileContext.java (1)
10-28: LGTM!The new interface methods are well-designed and properly documented.
isEnabled()provides an efficient way to skip profiling work when disabled, andsetPlanRoot()enables the plan tree integration. The JavaDoc includes appropriate@returnand@paramannotations as per coding guidelines.core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java (1)
323-328: No action needed. The code is safe as-is.
QueryProfiling.current()guarantees a non-null return in all cases: it either returns the active profiling context orNoopProfileContext.INSTANCEwhen profiling is not activated. The implementation handles null ThreadLocal values explicitly (line 28), ensuring this method can never return null. The code at lines 323-328 is correct without additional null checks.Likely an incorrect or invalid review comment.
opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java (2)
213-220: LGTM - Clean profiling integration.The EXECUTE phase profiling correctly captures both query execution and result set building time. The metric is properly recorded before calling
listener.onResponse(), ensuring timing is complete.
259-260: LGTM - Improved return-value design.Returning
QueryResponsedirectly instead of using callback-within-callback improves clarity and makes the profiling integration cleaner in the caller.Also applies to: 303-303
core/src/main/java/org/opensearch/sql/monitor/profile/ProfilePlanNodeMetrics.java (1)
13-14: Good use of LongAdder for thread-safe metrics.Using
LongAdderinstead ofAtomicLongis the right choice for a write-heavy counter accessed by multiple enumeration threads, as it provides better performance under contention.core/src/main/java/org/opensearch/sql/monitor/profile/ProfilePlanNode.java (1)
28-37: LGTM - Clean recursive snapshot implementation.The snapshot method correctly traverses children recursively and uses
List.copyOffor immutability. Returningnullfor empty children keeps the JSON output compact.core/src/main/java/org/opensearch/sql/calcite/profile/PlanProfileBuilder.java (1)
61-78: LGTM - Clean pair class for return value.The
ProfilePlanrecord-like class properly encapsulates the two related outputs from the profiling operation.core/src/main/java/org/opensearch/sql/monitor/profile/DefaultProfileContext.java (1)
43-61: LGTM!The
finish()method correctly:
- Uses the centralized
ProfileUtils.roundToMillisutility for consistent time conversion- Safely handles null
planRootbefore callingsnapshot()- Maintains thread safety with
synchronizedcore/src/test/java/org/opensearch/sql/calcite/profile/PlanProfileBuilderTest.java (2)
41-59: Good test coverage for core wrapping scenarios.Tests appropriately verify that:
EnumerableRelis wrapped asProfileEnumerableRelScannableimplementations are wrapped asProfileScannableRel- Node names are correctly propagated to snapshots
74-77: No action needed. The project's build configuration (sourceCompatibility = JavaVersion.VERSION_21,targetCompatibility = JavaVersion.VERSION_21) explicitly targets Java 21.List.getFirst()is an appropriate API for this target version and aligns with the project's development requirements.Likely an incorrect or invalid review comment.
core/src/main/java/org/opensearch/sql/calcite/profile/ProfileEnumerableRel.java (2)
154-196: LGTM! ProfileEnumerator correctly tracks metrics.The implementation properly:
- Times
moveNext()andclose()operations- Increments row count only on successful moves
- Uses try-finally to ensure timing is recorded even on exceptions
- Correctly delegates
current()andreset()without timing overhead
82-91: The implementation correctly handles the concern raised. ThewrapBlock()method includes explicit defensive checks and gracefully handles cases where blocks may not end with return statements. Lines 119-121 return the original block unchanged if the last statement is not aGotoStatement, and lines 124-126 return the original block if theGotoStatementlacks a Return kind or expression. The code does not assume the delegate'simplement()produces a block ending with a return statement—it validates this expectation and has fallback mechanisms.core/src/main/java/org/opensearch/sql/monitor/profile/QueryProfile.java (2)
81-99: LGTM! Clean hierarchical plan node structure.The
PlanNodenested class properly models the plan tree with:
- Appropriate
@SerializedNamefor JSON output consistency- Nullable
childrenlist for leaf nodes- Lombok
@Getterfor accessor generation
32-47: Good backward-compatible constructor design.The constructor chaining pattern maintains API compatibility while adding the new
planparameter. Existing callers continue to work withplan = null.
| private static RelNode wrap(RelNode rel, List<RelNode> inputs, ProfilePlanNode planNode) { | ||
| if (!(rel instanceof EnumerableRel)) { | ||
| try { | ||
| return rel.copy(rel.getTraitSet(), inputs); | ||
| } catch (UnsupportedOperationException e) { | ||
| return rel; | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential profiling chain break when copy() fails.
If rel.copy() throws UnsupportedOperationException, the method returns the original rel which still references its original unwrapped children. The wrapped inputs (with profiling instrumentation) are discarded, breaking the profiling chain for that subtree.
Consider logging a warning when this fallback occurs so operators can identify which node types don't support profiling.
Proposed fix with logging
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
public final class PlanProfileBuilder {
+ private static final Logger LOG = LogManager.getLogger(PlanProfileBuilder.class);
// ...
private static RelNode wrap(RelNode rel, List<RelNode> inputs, ProfilePlanNode planNode) {
if (!(rel instanceof EnumerableRel)) {
try {
return rel.copy(rel.getTraitSet(), inputs);
} catch (UnsupportedOperationException e) {
+ LOG.debug("Cannot wrap {} for profiling: copy not supported", rel.getRelTypeName());
return rel;
}
}| public final class ProfileScannableRel extends ProfileEnumerableRel implements Scannable { | ||
|
|
||
| private final Scannable scannable; | ||
|
|
||
| public ProfileScannableRel( | ||
| EnumerableRel delegate, List<RelNode> inputs, ProfilePlanNode planNode) { | ||
| super(delegate, inputs, planNode); | ||
| this.scannable = (Scannable) delegate; | ||
| } | ||
|
|
||
| @Override | ||
| public Enumerable<@Nullable Object> scan() { | ||
| return profile(scannable.scan(), planNode.metrics()); | ||
| } |
There was a problem hiding this comment.
Override copy() to preserve ProfileScannableRel type.
Based on learnings from this repository, custom Calcite RelNode classes that extend other RelNode types must override the copy method. Without this override, ProfileScannableRel will downgrade to ProfileEnumerableRel during copy operations, losing the Scannable interface implementation.
🔧 Proposed fix
public final class ProfileScannableRel extends ProfileEnumerableRel implements Scannable {
private final Scannable scannable;
public ProfileScannableRel(
EnumerableRel delegate, List<RelNode> inputs, ProfilePlanNode planNode) {
super(delegate, inputs, planNode);
this.scannable = (Scannable) delegate;
}
+ @Override
+ public RelNode copy(RelTraitSet traitSet, List<RelNode> inputs) {
+ return new ProfileScannableRel((EnumerableRel) scannable, inputs, planNode);
+ }
+
@Override
public Enumerable<@Nullable Object> scan() {
return profile(scannable.scan(), planNode.metrics());
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public final class ProfileScannableRel extends ProfileEnumerableRel implements Scannable { | |
| private final Scannable scannable; | |
| public ProfileScannableRel( | |
| EnumerableRel delegate, List<RelNode> inputs, ProfilePlanNode planNode) { | |
| super(delegate, inputs, planNode); | |
| this.scannable = (Scannable) delegate; | |
| } | |
| @Override | |
| public Enumerable<@Nullable Object> scan() { | |
| return profile(scannable.scan(), planNode.metrics()); | |
| } | |
| public final class ProfileScannableRel extends ProfileEnumerableRel implements Scannable { | |
| private final Scannable scannable; | |
| public ProfileScannableRel( | |
| EnumerableRel delegate, List<RelNode> inputs, ProfilePlanNode planNode) { | |
| super(delegate, inputs, planNode); | |
| this.scannable = (Scannable) delegate; | |
| } | |
| @Override | |
| public RelNode copy(RelTraitSet traitSet, List<RelNode> inputs) { | |
| return new ProfileScannableRel((EnumerableRel) scannable, inputs, planNode); | |
| } | |
| @Override | |
| public Enumerable<@Nullable Object> scan() { | |
| return profile(scannable.scan(), planNode.metrics()); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
@core/src/main/java/org/opensearch/sql/calcite/profile/ProfileScannableRel.java
around lines 17 - 30, ProfileScannableRel is missing an override of the
copy(...) method so copy operations can recreate a ProfileScannableRel (and
retain the Scannable behavior) instead of falling back to ProfileEnumerableRel;
add a public ProfileScannableRel copy(RelTraitSet traitSet, List<RelNode>
inputs) override that calls the superclass constructor with the incoming
delegate (cast to EnumerableRel), passes inputs and planNode, and sets the
scannable field by casting the delegate to Scannable so the new instance
preserves the Scannable implementation and metrics.
|
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-5044-to-2.19-dev
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 07629d43027f2fa406b268b8bc3ff5a25289ea26
# Push it to GitHub
git push --set-upstream origin backport/backport-5044-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 |
3 participants
Description
profile.planwith node,time_ms,rows, andchildren.EnumerableCalc,CalciteEnumerableIndexScan).analyze,optimize,execute,format) andsummary.total_time_ms.Classes
PlanProfileBuilder: builds a profiled RelNode tree and parallel plan-node tree.ProfileEnumerableRel: wraps EnumerableRel codegen; rewrites the return toProfileEnumerableRel.profile(...).ProfileEnumerator: measures per-operator wall-clock time and row counts.ProfileScannableRel: handlesScannableplans to keep scan timing in operator scope.ProfilePlanNode+ProfilePlanNodeMetrics: store per-node metrics and emit JSON snapshot.Example: Join Plan Interpretation
For a join query like
source=log00001 | join left=l right=r ON l.packets=r.packets log00001, the plan section is read top-down:EnumerableLimitis the root operator (final limiting of rows); itstime_msincludes all child work.EnumerableMergeJoinis the join operator; itstime_msincludes both scans plus join processing.CalciteEnumerableIndexScanrepresents one side of the join; two separate scan nodes indicate two index reads. Note, CalciteEnumerableIndexScan time include a OpenSearch inital scan timerowson each node is the number of rows produced by that operator.FAQ
Meaning of
time_msin Operatorstime_msis inclusive wall-clock time for that operator, covering time spent in its own enumerator plus any time spent waiting on children.Why Exclude Request-Level
time_msRelated Issues
#4983
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.