Skip to content

Commit ccd1665

Browse files
ahkcsahkcs
committed
Add error classification and expand integration test coverage
- Classify client vs server errors in RestUnifiedQueryAction (syntax errors return 400, engine failures return 500) - Fix JSON escaping in error responses for special characters - Add integration tests: explain with filter/aggregation, syntax error handling, response format validation, regression checks - Increment correct metrics (PPL_FAILED_REQ_COUNT_CUS vs SYS) Signed-off-by: Kai Huang <kaihuang@amazon.com> Signed-off-by: Kai Huang <ahkcs@amazon.com>
1 parent 3b1ecb8 commit ccd1665

3 files changed

Lines changed: 134 additions & 32 deletions

File tree

‎integ-test/src/test/java/org/opensearch/sql/ppl/AnalyticsPPLIT.java‎

Lines changed: 99 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,16 @@
66
package org.opensearch.sql.ppl;
77

88
import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_ACCOUNT;
9+
import static org.opensearch.sql.util.MatcherUtils.rows;
910
import static org.opensearch.sql.util.MatcherUtils.schema;
11+
import static org.opensearch.sql.util.MatcherUtils.verifyDataRows;
1012
import static org.opensearch.sql.util.MatcherUtils.verifyNumOfRows;
1113
import static org.opensearch.sql.util.MatcherUtils.verifySchema;
1214

1315
import java.io.IOException;
1416
import org.json.JSONObject;
1517
import org.junit.Test;
18+
import org.opensearch.client.ResponseException;
1619

