Make analytics-engine an optional dependency via AnalyticsFrontEndExtension SPI#5401
Open
ahkcs wants to merge 1 commit intoopensearch-project:feature/mustang-ppl-integrationfrom
Open
Conversation
Contributor
PR Code Analyzer ❗AI-powered 'Code-Diff-Analyzer' found issues on commit 35b2fd1.
The table above displays the top 10 most important findings. Pull Requests Author(s): Please update your Pull Request according to the report above. Repository Maintainer(s): You can Thanks. |
38d39d4 to
3d19ed1
Compare
…ension SPI End-to-end consumer-side wiring for the AnalyticsFrontEndExtension SPI (analytics-framework). Lets the SQL plugin install and run on stock OpenSearch distributions that don't ship analytics-engine, while preserving the analytics routing behavior when both plugins are co-installed. Build wiring (plugin/build.gradle): - Mark analytics-engine as ;optional=true in extendedPlugins. OpenSearch's PluginsService.checkBundleJarHell skips both URL-overlap and class-level JarHell against optional deps, so SQL bundles its own copies of the previously-stripped jars (Calcite, Guava, etc.) again. - Drop the entire bundlePlugin exclusion list — the hand-maintained jar-deduplication hack goes away with the SPI. - Vendor the rebuilt analytics-framework-3.7.0-SNAPSHOT.jar (with the new AnalyticsFrontEndExtension + AnalyticsServices types) and the rebuilt analytics-engine-3.7.0-SNAPSHOT.zip (with the producer-side Guice TypeListener that pushes services to consumers). SPI consumer (SQLAnalyticsFrontEndExtension.java + META-INF/services registration): - Implements AnalyticsFrontEndExtension; setAnalyticsServices stashes the executor + schemaProvider into AnalyticsExecutorHolder. - Kept in a class separate from SQLPlugin so SQLPlugin's bytecode doesn't reference any analytics-framework class. When AE is absent, ServiceLoader never touches this class and SQL boots without analytics-framework on its runtime classpath. Holder isolation (AnalyticsExecutorHolder.java): - Refactored to store services as Object internally — no analytics-framework type appears in any signature loaded at SQL plugin startup. Callers cast at use sites that are already gated on a non-null value. - Added schemaProvider alongside the executor. Lazy resolution at the request boundary: - TransportPPLQueryAction: drop the @Inject QueryPlanExecutor constructor parameter (the cross-plugin Guice dependency that broke the optional path). Replace with analyticsHandler() — null-checked lazy resolution from the holder; falls through to the legacy PPL path when AE is absent. - SQLPlugin#createSqlAnalyticsRouter: same lazy-supplier pattern against the new Object-typed holder + RestUnifiedQueryAction factory. - RestUnifiedQueryAction: add fromUnknownExecutor(Object, Object) factory (the only cast site for the analytics-framework types). Replaced static OpenSearchSchemaBuilder.buildSchema() call with the injected SchemaProvider so the runtime no longer has a hard static reference to analytics-engine. Verified end-to-end with bin/opensearch-plugin install on a real OpenSearch 3.7 distribution: - WITH analytics-engine: all three plugins install; Lucene PPL works; parquet_* PPL routes through the analytics engine (confirmed via /_plugins/_ppl/_explain showing Calcite logical plan). - WITHOUT analytics-engine: SQL + job-scheduler install (warning logged for missing optional AE); Lucene PPL works; parquet_* PPL falls through to legacy with a clean IndexNotFoundException — no NoClassDefFoundError, no startup crash. Pairs with the producer-side wiring in opensearch-project/OpenSearch. Signed-off-by: Kai Huang <ahkcs@amazon.com>
3d19ed1 to
35b2fd1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
End-to-end SQL-side wiring for the
AnalyticsFrontEndExtensionSPI. Lets the SQL plugin install and run on stock OpenSearch distros that don't shipanalytics-engine, while preserving today's analytics routing when both plugins are co-installed.Fixes the
Missing plugin [analytics-engine]install failure observed infeature-build-opensearch#203. Once this lands, Peter's install-script workaround in opensearch-build can be retired.Pairs with opensearch-project/OpenSearch#21453 — the producer-side wiring.
Why marking it
;optional=truealone is not enoughTwo failure modes block the optional path:
Guice cross-plugin injection.
TransportPPLQueryActionhad@Inject QueryPlanExecutor. When AE is absent the binding does not exist → SQL plugin construction fails. Fixed by removing the@Injectparameter and resolving the executor lazily from a holder.Static analytics-framework references in SQL signatures.
AnalyticsExecutorHolderpreviously hadQueryPlanExecutorin its setter/getter signatures. Even after dropping@Inject, the first call toAnalyticsExecutorHolder.get()would resolve the return type →NoClassDefFoundErrorwhen analytics-framework is not on the classpath. Fixed by typing the holder asObjectinternally and confining the cast toRestUnifiedQueryAction.fromUnknownExecutor(only loaded after the holder confirms a non-null executor).Why the bundlePlugin exclusion list is preserved
PR #5302 added a patched Calcite (CALCITE-3745) inside analytics-engine's bundled
calcite-core-1.41.0.jarso Janino picks up the thread context classloader for runtime code generation. Without that patch, queries usingmode=extendedfail withCompileException: Cannot determine simple type name "org"because Janino's default classloader (the parent's, AE's) can't see SQL-plugin classes.Removing the
bundlePlugin { exclude ... }block makes SQL bundle its own unpatched Calcite/Janino, which competes with AE's patched copy via parent-first delegation. Doctest'sendpoint.mdtest breaks. The exclusion list is therefore retained — SQL relies on AE's patched Calcite at runtime, and;optional=trueonly covers the install side, not the runtime classloader story.When AE is absent, the SPI never fires,
RestUnifiedQueryActionis never constructed, and SQL falls through to the legacy Lucene PPL pipeline for everything.What's in this PR
Build wiring (
plugin/build.gradle)extendedPlugins = ['opensearch-job-scheduler', 'analytics-engine;optional=true']. JarHell skipped against the optional dep.bundlePlugin { exclude ... }block kept (with three new entrieshttpcore5-h2-*.jar,httpcore5-reactive-*.jar,httpclient5-*.jarfrom Exclude httpcore5-h2, httpcore5-reactive, httpclient5 from SQL bundle to fix jar hell #5400).libs/analytics-framework-3.7.0-SNAPSHOT.jarandlibs/analytics-engine-3.7.0-SNAPSHOT.ziprebuilt: framework + engine JARs come from the producer-side branch (withAnalyticsFrontEndExtension,AnalyticsServices, and the Guice TypeListener that pushes services to consumers); the bundled transitive deps (patched Calcite, commons-text, jts-io, accessors-smart, etc.) come from the previously-vendored bundle so the existing CALCITE-3745 patch + Calcite-needed runtime deps from Wire analytics-engine as extendedPlugins dependency #5302 are preserved.SPI consumer
plugin/src/main/java/org/opensearch/sql/plugin/SQLAnalyticsFrontEndExtension.java— implementsAnalyticsFrontEndExtension;setAnalyticsServicesstashes the executor + schemaProvider intoAnalyticsExecutorHolder. Kept in a class separate fromSQLPluginso SQLPlugin's bytecode does not reference any analytics-framework class. When AE is absent, ServiceLoader never touches this class.plugin/src/main/resources/META-INF/services/org.opensearch.analytics.spi.AnalyticsFrontEndExtension— standard Java SPI registration.Holder refactor (the runtime-correctness fix)
plugin/src/main/java/org/opensearch/sql/plugin/rest/AnalyticsExecutorHolder.java:Objectso no analytics-framework class appears in any signature loaded at SQL plugin startup.set(...)andget*()useObject; callers cast at use sites already gated on a non-null value.Lazy resolution at the request boundary
TransportPPLQueryAction: dropped@Inject QueryPlanExecutor; replaced eagerunifiedQueryHandlerfield withanalyticsHandler()— synchronized double-checked lazy method.SQLPlugin#createSqlAnalyticsRouter: same lazy-supplier pattern.RestUnifiedQueryAction: addedfromUnknownExecutor(NodeClient, ClusterService, Object, Object)factory — the only cast site for analytics-framework types. Constructor now takes aSchemaProvider(replacing the previous staticOpenSearchSchemaBuilder.buildSchema(...)call).RestUnifiedQueryActionTestupdated.End-to-end verification
Local install + PPL queries
WITH analytics-engine:
The
calcite.logicalblock proves the SPI push lifecycle fired end-to-end: consumer received services → holder populated → lazy handler built → request dispatched to analytics-engine.WITHOUT analytics-engine:
Lucene PPL works:
parquet_*falls through to legacy → cleanIndexNotFoundException, NOTNoClassDefFoundError:Doctest
Test plan
./gradlew :opensearch-sql-plugin:spotlessCheck— passes./gradlew :opensearch-sql-plugin:compileJava :opensearch-sql-plugin:compileTestJava— passes./gradlew :opensearch-sql-plugin:test --tests "*RestUnifiedQueryActionTest*"— passes./gradlew :doctest:doctest— 75/75 pass_explainparquet_*returns cleanIndexNotFoundException, notNoClassDefFoundErrorRelated