Skip to content

Add Settings to UnifiedQueryPlanner #4911

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
lostella wants to merge 5 commits into from
Closed

Add Settings to UnifiedQueryPlanner #4911

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fecd769
Add Settings to UnifiedQueryPlanner
lostella Dec 5, 2025
1a0dd07
Remove unused constructor, consolidate tests
lostella Dec 8, 2025
c17afdf
Simplify test
lostella Dec 8, 2025
c5200d2
Fix formatting issues
lostella Dec 8, 2025
2f46c5f
Rebase
lostella Dec 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions api/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ dependencies {
testImplementation group: 'junit', name: 'junit', version: '4.13.2'
testImplementation group: 'org.hamcrest', name: 'hamcrest-library', version: "${hamcrest_version}"
testImplementation group: 'org.mockito', name: 'mockito-core', version: "${mockito_version}"
testImplementation group: 'org.mockito', name: 'mockito-junit-jupiter', version: "${mockito_version}"
testImplementation group: 'org.apache.calcite', name: 'calcite-testkit', version: '1.41.0'
}

Expand Down
27 changes: 23 additions & 4 deletions api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import org.opensearch.sql.calcite.SysLimit;
import org.opensearch.sql.common.antlr.Parser;
import org.opensearch.sql.common.antlr.SyntaxCheckException;
import org.opensearch.sql.common.setting.Settings;
import org.opensearch.sql.executor.QueryType;
import org.opensearch.sql.ppl.antlr.PPLSyntaxParser;
import org.opensearch.sql.ppl.parser.AstBuilder;
Expand All @@ -52,21 +53,26 @@ public class UnifiedQueryPlanner {
/** Calcite framework configuration used during logical plan construction. */
private final FrameworkConfig config;

private final Settings settings;

/** AST-to-RelNode visitor that builds logical plans from the parsed AST. */
private final CalciteRelNodeVisitor relNodeVisitor =
new CalciteRelNodeVisitor(new EmptyDataSourceService());

/**
* Constructs a UnifiedQueryPlanner for a given query type and schema root.
* Constructs a UnifiedQueryPlanner for a given query type, schema root, and settings.
*
* @param queryType the query language type (e.g., PPL)
* @param rootSchema the root Calcite schema containing all catalogs and tables
* @param defaultPath dot-separated path of schema to set as default schema
* @param settings configuration settings for query processing
*/
public UnifiedQueryPlanner(QueryType queryType, SchemaPlus rootSchema, String defaultPath) {
public UnifiedQueryPlanner(
QueryType queryType, SchemaPlus rootSchema, String defaultPath, Settings settings) {
this.queryType = queryType;
this.parser = buildQueryParser(queryType);
this.config = buildCalciteConfig(rootSchema, defaultPath);
this.settings = settings;
}
Comment on lines +70 to 76
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Validate Settings parameter or document null handling.

The constructor accepts Settings but does not validate whether it's null. According to issue #4910, the NPE occurs when validateJoinType calls settings.getSettingValue(...) on a null settings object. While this PR threads Settings through the planner, it does not prevent null from being passed, which would cause the same NPE for join queries.

Consider one of the following approaches:

  1. Require non-null Settings (recommended if Settings is always needed):
 public UnifiedQueryPlanner(
     QueryType queryType, SchemaPlus rootSchema, String defaultPath, Settings settings) {
+  this.settings = Objects.requireNonNull(settings, "Settings cannot be null");
   this.queryType = queryType;
   this.parser = buildQueryParser(queryType);
   this.config = buildCalciteConfig(rootSchema, defaultPath);
-  this.settings = settings;
 }
  1. Allow null but document the limitation (if Settings is optional for simple queries):
    • Update JavaDoc to state: "settings may be null; join queries will fail if settings is not provided"
    • Consider defensive checks in parse() method

Based on coding guidelines: validate inputs and use proper error handling.

🤖 Prompt for AI Agents 
In api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java around
lines 70 to 76, the constructor accepts a Settings parameter but does not
validate it, leading to NPEs later when methods like validateJoinType call
settings.getSettingValue(...). Fix by requiring a non-null Settings: validate
the settings argument in the constructor (throw
IllegalArgumentException/NullPointerException with a clear message if null) and
update the constructor JavaDoc to state Settings is required; alternatively, if
Settings must remain optional, add a JavaDoc note that settings may be null and
add defensive null checks where settings is used (e.g., in
parse()/validateJoinType) to avoid NPEs.


/**
Expand Down Expand Up @@ -124,7 +130,8 @@ private UnresolvedPlan parse(String query) {
ParseTree cst = parser.parse(query);
AstStatementBuilder astStmtBuilder =
new AstStatementBuilder(
new AstBuilder(query), AstStatementBuilder.StatementBuilderContext.builder().build());
new AstBuilder(query, settings),
Copy link
Member

@LantaoJin LantaoJin Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the key part to resolve the issue. Can you remove the single parameter construction?

  public AstBuilder(String query) {
    this(query, null);
  }

It's nothing but a hassle.

Copy link
Author

@lostella lostella Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried, but realized some Calcite tests rely on this:

Maybe this can be done as a follow-up together with changes to the tests?

AstStatementBuilder.StatementBuilderContext.builder().build());
Statement statement = cst.accept(astStmtBuilder);

if (statement instanceof Query) {
Expand Down Expand Up @@ -164,6 +171,7 @@ public static class Builder {
private String defaultNamespace;
private QueryType queryType;
private boolean cacheMetadata;
private Settings settings;

/**
* Sets the query language frontend to be used by the planner.
Expand All @@ -176,6 +184,17 @@ public Builder language(QueryType queryType) {
return this;
}

/**
* Sets the settings for query processing configuration.
*
* @param settings the Settings instance for configuration
* @return this builder instance
*/
public Builder settings(Settings settings) {
this.settings = settings;
return this;
}

/**
* Registers a catalog with the specified name and its associated schema. The schema can be a
* flat or nested structure (e.g., catalog → schema → table), depending on how data is
Expand Down Expand Up @@ -221,7 +240,7 @@ public UnifiedQueryPlanner build() {
Objects.requireNonNull(queryType, "Must specify language before build");
SchemaPlus rootSchema = CalciteSchema.createRootSchema(true, cacheMetadata).plus();
catalogs.forEach(rootSchema::add);
return new UnifiedQueryPlanner(queryType, rootSchema, defaultNamespace);
return new UnifiedQueryPlanner(queryType, rootSchema, defaultNamespace, settings);
}
}
}
20 changes: 20 additions & 0 deletions api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,14 @@
import org.apache.calcite.schema.Schema;
import org.apache.calcite.schema.impl.AbstractSchema;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnitRunner;
import org.opensearch.sql.common.antlr.SyntaxCheckException;
import org.opensearch.sql.common.setting.Settings;
import org.opensearch.sql.executor.QueryType;

@RunWith(MockitoJUnitRunner.class)
public class UnifiedQueryPlannerTest extends UnifiedQueryTestBase {

/** Test catalog consists of test schema above */
Expand All @@ -27,6 +32,8 @@ protected Map<String, Schema> getSubSchemaMap() {
}
};

@Mock private Settings testSettings;

@Test
public void testPPLQueryPlanning() {
UnifiedQueryPlanner planner =
Expand Down Expand Up @@ -155,4 +162,17 @@ public void testPlanPropagatingSyntaxCheckException() {

planner.plan("source = employees | eval"); // Trigger syntax error from parser
}

@Test
public void testJoinQuery() {
UnifiedQueryPlanner planner =
UnifiedQueryPlanner.builder()
.language(QueryType.PPL)
.catalog("opensearch", testSchema)
.defaultNamespace("opensearch")
.settings(testSettings)
.build();

planner.plan("source = employees | join on employees.id = payslips.id payslips");
}
}
16 changes: 15 additions & 1 deletion api/src/test/java/org/opensearch/sql/api/UnifiedQueryTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public void setUp() {
new AbstractSchema() {
@Override
protected Map<String, Table> getTableMap() {
return Map.of("employees", createEmployeesTable());
return Map.of("employees", createEmployeesTable(), "payslips", createPayslipsTable());
}
};

Expand All @@ -57,4 +57,18 @@ public RelDataType getRowType(RelDataTypeFactory typeFactory) {
}
};
}

protected Table createPayslipsTable() {
return new AbstractTable() {
@Override
public RelDataType getRowType(RelDataTypeFactory typeFactory) {
return typeFactory.createStructType(
List.of(
typeFactory.createSqlType(SqlTypeName.INTEGER),
typeFactory.createSqlType(SqlTypeName.DATE),
typeFactory.createSqlType(SqlTypeName.FLOAT)),
List.of("id", "date", "salary"));
}
};
}
}
Loading