[Experimental] Add engine extensions interface

[LantaoJin]

Description

Add engine extensions interface for better testing olap plugin. This code will be refactor when #5068 resolved.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

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.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 3e3321c)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Add core engine extension interface and delegating engine Relevant files: core/src/main/java/org/opensearch/sql/executor/ExecutionEngine.java core/src/main/java/org/opensearch/sql/executor/DelegatingExecutionEngine.java core/src/test/java/org/opensearch/sql/executor/DelegatingExecutionEngineTest.java Sub-PR theme: Wire engine extensions into plugin and transport layer Relevant files: plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java plugin/src/main/java/org/opensearch/sql/plugin/config/EngineExtensionsHolder.java plugin/src/main/java/org/opensearch/sql/plugin/config/OpenSearchPluginModule.java plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryAction.java

⚡ Recommended focus areas for review Redundant Constructor The record EngineExtensionsHolder defines an explicit compact constructor that duplicates what a canonical record constructor would do. Additionally, since this is a record, the explicit constructor body is redundant with the record's built-in accessor. Consider using a compact constructor instead, or verifying that the record semantics are correctly applied here. public EngineExtensionsHolder(List<ExecutionEngine> engines) { this.engines = engines != null ? List.copyOf(engines) : List.of(); } Mutable Field The field executionEngineExtensions is initialized to List.of() but then reassigned in loadExtensions. Since loadExtensions may be called at any time (including concurrently during plugin initialization), this mutable field could lead to race conditions or inconsistent state if createComponents is called before or during loadExtensions. Consider making the field volatile or ensuring proper synchronization. private List<ExecutionEngine> executionEngineExtensions = List.of(); Missing SQL Action Update The TransportPPLQueryAction is updated to accept EngineExtensionsHolder and use OpenSearchPluginModule(extensionsHolder.engines()), but there may be a corresponding SQL transport action (e.g., TransportSQLQueryAction) that also constructs OpenSearchPluginModule and would need the same update to support engine extensions for SQL queries. modules.add(new OpenSearchPluginModule(extensionsHolder.engines())); Double canVectorize Call In both execute(RelNode, ...) and explain(RelNode, ...), findExtension is called which internally calls canVectorize on each extension. This means canVectorize is called twice per request when canVectorize is also invoked externally (e.g., from DelegatingExecutionEngine.canVectorize). If canVectorize has side effects or is expensive, this could be a concern. Consider caching the result or documenting that canVectorize must be stateless and cheap. Optional<ExecutionEngine> ext = findExtension(plan); if (ext.isPresent()) { log.info("Routing query to extension engine : {}", ext.get().getClass().getSimpleName()); ext.get().execute(plan, context, listener); } else { defaultEngine.execute(plan, context, listener); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 3e3321c
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Use compact canonical constructor in record In a Java record, the compact canonical constructor should be used for validation/normalization instead of the full constructor, because the full constructor in a record must assign all components directly. The current code attempts to reassign this.engines in a non-compact constructor, which is valid Java syntax for records but redundant since the record's accessor will return the value set. However, using the compact constructor form is the idiomatic and safer approach to ensure the normalization is always applied. plugin/src/main/java/org/opensearch/sql/plugin/config/EngineExtensionsHolder.java [16-20] public record EngineExtensionsHolder(List<ExecutionEngine> engines) { - public EngineExtensionsHolder(List<ExecutionEngine> engines) { - this.engines = engines != null ? List.copyOf(engines) : List.of(); + public EngineExtensionsHolder { + engines = engines != null ? List.copyOf(engines) : List.of(); } } Suggestion importance[1-10]: 5 __ Why: Using the compact canonical constructor is the idiomatic Java record pattern for normalization. The current full constructor form is valid but non-idiomatic; the compact form is cleaner and ensures normalization is always applied as part of the record's canonical construction.	Low
	Remove unnecessary null check on extension loader result The loadExtensions method in OpenSearch's ExtensiblePlugin.ExtensionLoader never returns null — it returns an empty list when no extensions are found. The null check is unnecessary and may mask future API changes. More importantly, executionEngineExtensions is initialized to List.of() at field declaration, so if loadExtensions is never called, the field is already safe. The null guard is harmless but misleading. plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java [161-162] List<ExecutionEngine> loaded = loader.loadExtensions(ExecutionEngine.class); -this.executionEngineExtensions = loaded != null ? List.copyOf(loaded) : List.of(); +this.executionEngineExtensions = List.copyOf(loaded); Suggestion importance[1-10]: 3 __ Why: The null check is technically unnecessary since loadExtensions returns an empty list rather than null, but removing it is a minor cleanup. The suggestion is accurate and the improved_code correctly reflects the change, but the impact is low.	Low

