From 915686b1f95328511f0eab4cb6f267243f5c1888 Mon Sep 17 00:00:00 2001 From: Lantao Jin Date: Tue, 22 Apr 2025 15:07:32 +0800 Subject: [PATCH 1/5] New added commands should throw exception when calcite disabled Signed-off-by: Lantao Jin --- .../org/opensearch/sql/analysis/Analyzer.java | 19 ++- .../sql/analysis/ExpressionAnalyzer.java | 22 +++ .../remote/CalciteNewAddedCommandsIT.java | 41 ++++++ .../sql/ppl/NewAddedCommandsIT.java | 136 ++++++++++++++++++ .../opensearch/sql/ppl/PPLIntegTestCase.java | 32 +++-- 5 files changed, 238 insertions(+), 12 deletions(-) create mode 100644 integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteNewAddedCommandsIT.java create mode 100644 integ-test/src/test/java/org/opensearch/sql/ppl/NewAddedCommandsIT.java diff --git a/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java b/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java index c9b5cabf198..ffa62140de1 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java +++ b/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java @@ -10,6 +10,7 @@ import static org.opensearch.sql.ast.tree.Sort.NullOrder.NULL_LAST; import static org.opensearch.sql.ast.tree.Sort.SortOrder.ASC; import static org.opensearch.sql.ast.tree.Sort.SortOrder.DESC; +import static org.opensearch.sql.common.setting.Settings.Key.CALCITE_ENGINE_ENABLED; import static org.opensearch.sql.data.type.ExprCoreType.DATE; import static org.opensearch.sql.data.type.ExprCoreType.STRUCT; import static org.opensearch.sql.data.type.ExprCoreType.TIME; @@ -53,8 +54,10 @@ import org.opensearch.sql.ast.tree.FillNull; import org.opensearch.sql.ast.tree.Filter; import org.opensearch.sql.ast.tree.Head; +import org.opensearch.sql.ast.tree.Join; import org.opensearch.sql.ast.tree.Kmeans; import org.opensearch.sql.ast.tree.Limit; +import org.opensearch.sql.ast.tree.Lookup; import org.opensearch.sql.ast.tree.ML; import org.opensearch.sql.ast.tree.Paginate; import org.opensearch.sql.ast.tree.Parse; @@ -162,8 +165,8 @@ public LogicalPlan visitSubqueryAlias(SubqueryAlias node, AnalysisContext contex STRUCT); return child; } else { - // TODO - throw new UnsupportedOperationException("SubqueryAlias is only supported in table alias"); + throw new UnsupportedOperationException( + "Subsearch is supported only when " + CALCITE_ENGINE_ENABLED.getKeyValue() + "=true"); } } @@ -685,6 +688,18 @@ public LogicalPlan visitCloseCursor(CloseCursor closeCursor, AnalysisContext con return new LogicalCloseCursor(closeCursor.getChild().get(0).accept(this, context)); } + @Override + public LogicalPlan visitJoin(Join node, AnalysisContext context) { + throw new UnsupportedOperationException( + "Join is supported only when " + CALCITE_ENGINE_ENABLED.getKeyValue() + "=true"); + } + + @Override + public LogicalPlan visitLookup(Lookup node, AnalysisContext context) { + throw new UnsupportedOperationException( + "Lookup is supported only when " + CALCITE_ENGINE_ENABLED.getKeyValue() + "=true"); + } + private LogicalSort buildSort( LogicalPlan child, AnalysisContext context, List sortFields) { ExpressionReferenceOptimizer optimizer = diff --git a/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java b/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java index fa2478d53ab..a7f60dd65c8 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java +++ b/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java @@ -7,6 +7,7 @@ import static org.opensearch.sql.ast.dsl.AstDSL.and; import static org.opensearch.sql.ast.dsl.AstDSL.compare; +import static org.opensearch.sql.common.setting.Settings.Key.CALCITE_ENGINE_ENABLED; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -49,6 +50,9 @@ import org.opensearch.sql.ast.expression.When; import org.opensearch.sql.ast.expression.WindowFunction; import org.opensearch.sql.ast.expression.Xor; +import org.opensearch.sql.ast.expression.subquery.ExistsSubquery; +import org.opensearch.sql.ast.expression.subquery.InSubquery; +import org.opensearch.sql.ast.expression.subquery.ScalarSubquery; import org.opensearch.sql.data.model.ExprValueUtils; import org.opensearch.sql.data.type.ExprCoreType; import org.opensearch.sql.data.type.ExprType; @@ -406,6 +410,24 @@ public Expression visitArgument(Argument node, AnalysisContext context) { return new NamedArgumentExpression(node.getArgName(), node.getValue().accept(this, context)); } + @Override + public Expression visitScalarSubquery(ScalarSubquery node, AnalysisContext context) { + throw new UnsupportedOperationException( + "Subsearch is supported only when " + CALCITE_ENGINE_ENABLED.getKeyValue() + "=true"); + } + + @Override + public Expression visitExistsSubquery(ExistsSubquery node, AnalysisContext context) { + throw new UnsupportedOperationException( + "Subsearch is supported only when " + CALCITE_ENGINE_ENABLED.getKeyValue() + "=true"); + } + + @Override + public Expression visitInSubquery(InSubquery node, AnalysisContext context) { + throw new UnsupportedOperationException( + "Subsearch is supported only when " + CALCITE_ENGINE_ENABLED.getKeyValue() + "=true"); + } + /** * If QualifiedName is actually a reserved metadata field, return the expr type associated with * the metadata field. diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteNewAddedCommandsIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteNewAddedCommandsIT.java new file mode 100644 index 00000000000..e4661ed95a5 --- /dev/null +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteNewAddedCommandsIT.java @@ -0,0 +1,41 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.calcite.remote; + +import org.opensearch.sql.ppl.NewAddedCommandsIT; + +public class CalciteNewAddedCommandsIT extends NewAddedCommandsIT { + @Override + public void init() throws Exception { + super.init(); + enableCalcite(); + disallowCalciteFallback(); + } + + @Override + public void testJoin() { + // fallback disabled, should pass + super.testJoin(); + // fallback enabled, should pass too + withFallbackEnabled(super::testJoin, ""); + } + + @Override + public void testLookup() { + // fallback disabled, should pass + super.testLookup(); + // fallback enabled, should pass too + withFallbackEnabled(super::testLookup, ""); + } + + @Override + public void testSubsearch() { + // fallback disabled, should pass + super.testSubsearch(); + // fallback enabled, should pass too + withFallbackEnabled(super::testSubsearch, ""); + } +} diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/NewAddedCommandsIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/NewAddedCommandsIT.java new file mode 100644 index 00000000000..95072e5766d --- /dev/null +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/NewAddedCommandsIT.java @@ -0,0 +1,136 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.ppl; + +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.opensearch.sql.common.setting.Settings.Key.CALCITE_ENGINE_ENABLED; +import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_BANK; +import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_DOG; + +import java.io.IOException; +import org.json.JSONObject; +import org.junit.jupiter.api.Test; +import org.opensearch.client.ResponseException; +import org.opensearch.sql.util.TestUtils; + +public class NewAddedCommandsIT extends PPLIntegTestCase { + @Override + public void init() throws Exception { + super.init(); + loadIndex(Index.BANK); + loadIndex(Index.DOG); + } + + @Test + public void testJoin() { + try { + JSONObject result; + try { + result = + executeQuery( + String.format( + "search source=%s | join on firstname=holdersName %s", + TEST_INDEX_BANK, TEST_INDEX_DOG)); + } catch (ResponseException e) { + result = new JSONObject(TestUtils.getResponseBody(e.getResponse())); + verifyQuery(result); + } + } catch (IOException e) { + fail(e.getMessage()); + } + } + + @Test + public void testLookup() { + try { + JSONObject result; + try { + result = + executeQuery( + String.format( + "search source=%s | lookup %s holdersName as firstname", + TEST_INDEX_BANK, TEST_INDEX_DOG)); + } catch (ResponseException e) { + result = new JSONObject(TestUtils.getResponseBody(e.getResponse())); + } + verifyQuery(result); + } catch (IOException e) { + fail(e.getMessage()); + } + } + + @Test + public void testSubsearch() { + try { + JSONObject result; + try { + result = + executeQuery( + String.format( + "search source=[source=%s | where age>35] as t | where age>35", + TEST_INDEX_BANK)); + } catch (ResponseException e) { + result = new JSONObject(TestUtils.getResponseBody(e.getResponse())); + } + verifyQuery(result); + + try { + result = + executeQuery( + String.format( + "search source=%s | where exists [ source=%s | where firstname=holdersName]", + TEST_INDEX_BANK, TEST_INDEX_DOG)); + } catch (ResponseException e) { + result = new JSONObject(TestUtils.getResponseBody(e.getResponse())); + } + verifyQuery(result); + + try { + result = + executeQuery( + String.format( + "search source=%s | where firstname in [ source=%s | fields holdersName]", + TEST_INDEX_BANK, TEST_INDEX_DOG)); + } catch (ResponseException e) { + result = new JSONObject(TestUtils.getResponseBody(e.getResponse())); + } + verifyQuery(result); + + try { + result = + executeQuery( + String.format( + "search source=%s | where firstname = [ source=%s | where holdersName='Hattie'" + + " | fields holdersName | head 1]", + TEST_INDEX_BANK, TEST_INDEX_DOG)); + } catch (ResponseException e) { + result = new JSONObject(TestUtils.getResponseBody(e.getResponse())); + } + verifyQuery(result); + } catch (IOException e) { + fail(e.getMessage()); + } + } + + private void verifyQuery(JSONObject result) { + try { + if (isCalciteEnabled()) { + assertFalse(result.getJSONArray("datarows").isEmpty()); + } else { + assertThat(result.getInt("status"), equalTo(500)); + JSONObject error = result.getJSONObject("error"); + assertThat( + error.getString("details"), + containsString( + "is supported only when " + CALCITE_ENGINE_ENABLED.getKeyValue() + "=true")); + assertThat(error.getString("type"), equalTo("UnsupportedOperationException")); + } + } catch (IOException e) { + fail(e.getMessage()); + } + } +} diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java index 948b0733723..b22ec4ce5c1 100644 --- a/integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java @@ -147,27 +147,39 @@ public static void disallowCalciteFallback() throws IOException { LOG.info("{} disabled", Settings.Key.CALCITE_FALLBACK_ALLOWED.name()); } - protected static boolean isFallbackEnabled() throws IOException { - return Boolean.parseBoolean( - getClusterSetting(Settings.Key.CALCITE_FALLBACK_ALLOWED.getKeyValue(), "persistent")); + protected static boolean isFallbackEnabled() { + try { + return Boolean.parseBoolean( + getClusterSetting(Settings.Key.CALCITE_FALLBACK_ALLOWED.getKeyValue(), "persistent")); + } catch (IOException e) { + throw new RuntimeException(e); + } } - public static void withFallbackEnabled(Runnable f, String msg) throws IOException { + public static void withFallbackEnabled(Runnable f, String msg) { LOG.info("Need fallback to v2 due to {}", msg); boolean isFallbackEnabled = isFallbackEnabled(); if (isFallbackEnabled) f.run(); else { try { - updateClusterSettings( - new SQLIntegTestCase.ClusterSetting( - "persistent", Settings.Key.CALCITE_FALLBACK_ALLOWED.getKeyValue(), "true")); + try { + updateClusterSettings( + new SQLIntegTestCase.ClusterSetting( + "persistent", Settings.Key.CALCITE_FALLBACK_ALLOWED.getKeyValue(), "true")); + } catch (IOException e) { + throw new RuntimeException(e); + } LOG.info( "Set {} to enabled and run the test", Settings.Key.CALCITE_FALLBACK_ALLOWED.name()); f.run(); } finally { - updateClusterSettings( - new SQLIntegTestCase.ClusterSetting( - "persistent", Settings.Key.CALCITE_FALLBACK_ALLOWED.getKeyValue(), "false")); + try { + updateClusterSettings( + new SQLIntegTestCase.ClusterSetting( + "persistent", Settings.Key.CALCITE_FALLBACK_ALLOWED.getKeyValue(), "false")); + } catch (IOException e) { + throw new RuntimeException(e); + } LOG.info("Reset {} back to disabled", Settings.Key.CALCITE_FALLBACK_ALLOWED.name()); } } From 777273b962b04acdc016d1874a6183aaca9c8bfd Mon Sep 17 00:00:00 2001 From: Lantao Jin Date: Tue, 22 Apr 2025 15:11:33 +0800 Subject: [PATCH 2/5] remove useless code Signed-off-by: Lantao Jin --- .../remote/CalciteNewAddedCommandsIT.java | 24 ------------------- 1 file changed, 24 deletions(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteNewAddedCommandsIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteNewAddedCommandsIT.java index e4661ed95a5..c8a8a2c8384 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteNewAddedCommandsIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteNewAddedCommandsIT.java @@ -14,28 +14,4 @@ public void init() throws Exception { enableCalcite(); disallowCalciteFallback(); } - - @Override - public void testJoin() { - // fallback disabled, should pass - super.testJoin(); - // fallback enabled, should pass too - withFallbackEnabled(super::testJoin, ""); - } - - @Override - public void testLookup() { - // fallback disabled, should pass - super.testLookup(); - // fallback enabled, should pass too - withFallbackEnabled(super::testLookup, ""); - } - - @Override - public void testSubsearch() { - // fallback disabled, should pass - super.testSubsearch(); - // fallback enabled, should pass too - withFallbackEnabled(super::testSubsearch, ""); - } } From b9cbb9ba63cc8f7ec0e46e71233feb0795aa3f94 Mon Sep 17 00:00:00 2001 From: Lantao Jin Date: Tue, 22 Apr 2025 20:38:01 +0800 Subject: [PATCH 3/5] address comments Signed-off-by: Lantao Jin --- .../opensearch/sql/executor/QueryService.java | 35 +++- .../sql/ppl/NewAddedCommandsIT.java | 159 ++++++++---------- .../opensearch/sql/ppl/PPLIntegTestCase.java | 32 ++-- .../executor/OpenSearchExecutionEngine.java | 2 +- 4 files changed, 109 insertions(+), 119 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/executor/QueryService.java b/core/src/main/java/org/opensearch/sql/executor/QueryService.java index 0d9fc4c762e..c579fa14f85 100644 --- a/core/src/main/java/org/opensearch/sql/executor/QueryService.java +++ b/core/src/main/java/org/opensearch/sql/executor/QueryService.java @@ -11,6 +11,7 @@ import java.security.AccessController; import java.security.PrivilegedAction; import java.util.List; +import java.util.Optional; import lombok.AllArgsConstructor; import lombok.Getter; import lombok.RequiredArgsConstructor; @@ -67,7 +68,7 @@ public void execute( if (shouldUseCalcite(queryType)) { executeWithCalcite(plan, queryType, listener); } else { - executeWithLegacy(plan, queryType, listener); + executeWithLegacy(plan, queryType, listener, Optional.empty()); } } @@ -80,7 +81,7 @@ public void explain( if (shouldUseCalcite(queryType)) { explainWithCalcite(plan, queryType, listener, format); } else { - explainWithLegacy(plan, queryType, listener); + explainWithLegacy(plan, queryType, listener, Optional.empty()); } } @@ -103,7 +104,7 @@ public void executeWithCalcite( } catch (Throwable t) { if (isCalciteFallbackAllowed()) { log.warn("Fallback to V2 query engine since got exception", t); - executeWithLegacy(plan, queryType, listener); + executeWithLegacy(plan, queryType, listener, Optional.of(t)); } else { if (t instanceof Error) { // Calcite may throw AssertError during query execution. @@ -135,7 +136,7 @@ public void explainWithCalcite( } catch (Throwable t) { if (isCalciteFallbackAllowed()) { log.warn("Fallback to V2 query engine since got exception", t); - explainWithLegacy(plan, queryType, listener); + explainWithLegacy(plan, queryType, listener, Optional.of(t)); } else { if (t instanceof Error) { // Calcite may throw AssertError during query execution. @@ -150,11 +151,19 @@ public void explainWithCalcite( public void executeWithLegacy( UnresolvedPlan plan, QueryType queryType, - ResponseListener listener) { + ResponseListener listener, + Optional calciteFailure) { try { executePlan(analyze(plan, queryType), PlanContext.emptyPlanContext(), listener); } catch (Exception e) { - listener.onFailure(e); + if (shouldUseCalcite(queryType) && isCalciteFallbackAllowed()) { + // if there is a failure thrown from Calcite and execution after fallback V2 + // keeps failure, we should throw the failure from Calcite. + calciteFailure.ifPresentOrElse( + t -> listener.onFailure(new RuntimeException(t)), () -> listener.onFailure(e)); + } else { + listener.onFailure(e); + } } } @@ -163,16 +172,26 @@ public void executeWithLegacy( * explain response. * * @param plan {@link UnresolvedPlan} + * @param queryType {@link QueryType} * @param listener {@link ResponseListener} for explain response + * @param calciteFailure Optional failure thrown from calcite */ public void explainWithLegacy( UnresolvedPlan plan, QueryType queryType, - ResponseListener listener) { + ResponseListener listener, + Optional calciteFailure) { try { executionEngine.explain(plan(analyze(plan, queryType)), listener); } catch (Exception e) { - listener.onFailure(e); + if (shouldUseCalcite(queryType) && isCalciteFallbackAllowed()) { + // if there is a failure thrown from Calcite and execution after fallback V2 + // keeps failure, we should throw the failure from Calcite. + calciteFailure.ifPresentOrElse( + t -> listener.onFailure(new RuntimeException(t)), () -> listener.onFailure(e)); + } else { + listener.onFailure(e); + } } } diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/NewAddedCommandsIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/NewAddedCommandsIT.java index 95072e5766d..d05db6c09df 100644 --- a/integ-test/src/test/java/org/opensearch/sql/ppl/NewAddedCommandsIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/NewAddedCommandsIT.java @@ -26,111 +26,94 @@ public void init() throws Exception { } @Test - public void testJoin() { + public void testJoin() throws IOException { + JSONObject result; try { - JSONObject result; - try { - result = - executeQuery( - String.format( - "search source=%s | join on firstname=holdersName %s", - TEST_INDEX_BANK, TEST_INDEX_DOG)); - } catch (ResponseException e) { - result = new JSONObject(TestUtils.getResponseBody(e.getResponse())); - verifyQuery(result); - } - } catch (IOException e) { - fail(e.getMessage()); + result = + executeQuery( + String.format( + "search source=%s | join on firstname=holdersName %s", + TEST_INDEX_BANK, TEST_INDEX_DOG)); + } catch (ResponseException e) { + result = new JSONObject(TestUtils.getResponseBody(e.getResponse())); + verifyQuery(result); } } @Test - public void testLookup() { + public void testLookup() throws IOException { + JSONObject result; try { - JSONObject result; - try { - result = - executeQuery( - String.format( - "search source=%s | lookup %s holdersName as firstname", - TEST_INDEX_BANK, TEST_INDEX_DOG)); - } catch (ResponseException e) { - result = new JSONObject(TestUtils.getResponseBody(e.getResponse())); - } - verifyQuery(result); - } catch (IOException e) { - fail(e.getMessage()); + result = + executeQuery( + String.format( + "search source=%s | lookup %s holdersName as firstname", + TEST_INDEX_BANK, TEST_INDEX_DOG)); + } catch (ResponseException e) { + result = new JSONObject(TestUtils.getResponseBody(e.getResponse())); } + verifyQuery(result); } @Test - public void testSubsearch() { + public void testSubsearch() throws IOException { + JSONObject result; try { - JSONObject result; - try { - result = - executeQuery( - String.format( - "search source=[source=%s | where age>35] as t | where age>35", - TEST_INDEX_BANK)); - } catch (ResponseException e) { - result = new JSONObject(TestUtils.getResponseBody(e.getResponse())); - } - verifyQuery(result); + result = + executeQuery( + String.format( + "search source=[source=%s | where age>35] as t | where age>35", TEST_INDEX_BANK)); + } catch (ResponseException e) { + result = new JSONObject(TestUtils.getResponseBody(e.getResponse())); + } + verifyQuery(result); - try { - result = - executeQuery( - String.format( - "search source=%s | where exists [ source=%s | where firstname=holdersName]", - TEST_INDEX_BANK, TEST_INDEX_DOG)); - } catch (ResponseException e) { - result = new JSONObject(TestUtils.getResponseBody(e.getResponse())); - } - verifyQuery(result); + try { + result = + executeQuery( + String.format( + "search source=%s | where exists [ source=%s | where firstname=holdersName]", + TEST_INDEX_BANK, TEST_INDEX_DOG)); + } catch (ResponseException e) { + result = new JSONObject(TestUtils.getResponseBody(e.getResponse())); + } + verifyQuery(result); - try { - result = - executeQuery( - String.format( - "search source=%s | where firstname in [ source=%s | fields holdersName]", - TEST_INDEX_BANK, TEST_INDEX_DOG)); - } catch (ResponseException e) { - result = new JSONObject(TestUtils.getResponseBody(e.getResponse())); - } - verifyQuery(result); + try { + result = + executeQuery( + String.format( + "search source=%s | where firstname in [ source=%s | fields holdersName]", + TEST_INDEX_BANK, TEST_INDEX_DOG)); + } catch (ResponseException e) { + result = new JSONObject(TestUtils.getResponseBody(e.getResponse())); + } + verifyQuery(result); - try { - result = - executeQuery( - String.format( - "search source=%s | where firstname = [ source=%s | where holdersName='Hattie'" - + " | fields holdersName | head 1]", - TEST_INDEX_BANK, TEST_INDEX_DOG)); - } catch (ResponseException e) { - result = new JSONObject(TestUtils.getResponseBody(e.getResponse())); - } - verifyQuery(result); - } catch (IOException e) { - fail(e.getMessage()); + try { + result = + executeQuery( + String.format( + "search source=%s | where firstname = [ source=%s | where holdersName='Hattie'" + + " | fields holdersName | head 1]", + TEST_INDEX_BANK, TEST_INDEX_DOG)); + } catch (ResponseException e) { + result = new JSONObject(TestUtils.getResponseBody(e.getResponse())); } + verifyQuery(result); } - private void verifyQuery(JSONObject result) { - try { - if (isCalciteEnabled()) { - assertFalse(result.getJSONArray("datarows").isEmpty()); - } else { - assertThat(result.getInt("status"), equalTo(500)); - JSONObject error = result.getJSONObject("error"); - assertThat( - error.getString("details"), - containsString( - "is supported only when " + CALCITE_ENGINE_ENABLED.getKeyValue() + "=true")); - assertThat(error.getString("type"), equalTo("UnsupportedOperationException")); - } - } catch (IOException e) { - fail(e.getMessage()); + private void verifyQuery(JSONObject result) throws IOException { + if (isCalciteEnabled()) { + assertFalse(result.getJSONArray("datarows").isEmpty()); + } else { + assertThat(result.getInt("status"), equalTo(500)); + JSONObject error = result.getJSONObject("error"); + assertThat( + error.getString("details"), + containsString( + "is supported only when " + CALCITE_ENGINE_ENABLED.getKeyValue() + "=true")); + assertThat(error.getString("type"), equalTo("UnsupportedOperationException")); } } } diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java index b22ec4ce5c1..948b0733723 100644 --- a/integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java @@ -147,39 +147,27 @@ public static void disallowCalciteFallback() throws IOException { LOG.info("{} disabled", Settings.Key.CALCITE_FALLBACK_ALLOWED.name()); } - protected static boolean isFallbackEnabled() { - try { - return Boolean.parseBoolean( - getClusterSetting(Settings.Key.CALCITE_FALLBACK_ALLOWED.getKeyValue(), "persistent")); - } catch (IOException e) { - throw new RuntimeException(e); - } + protected static boolean isFallbackEnabled() throws IOException { + return Boolean.parseBoolean( + getClusterSetting(Settings.Key.CALCITE_FALLBACK_ALLOWED.getKeyValue(), "persistent")); } - public static void withFallbackEnabled(Runnable f, String msg) { + public static void withFallbackEnabled(Runnable f, String msg) throws IOException { LOG.info("Need fallback to v2 due to {}", msg); boolean isFallbackEnabled = isFallbackEnabled(); if (isFallbackEnabled) f.run(); else { try { - try { - updateClusterSettings( - new SQLIntegTestCase.ClusterSetting( - "persistent", Settings.Key.CALCITE_FALLBACK_ALLOWED.getKeyValue(), "true")); - } catch (IOException e) { - throw new RuntimeException(e); - } + updateClusterSettings( + new SQLIntegTestCase.ClusterSetting( + "persistent", Settings.Key.CALCITE_FALLBACK_ALLOWED.getKeyValue(), "true")); LOG.info( "Set {} to enabled and run the test", Settings.Key.CALCITE_FALLBACK_ALLOWED.name()); f.run(); } finally { - try { - updateClusterSettings( - new SQLIntegTestCase.ClusterSetting( - "persistent", Settings.Key.CALCITE_FALLBACK_ALLOWED.getKeyValue(), "false")); - } catch (IOException e) { - throw new RuntimeException(e); - } + updateClusterSettings( + new SQLIntegTestCase.ClusterSetting( + "persistent", Settings.Key.CALCITE_FALLBACK_ALLOWED.getKeyValue(), "false")); LOG.info("Reset {} back to disabled", Settings.Key.CALCITE_FALLBACK_ALLOWED.name()); } } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java index 60998894954..54452a04e81 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java @@ -177,7 +177,7 @@ public void execute( ResultSet result = statement.executeQuery(); buildResultSet(result, rel.getRowType(), listener); } catch (SQLException e) { - listener.onFailure(e); + throw new RuntimeException(e); } return null; })); From 8e25b66ee0eef8ed51eb709662107fd8a5b08450 Mon Sep 17 00:00:00 2001 From: Lantao Jin Date: Tue, 22 Apr 2025 21:55:31 +0800 Subject: [PATCH 4/5] fix UT Signed-off-by: Lantao Jin --- .../test/java/org/opensearch/sql/ppl/NewAddedCommandsIT.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/NewAddedCommandsIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/NewAddedCommandsIT.java index d05db6c09df..72585477d44 100644 --- a/integ-test/src/test/java/org/opensearch/sql/ppl/NewAddedCommandsIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/NewAddedCommandsIT.java @@ -62,7 +62,7 @@ public void testSubsearch() throws IOException { result = executeQuery( String.format( - "search source=[source=%s | where age>35] as t | where age>35", TEST_INDEX_BANK)); + "search source=[source=%s | where age>35 | fields age] as t", TEST_INDEX_BANK)); } catch (ResponseException e) { result = new JSONObject(TestUtils.getResponseBody(e.getResponse())); } @@ -107,7 +107,6 @@ private void verifyQuery(JSONObject result) throws IOException { if (isCalciteEnabled()) { assertFalse(result.getJSONArray("datarows").isEmpty()); } else { - assertThat(result.getInt("status"), equalTo(500)); JSONObject error = result.getJSONObject("error"); assertThat( error.getString("details"), From 879240c85f67303fbc991eb48a0923c517099330 Mon Sep 17 00:00:00 2001 From: Lantao Jin Date: Thu, 24 Apr 2025 14:58:02 +0800 Subject: [PATCH 5/5] Add a new IT case Signed-off-by: Lantao Jin --- .../calcite/standalone/CalcitePPLBasicIT.java | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLBasicIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLBasicIT.java index 4329b1dbd66..b65613a27e2 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLBasicIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLBasicIT.java @@ -524,6 +524,25 @@ public void testXor() { verifyDataRows(result, rows("Elinor", 36)); } + @Test + public void testKeepThrowCalciteException() throws IOException { + withFallbackEnabled( + () -> { + IllegalArgumentException e = + assertThrows( + IllegalArgumentException.class, + () -> + executeQuery( + String.format("source=%s | fields firstname1, age", TEST_INDEX_BANK))); + verifyErrorMessageContains( + e, + "field [firstname1] not found; input fields are: [account_number, firstname, address," + + " birthdate, gender, city, lastname, balance, employer, state, age, email," + + " male, _id, _index, _score, _maxscore, _sort, _routing]"); + }, + ""); + } + @Test public void testAliasDataType() { JSONObject result =