Skip to content

Use Object type for queryCtx in AnalyticsExecutionEngine#5496

Open
finnegancarroll wants to merge 1 commit into
opensearch-project:mainfrom
finnegancarroll:fix/remove-queryreqctx-from-core
Open

Use Object type for queryCtx in AnalyticsExecutionEngine#5496
finnegancarroll wants to merge 1 commit into
opensearch-project:mainfrom
finnegancarroll:fix/remove-queryreqctx-from-core

Conversation

@finnegancarroll
Copy link
Copy Markdown
Contributor

Description

The QueryRequestContext class lives in analytics-engine (plugin), not analytics-api (library). The core module only has a compileOnly dependency on analytics-api, so referencing QueryRequestContext directly causes compilation failures when the published analytics-api SNAPSHOT does not include it.

Since QueryPlanExecutor.execute() takes Object as the context parameter anyway, this PR changes the parameter type from org.opensearch.analytics.QueryRequestContext to Object. The value is passed through opaquely — no type-specific operations are performed on it in this module.

Issues Resolved

Fixes snapshot publish failure: cannot find symbol: class QueryRequestContext in package org.opensearch.analytics

Check List

  • New functionality includes testing
    • N/A — type-only change, no behavioral difference
  • Commits are signed with DCO

The QueryRequestContext class lives in analytics-engine (plugin) not
analytics-api (library). The core module only depends on analytics-api
at compile time, so referencing QueryRequestContext directly causes
compilation failures when the published analytics-api SNAPSHOT does
not include it.

Since QueryPlanExecutor.execute() takes Object as the context
parameter anyway, just use Object here. The value is passed through
opaquely — no type-specific operations are performed on it.

Signed-off-by: Finn Carroll <carrofin@amazon.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 2, 2026

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit ad2b1f5.

PathLineSeverityDescription
core/src/main/java/org/opensearch/sql/executor/analytics/AnalyticsExecutionEngine.java94lowBoth execute() and executeWithProfile() had their queryCtx parameter type widened from the specific org.opensearch.analytics.QueryRequestContext to Object. This removes compile-time type safety and could allow callers to pass arbitrary objects where a validated context was previously required. While this is likely a legitimate decoupling refactor, it's worth verifying that downstream usage of queryCtx still validates the actual runtime type before use, and that QueryRequestContext provided no security-relevant invariants (e.g., authorization checks, tenant isolation) that are now bypassed.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 0 | Low: 1


Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.


⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 2, 2026

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ No major issues detected

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 2, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Add generic type parameter

Using Object type for queryCtx removes type safety and makes the API contract
unclear. Consider using a generic type parameter or creating an interface that
QueryRequestContext implements to maintain type safety while avoiding direct
dependency.

core/src/main/java/org/opensearch/sql/executor/analytics/AnalyticsExecutionEngine.java [89-93]

-public void execute(
+public <T> void execute(
     RelNode plan,
     CalcitePlanContext context,
-    Object queryCtx,
+    T queryCtx,
     ResponseListener<QueryResponse> listener) {
Suggestion importance[1-10]: 5

__

Why: While the suggestion correctly identifies that using Object reduces type safety, the proposed generic type parameter <T> doesn't significantly improve the situation without constraints. The change from a specific type to Object appears intentional to decouple from org.opensearch.analytics.QueryRequestContext, and adding an unconstrained generic provides minimal benefit over Object.

Low

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant