Skip to content

Commit f902540

Browse files
committed
Add setContextClassLoader wrapper to QueryService Calcite paths
The analyze() and convertToCalcitePlan() steps in executeWithCalcite() and explainWithCalcite() trigger RexSimplify which uses Janino for constant folding. Without the context classloader set, UDF expressions like SAFE_CAST('TRUE') can't be folded, causing explain plan differences vs main branch. Adding the wrapper here covers the entire Calcite lifecycle for both execute and explain paths. Revert the separate boolean string literal YAML since all three boolean tests now produce the same plan (SAFE_CAST folds correctly). Signed-off-by: Kai Huang <ahkcs@amazon.com>
1 parent 7187f0b commit f902540

File tree

3 files changed

+40
-32
lines changed

3 files changed

+40
-32
lines changed

core/src/main/java/org/opensearch/sql/executor/QueryService.java

Lines changed: 37 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -139,14 +139,23 @@ public void executeWithCalcite(
139139
QueryProfiling.activate(QueryContext.isProfileEnabled());
140140
ProfileMetric analyzeMetric = profileContext.getOrCreateMetric(MetricName.ANALYZE);
141141
long analyzeStart = System.nanoTime();
142-
CalcitePlanContext context =
143-
CalcitePlanContext.create(
144-
buildFrameworkConfig(), SysLimit.fromSettings(settings), queryType);
145-
context.setHighlightConfig(highlightConfig);
146-
RelNode relNode = analyze(plan, context);
147-
RelNode calcitePlan = convertToCalcitePlan(relNode, context);
148-
analyzeMetric.set(System.nanoTime() - analyzeStart);
149-
executionEngine.execute(calcitePlan, context, listener);
142+
// Set thread context classloader so patched Calcite (CALCITE-3745) uses the
143+
// SQL plugin's classloader for Janino compilation during planning/execution.
144+
Thread currentThread = Thread.currentThread();
145+
ClassLoader originalCl = currentThread.getContextClassLoader();
146+
currentThread.setContextClassLoader(QueryService.class.getClassLoader());
147+
try {
148+
CalcitePlanContext context =
149+
CalcitePlanContext.create(
150+
buildFrameworkConfig(), SysLimit.fromSettings(settings), queryType);
151+
context.setHighlightConfig(highlightConfig);
152+
RelNode relNode = analyze(plan, context);
153+
RelNode calcitePlan = convertToCalcitePlan(relNode, context);
154+
analyzeMetric.set(System.nanoTime() - analyzeStart);
155+
executionEngine.execute(calcitePlan, context, listener);
156+
} finally {
157+
currentThread.setContextClassLoader(originalCl);
158+
}
150159
} catch (Throwable t) {
151160
if (isCalciteFallbackAllowed(t) && !(t instanceof NonFallbackCalciteException)) {
152161
log.warn("Fallback to V2 query engine since got exception", t);
@@ -169,17 +178,26 @@ public void explainWithCalcite(
169178
() -> {
170179
try {
171180
QueryProfiling.noop();
172-
CalcitePlanContext context =
173-
CalcitePlanContext.create(
174-
buildFrameworkConfig(), SysLimit.fromSettings(settings), queryType);
175-
context.setHighlightConfig(highlightConfig);
176-
context.run(
177-
() -> {
178-
RelNode relNode = analyze(plan, context);
179-
RelNode calcitePlan = convertToCalcitePlan(relNode, context);
180-
executionEngine.explain(calcitePlan, mode, context, listener);
181-
},
182-
settings);
181+
// Set thread context classloader so patched Calcite (CALCITE-3745) uses the
182+
// SQL plugin's classloader for Janino compilation during planning/execution.
183+
Thread currentThread = Thread.currentThread();
184+
ClassLoader originalCl = currentThread.getContextClassLoader();
185+
currentThread.setContextClassLoader(QueryService.class.getClassLoader());
186+
try {
187+
CalcitePlanContext context =
188+
CalcitePlanContext.create(
189+
buildFrameworkConfig(), SysLimit.fromSettings(settings), queryType);
190+
context.setHighlightConfig(highlightConfig);
191+
context.run(
192+
() -> {
193+
RelNode relNode = analyze(plan, context);
194+
RelNode calcitePlan = convertToCalcitePlan(relNode, context);
195+
executionEngine.explain(calcitePlan, mode, context, listener);
196+
},
197+
settings);
198+
} finally {
199+
currentThread.setContextClassLoader(originalCl);
200+
}
183201
} catch (Throwable t) {
184202
if (isCalciteFallbackAllowed(t)) {
185203
log.warn("Fallback to V2 query engine since got exception", t);

integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2735,14 +2735,13 @@ public void testFilterBooleanFieldWithTRUE() throws IOException {
27352735
@Test
27362736
public void testFilterBooleanFieldWithStringLiteral() throws IOException {
27372737
enabledOnlyWhenPushdownIsEnabled();
2738-
// Test boolean field with string literal 'TRUE' - Calcite keeps SAFE_CAST in logical plan
2739-
// but still generates the same term query in physical plan
2738+
// Test boolean field with string literal 'TRUE' - Calcite converts to boolean true
2739+
// and generates same term query as boolean literal
27402740
String query =
27412741
StringUtils.format(
27422742
"source=%s firstname=Amber | where male = 'TRUE' | fields firstname", TEST_INDEX_BANK);
27432743
var result = explainQueryYaml(query);
2744-
String expected =
2745-
loadExpectedPlan("explain_filter_query_string_with_boolean_string_literal.yaml");
2744+
String expected = loadExpectedPlan("explain_filter_query_string_with_boolean.yaml");
27462745
assertYamlEqualsIgnoreId(expected, result);
27472746
}
27482747

integ-test/src/test/resources/expectedOutput/calcite/explain_filter_query_string_with_boolean_string_literal.yaml

Lines changed: 0 additions & 9 deletions
This file was deleted.

0 commit comments

Comments
 (0)