1720
/**
1821
* Integration tests for PPL queries routed through the analytics engine path (Project Mustang).
@@ -22,10 +25,10 @@
2225
* <p>These tests validate the full pipeline: REST request → routing → planning via
2326
* UnifiedQueryPlanner → execution via AnalyticsExecutionEngine → response formatting.
2427
*
25-
* <p>Note: The stub executor ignores the logical plan and always returns all rows for the matched
26-
* table. Tests that use projection (| fields) or filter (| where) will still get all rows back with
27-
* the projected schema, but the data values may not match the projected columns since the stub does
28-
* not actually evaluate the plan.
28+
* <p>The stub executor always returns the full table rows regardless of the logical plan. After
29+
* projection (| fields), the execution engine maps row values by position — so projected columns
30+
* get the values from the corresponding positions in the full row, not the actual projected column.
31+
* This is expected behavior for a stub; the real analytics engine will evaluate the plan correctly.
2932
*/
3033
public class AnalyticsPPLIT extends PPLIntegTestCase {
3134

@@ -35,8 +38,10 @@ protected void init() throws Exception {
3538
// in RestUnifiedQueryAction and StubQueryPlanExecutor
3639
}
3740

41+
// --- Full table scan tests with schema + data verification ---
42+
3843
@Test
39-
public void testBasicQueryReturnsResults() throws IOException {
44+
public void testBasicQuerySchemaAndData() throws IOException {
4045
JSONObject result = executeQuery("source = opensearch.parquet_logs");
4146
verifySchema(
4247
result,
@@ -45,10 +50,16 @@ public void testBasicQueryReturnsResults() throws IOException {
4550
schema("message", "keyword"),
4651
schema("ip_addr", "keyword"));
4752
verifyNumOfRows(result, 3);
53+
// Verify actual row data from StubQueryPlanExecutor (timestamps in UTC)
54+
verifyDataRows(
55+
result,
56+
rows("2024-01-15 10:30:00", 200, "Request completed", "192.168.1.1"),
57+
rows("2024-01-15 10:31:00", 200, "Health check OK", "192.168.1.2"),
58+
rows("2024-01-15 10:32:00", 500, "Internal server error", "192.168.1.3"));
4859
}
4960

5061
@Test
51-
public void testParquetMetricsTable() throws IOException {
62+
public void testParquetMetricsSchemaAndData() throws IOException {
5263
JSONObject result = executeQuery("source = opensearch.parquet_metrics");
5364
verifySchema(
5465
result,
@@ -57,43 +68,110 @@ public void testParquetMetricsTable() throws IOException {
5768
schema("memory", "double"),
5869
schema("host", "keyword"));
5970
verifyNumOfRows(result, 2);
71+
verifyDataRows(
72+
result,
73+
rows("2024-01-15 10:30:00", 75.5, 8192.5, "host-1"),
74+
rows("2024-01-15 10:31:00", 82.3, 7680.5, "host-2"));
6075
}
6176

77+
// --- Response format validation ---
78+
6279
@Test
6380
public void testResponseFormatHasRequiredFields() throws IOException {
6481
JSONObject result = executeQuery("source = opensearch.parquet_logs");
65-
assertTrue(result.has("schema"));
66-
assertTrue(result.has("datarows"));
67-
assertTrue(result.has("total"));
68-
assertTrue(result.has("size"));
69-
assertTrue(result.has("status"));
82+
assertTrue("Response should have 'schema' field", result.has("schema"));
83+
assertTrue("Response should have 'datarows' field", result.has("datarows"));
84+
assertTrue("Response should have 'total' field", result.has("total"));
85+
assertTrue("Response should have 'size' field", result.has("size"));
86+
assertTrue("Response should have 'status' field", result.has("status"));
7087
assertEquals(200, result.getInt("status"));
7188
}
7289

7390
@Test
74-
public void testExplainQuery() throws IOException {
75-
String explainResult =
76-
explainQueryToString("source = opensearch.parquet_logs | fields ts, message");
77-
assertTrue(explainResult.contains("LogicalProject"));
78-
assertTrue(explainResult.contains("LogicalTableScan"));
91+
public void testTotalAndSizeMatchRowCount() throws IOException {
92+
JSONObject result = executeQuery("source = opensearch.parquet_logs");
93+
int rowCount = result.getJSONArray("datarows").length();
94+
assertEquals(rowCount, result.getInt("total"));
95+
assertEquals(rowCount, result.getInt("size"));
7996
}
8097

98+
// --- Projection tests (schema verification only — stub doesn't evaluate projections) ---
99+
81100
@Test
82101
public void testFieldsProjectionChangesSchema() throws IOException {
83102
JSONObject result = executeQuery("source = opensearch.parquet_logs | fields ts, message");
84-
// Schema should only have the projected columns
85103
verifySchema(result, schema("ts", "timestamp"), schema("message", "keyword"));
86-
// Stub returns all rows (it ignores the plan), but the row data is projected
87-
// by column position, so row count should still be 3
88104
verifyNumOfRows(result, 3);
89105
}
90106

107+
@Test
108+
public void testSingleFieldProjection() throws IOException {
109+
JSONObject result = executeQuery("source = opensearch.parquet_logs | fields status");
110+
verifySchema(result, schema("status", "integer"));
111+
verifyNumOfRows(result, 3);
112+
}
113+
114+
// --- Explain tests ---
115+
116+
@Test
117+
public void testExplainBasicQuery() throws IOException {
118+
String explainResult =
119+
explainQueryToString("source = opensearch.parquet_logs | fields ts, message");
120+
assertTrue("Explain should contain LogicalProject", explainResult.contains("LogicalProject"));
121+
assertTrue(
122+
"Explain should contain LogicalTableScan", explainResult.contains("LogicalTableScan"));
123+
assertTrue(
124+
"Explain should reference parquet_logs table", explainResult.contains("parquet_logs"));
125+
}
126+
127+
@Test
128+
public void testExplainFilterQuery() throws IOException {
129+
String explainResult =
130+
explainQueryToString(
131+
"source = opensearch.parquet_logs | where status = 200 | fields ts, message");
132+
assertTrue("Explain should contain LogicalFilter", explainResult.contains("LogicalFilter"));
133+
assertTrue("Explain should contain LogicalProject", explainResult.contains("LogicalProject"));
134+
}
135+
136+
@Test
137+
public void testExplainAggregation() throws IOException {
138+
String explainResult =
139+
explainQueryToString("source = opensearch.parquet_logs | stats count() by status");
140+
assertTrue(
141+
"Explain should contain LogicalAggregate", explainResult.contains("LogicalAggregate"));
142+
}
143+
144+
// --- Error handling tests ---
145+
146+
@Test
147+
public void testSyntaxErrorReturnsClientError() {
148+
ResponseException e =
149+
assertThrows(
150+
ResponseException.class,
151+
() -> executeQuery("source = opensearch.parquet_logs | invalid_command"));
152+
int statusCode = e.getResponse().getStatusLine().getStatusCode();
153+
assertTrue(
154+
"Syntax error should return 4xx, got " + statusCode, statusCode >= 400 && statusCode < 500);
155+
}
156+
157+
// --- Regression tests ---
158+
91159
@Test
92160
public void testNonParquetQueryStillWorks() throws IOException {
93161
loadIndex(Index.ACCOUNT);
94162
JSONObject result =
95163
executeQuery(String.format("source=%s | head 1 | fields firstname", TEST_INDEX_ACCOUNT));
96-
assertNotNull(result);
97-
assertTrue(result.has("datarows"));
164+
assertNotNull("Non-parquet query should return results", result);
165+
assertTrue("Non-parquet query should have datarows", result.has("datarows"));
166+
assertTrue(
167+
"Non-parquet query should return data", result.getJSONArray("datarows").length() > 0);
168+
}
169+
170+
@Test
171+
public void testNonParquetAggregationStillWorks() throws IOException {
172+
loadIndex(Index.ACCOUNT);
173+
JSONObject result =
174+
executeQuery(String.format("source=%s | stats count()", TEST_INDEX_ACCOUNT));
175+
assertTrue("Non-parquet aggregation should work", result.getInt("total") > 0);
98176
}
99177
}

‎plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java‎

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,13 +25,16 @@
2525
import org.apache.logging.log4j.LogManager;
2626
import org.apache.logging.log4j.Logger;
2727
import org.opensearch.common.unit.TimeValue;
28+
import org.opensearch.core.rest.RestStatus;
2829
import org.opensearch.rest.BytesRestResponse;
2930
import org.opensearch.rest.RestChannel;
3031
import org.opensearch.sql.api.UnifiedQueryContext;
3132
import org.opensearch.sql.api.UnifiedQueryPlanner;
3233
import org.opensearch.sql.ast.statement.ExplainMode;
3334
import org.opensearch.sql.calcite.CalcitePlanContext;
35+
import org.opensearch.sql.common.antlr.SyntaxCheckException;
3436
import org.opensearch.sql.common.response.ResponseListener;
37+
import org.opensearch.sql.exception.SemanticCheckException;
3538
import org.opensearch.sql.executor.QueryType;
3639
import org.opensearch.sql.executor.analytics.AnalyticsExecutionEngine;
3740
import org.opensearch.sql.executor.analytics.QueryPlanExecutor;
@@ -255,20 +258,41 @@ public RelDataType getRowType(RelDataTypeFactory typeFactory) {
255258
};
256259
}
257260

261+
/** Classify whether the exception is a client error (bad query) or server error (engine bug). */
262+
private static boolean isClientError(Exception e) {
263+
return e instanceof SyntaxCheckException
264+
|| e instanceof SemanticCheckException
265+
|| e instanceof IllegalArgumentException
266+
|| e instanceof NullPointerException;
267+
}
268+
258269
private static void recordFailureMetric(Exception e) {
259-
LOG.error("[unified] Query execution failed", e);
260-
Metrics.getInstance().getNumericalMetric(MetricName.PPL_FAILED_REQ_COUNT_SYS).increment();
270+
if (isClientError(e)) {
271+
LOG.warn("[unified] Client error in query execution", e);
272+
Metrics.getInstance().getNumericalMetric(MetricName.PPL_FAILED_REQ_COUNT_CUS).increment();
273+
} else {
274+
LOG.error("[unified] Server error in query execution", e);
275+
Metrics.getInstance().getNumericalMetric(MetricName.PPL_FAILED_REQ_COUNT_SYS).increment();
276+
}
261277
}
262278

263279
private static void reportError(RestChannel channel, Exception e) {
280+
RestStatus status =
281+
isClientError(e) ? RestStatus.BAD_REQUEST : RestStatus.INTERNAL_SERVER_ERROR;
282+
String reason = e.getMessage() != null ? e.getMessage() : "Unknown error";
283+
// Escape characters that would break JSON
284+
reason =
285+
reason.replace("\\", "\\\\").replace("\"", "\\\"").replace("\n", "\\n").replace("\r", "");
264286
channel.sendResponse(
265287
new BytesRestResponse(
266-
org.opensearch.core.rest.RestStatus.INTERNAL_SERVER_ERROR,
288+
status,
267289
"application/json; charset=UTF-8",
268290
"{\"error\":{\"type\":\""
269291
+ e.getClass().getSimpleName()
270292
+ "\",\"reason\":\""
271-
+ (e.getMessage() != null ? e.getMessage().replace("\"", "\\\"") : "Unknown error")
272-
+ "\"},\"status\":500}"));
293+
+ reason
294+
+ "\"},\"status\":"
295+
+ status.getStatus()
296+
+ "}"));
273297
}
274298
}

‎plugin/src/main/java/org/opensearch/sql/plugin/rest/StubQueryPlanExecutor.java‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55

66
package org.opensearch.sql.plugin.rest;
77

8-
import java.sql.Timestamp;
8+
import java.time.Instant;
99
import java.util.List;
1010
import org.apache.calcite.plan.RelOptUtil;
1111
import org.apache.calcite.rel.RelNode;
@@ -29,19 +29,19 @@ public Iterable<Object[]> execute(RelNode plan, Object context) {
2929
if (tableName != null && tableName.contains("parquet_logs")) {
3030
return List.of(
3131
new Object[] {
32-
Timestamp.valueOf("2024-01-15 10:30:00"), 200, "Request completed", "192.168.1.1"
32+
Instant.parse("2024-01-15T10:30:00Z"), 200, "Request completed", "192.168.1.1"
3333
},
3434
new Object[] {
35-
Timestamp.valueOf("2024-01-15 10:31:00"), 200, "Health check OK", "192.168.1.2"
35+
Instant.parse("2024-01-15T10:31:00Z"), 200, "Health check OK", "192.168.1.2"
3636
},
3737
new Object[] {
38-
Timestamp.valueOf("2024-01-15 10:32:00"), 500, "Internal server error", "192.168.1.3"
38+
Instant.parse("2024-01-15T10:32:00Z"), 500, "Internal server error", "192.168.1.3"
3939
});
4040
}
4141
if (tableName != null && tableName.contains("parquet_metrics")) {
4242
return List.of(
43-
new Object[] {Timestamp.valueOf("2024-01-15 10:30:00"), 75.5, 8192.0, "host-1"},
44-
new Object[] {Timestamp.valueOf("2024-01-15 10:31:00"), 82.3, 7680.0, "host-2"});
43+
new Object[] {Instant.parse("2024-01-15T10:30:00Z"), 75.5, 8192.5, "host-1"},
44+
new Object[] {Instant.parse("2024-01-15T10:31:00Z"), 82.3, 7680.5, "host-2"});
4545
}
4646
return List.of();
4747
}

0 commit comments

Comments
 (0)