Wire analytics-engine as extendedPlugins dependency#5302
Open
ahkcs wants to merge 7 commits intofeature/mustang-ppl-integrationfrom
Open
Wire analytics-engine as extendedPlugins dependency#5302ahkcs wants to merge 7 commits intofeature/mustang-ppl-integrationfrom
ahkcs wants to merge 7 commits intofeature/mustang-ppl-integrationfrom
Conversation
Contributor
PR Code Analyzer ❗AI-powered 'Code-Diff-Analyzer' found issues on commit 0ad18a8.
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. |
Step 1: Plugin wiring and dependency integration. - Add analytics-engine as extendedPlugins in plugin/build.gradle - Vendor analytics-framework JAR (interfaces) and analytics-engine ZIP (plugin) built from OpenSearch sandbox/3.6 branch - Delete local QueryPlanExecutor interface, use upstream org.opensearch.analytics.exec.QueryPlanExecutor from JAR - Replace StubSchemaProvider with OpenSearchSchemaBuilder which reads real index mappings from ClusterState - Delete StubSchemaProvider (no longer needed) - Exclude shared JARs (Calcite, Guava, commons-*, etc.) from SQL plugin bundle to avoid jar hell with analytics-engine classloader - Load analytics-engine plugin in integTest and remoteCluster test clusters before opensearch-sql-plugin - Create parquet_logs and parquet_metrics indices in ITs so OpenSearchSchemaBuilder can resolve the schema - Update explain expected files for alphabetical field ordering Signed-off-by: Kai Huang <kaihuang@amazon.com> Signed-off-by: Kai Huang <ahkcs@amazon.com>
Every test cluster that loads opensearch-sql-plugin needs the analytics-engine plugin because SQL declares it as extendedPlugins. Added to yamlRestTest, integTestWithSecurity, remoteIntegTestWithSecurity, and integJdbcTest clusters. Signed-off-by: Kai Huang <kaihuang@amazon.com> Signed-off-by: Kai Huang <ahkcs@amazon.com>
Signed-off-by: Kai Huang <kaihuang@amazon.com> Signed-off-by: Kai Huang <ahkcs@amazon.com>
commons-text is needed by Calcite (parent classloader) for fuzzy matching but was only in the SQL plugin (child classloader). Also use lightweight parsing context in isAnalyticsIndex to avoid requiring cluster state. Signed-off-by: Kai Huang <ahkcs@amazon.com>
ahkcs
force-pushed
the
pr/mustang-plugin-wiring
branch
from
300fc86 to
62578cd
Compare
Calcite's EnumerableInterpretable.getBindable() hardcodes EnumerableInterpretable.class.getClassLoader() for Janino compilation. When analytics-engine is the parent classloader via extendedPlugins, this returns the parent classloader which cannot see SQL plugin classes, causing CompileException for any Enumerable code generation. Override implement() in OpenSearchCalcitePreparingStmt to use our own compileWithPluginClassLoader() which does the same code generation but uses CalciteToolsHelper.class.getClassLoader() (SQL plugin's child classloader) so Janino can resolve both parent and child classes. Signed-off-by: Kai Huang <ahkcs@amazon.com>
ahkcs
force-pushed
the
pr/mustang-plugin-wiring
branch
from
ca8bcf1 to
2c4c401
Compare
The previous approach (analytics-engine as parent) caused Janino classloader issues in 4 Calcite code paths. Reversing the relationship makes SQL plugin the parent, so Janino uses the SQL plugin classloader which can see both Calcite and SQL plugin classes. Changes: - Remove extendedPlugins=['analytics-engine'] from SQL plugin - Add ExtensiblePlugin interface to SQLPlugin - Rebuild analytics-engine ZIP with extended.plugins=opensearch-sql and without Calcite JARs (inherited from parent) - Move OpenSearchSchemaBuilder to analytics-framework JAR - Change analytics-framework from compileOnly to api in core - Reverse test cluster plugin load order (SQL first) - Revert all Janino classloader fixes (no longer needed) - Add classloader architecture options doc Signed-off-by: Kai Huang <ahkcs@amazon.com>
Signed-off-by: Kai Huang <ahkcs@amazon.com>
ahkcs
requested review from
LantaoJin,
RyanL1997,
Swiddis,
acarbonetto,
anirudha,
dai-chen,
joshuali925,
mengweieric,
noCharger,
penghuo,
ps48,
qianheng-aws,
songkant-aws,
vamsimanohar,
ykmr1224 and
yuancu
as code owners
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
1 participant
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
extendedPluginsparent classloader for shared Calcite classesbundlePluginexcludes to prevent jar hell between parent/child classloadersisAnalyticsIndexto use lightweight parsing context (no cluster state needed)Test plan
RestUnifiedQueryActionTestpasses (routing logic)