---

Previous suggestions

Suggestions up to commit fc9261e

Category	Suggestion	Impact
Possible issue	Guard against null extension list assignment The loadExtensions method from OpenSearch's ExtensiblePlugin.ExtensionLoader returns a non-null list, but assigning it to a field initialized as List.of() (immutable) and then checking for null is inconsistent. More critically, if loadExtensions returns null (e.g., in some environments), the null value will be stored and later passed to EngineExtensionsHolder and OpenSearchPluginModule, causing a NullPointerException. Add a null-safe assignment. plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java [161-162] -this.executionEngineExtensions = loader.loadExtensions(ExecutionEngine.class); -if (executionEngineExtensions != null && !executionEngineExtensions.isEmpty()) { +List<ExecutionEngine> loaded = loader.loadExtensions(ExecutionEngine.class); +this.executionEngineExtensions = loaded != null ? loaded : List.of(); +if (!executionEngineExtensions.isEmpty()) { Suggestion importance[1-10]: 6 __ Why: The null check after assignment is inconsistent with the field's initialization to List.of(). If loadExtensions can return null, storing it directly could cause NullPointerException downstream. The fix is valid and defensive, though OpenSearch's ExtensionLoader typically guarantees non-null returns.	Low
General	Avoid double-calling canVectorize per request The canVectorize method is called twice for the same extension in execute — once in canVectorize() and again here. While not a bug per se, a more critical issue is that explain also calls canVectorize independently, meaning a stateful or non-deterministic canVectorize could route execute and explain to different engines for the same plan. Consider caching the result or using a helper method to select the engine once. core/src/main/java/org/opensearch/sql/executor/DelegatingExecutionEngine.java [58-65] -for (ExecutionEngine ext : extensions) { - if (ext.canVectorize(plan)) { - log.info("Routing query to extension engine : {}", ext.getClass().getSimpleName()); - ext.execute(plan, context, listener); - return; - } +ExecutionEngine selected = extensions.stream() + .filter(ext -> ext.canVectorize(plan)) + .findFirst() + .orElse(null); +if (selected != null) { + log.info("Routing query to extension engine : {}", selected.getClass().getSimpleName()); + selected.execute(plan, context, listener); + return; } defaultEngine.execute(plan, context, listener); Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that canVectorize is called twice per extension per request (once in canVectorize() and once in execute/explain). However, for typical deterministic implementations this is a minor inefficiency rather than a bug, and the refactoring to use streams doesn't significantly improve correctness.	Low
	Ensure immutability of stored extensions list The engines list stored in the record could be a mutable list (e.g., returned by loader.loadExtensions), allowing external mutation of the holder's state. Wrap it in an unmodifiable list to ensure immutability. plugin/src/main/java/org/opensearch/sql/plugin/config/EngineExtensionsHolder.java [16] -public record EngineExtensionsHolder(List<ExecutionEngine> engines) {} +public record EngineExtensionsHolder(List<ExecutionEngine> engines) { + public EngineExtensionsHolder(List<ExecutionEngine> engines) { + this.engines = engines != null ? List.copyOf(engines) : List.of(); + } +} Suggestion importance[1-10]: 4 __ Why: Wrapping the list in List.copyOf() is a valid defensive practice to prevent external mutation of the record's state. However, this is a minor defensive improvement since the list is only used for iteration and the risk of external mutation is low in this context.	Low

[github-actions]

Persistent review updated to latest commit 3e3321c