Skip to content

Commit 74f3f80

Browse files
dai-chendai-chen
committed
Fix analytics engine routing for SQL after V2 parser change
The switch from CalciteSqlQueryParser to SqlV2QueryParser (#5438) changed the parse return type from SqlNode to UnresolvedPlan. This caused a ClassCastException in extractIndexName(), which was silently caught and returned false, routing all SQL queries to V2 engine instead of analytics. Fix: unify SQL and PPL paths to both use UnresolvedPlan + IndexNameExtractor. Remove dead SqlNode-based extraction code (SqlTableNameExtractor). Signed-off-by: Chen Dai <daichen@amazon.com>
1 parent 5851bdb commit 74f3f80

2 files changed

Lines changed: 72 additions & 56 deletions

File tree

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

Lines changed: 2 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,6 @@
1313
import java.util.Map;
1414
import java.util.Optional;
1515
import org.apache.calcite.rel.RelNode;
16-
import org.apache.calcite.sql.SqlCall;
17-
import org.apache.calcite.sql.SqlIdentifier;
18-
import org.apache.calcite.sql.SqlJoin;
19-
import org.apache.calcite.sql.SqlNode;
20-
import org.apache.calcite.sql.SqlSelect;
21-
import org.apache.calcite.sql.util.SqlBasicVisitor;
2216
import org.apache.logging.log4j.LogManager;
2317
import org.apache.logging.log4j.Logger;
2418
import org.apache.logging.log4j.ThreadContext;
@@ -213,12 +207,8 @@ private UnifiedQueryContext.Builder applyClusterOverrides(UnifiedQueryContext.Bu
213207
*/
214208
private static Optional<String> extractIndexName(
215209
String query, QueryType queryType, UnifiedQueryContext context) {
216-
if (queryType == QueryType.PPL) {
217-
UnresolvedPlan unresolvedPlan = (UnresolvedPlan) context.getParser().parse(query);
218-
return Optional.ofNullable(unresolvedPlan.accept(new IndexNameExtractor(), null));
219-
}
220-
SqlNode sqlNode = (SqlNode) context.getParser().parse(query);
221-
return Optional.ofNullable(extractTableNameFromSqlNode(sqlNode));
210+
UnresolvedPlan unresolvedPlan = (UnresolvedPlan) context.getParser().parse(query);
211+
return Optional.ofNullable(unresolvedPlan.accept(new IndexNameExtractor(), null));
222212
}
223213

224214
/** AST visitor that extracts the source index name from a Relation node (PPL path). */
@@ -229,29 +219,6 @@ public String visitRelation(Relation node, Void context) {
229219
}
230220
}
231221

232-
/** SqlNode visitor that extracts the source table name from a SQL parse tree. */
233-
private static class SqlTableNameExtractor extends SqlBasicVisitor<String> {
234-
@Override
235-
public String visit(SqlCall call) {
236-
if (call instanceof SqlSelect select) {
237-
return select.getFrom().accept(this);
238-
}
239-
if (call instanceof SqlJoin join) {
240-
return join.getLeft().accept(this);
241-
}
242-
return null;
243-
}
244-
245-
@Override
246-
public String visit(SqlIdentifier id) {
247-
return id.toString();
248-
}
249-
}
250-
251-
private static String extractTableNameFromSqlNode(SqlNode sqlNode) {
252-
return sqlNode.accept(new SqlTableNameExtractor());
253-
}
254-
255222
private static RelNode addQuerySizeLimit(RelNode plan, CalcitePlanContext context) {
256223
return LogicalSystemLimit.create(
257224
LogicalSystemLimit.SystemLimitType.QUERY_SIZE_LIMIT,

‎plugin/src/test/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryActionTest.java‎

Lines changed: 70 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import static org.mockito.Mockito.mock;
1111
import static org.mockito.Mockito.when;
1212

13+
import java.util.function.Consumer;
1314
import org.apache.calcite.rel.RelNode;
1415
import org.junit.Before;
1516
import org.junit.Test;
@@ -52,47 +53,95 @@ public void setUp() {
5253
}
5354

5455
@Test
55-
public void pluggableDataformatIndexRoutesToAnalytics() {
56-
registerIndex(
56+
public void pplQueryRoutesToAnalyticsForParquetIndex() {
57+
withParquetIndex(
5758
"parquet_logs",
58-
Settings.builder()
59-
.put(IndexSettings.PLUGGABLE_DATAFORMAT_ENABLED_SETTING.getKey(), true)
60-
.put(IndexSettings.PLUGGABLE_DATAFORMAT_VALUE_SETTING.getKey(), "composite")
61-
.build());
59+
index -> {
60+
assertTrue(action.isAnalyticsIndex("source = parquet_logs | fields ts", QueryType.PPL));
61+
assertTrue(
62+
action.isAnalyticsIndex(
63+
"source = opensearch.parquet_logs | fields ts", QueryType.PPL));
64+
});
65+
}
6266

63-
assertTrue(action.isAnalyticsIndex("source = parquet_logs | fields ts", QueryType.PPL));
64-
assertTrue(
65-
action.isAnalyticsIndex("source = opensearch.parquet_logs | fields ts", QueryType.PPL));
67+
@Test
68+
public void sqlQueryRoutesToAnalyticsForParquetIndex() {
69+
withParquetIndex(
70+
"parquet_logs",
71+
index -> {
72+
assertTrue(action.isAnalyticsIndex("SELECT * FROM parquet_logs", QueryType.SQL));
73+
assertTrue(
74+
action.isAnalyticsIndex(
75+
"SELECT ts, level FROM parquet_logs WHERE level = 'ERROR'", QueryType.SQL));
76+
assertTrue(
77+
action.isAnalyticsIndex("SELECT * FROM opensearch.parquet_logs", QueryType.SQL));
78+
});
6679
}
6780

6881
@Test
69-
public void pluggableEnabledButLuceneFormatRoutesToLucene() {
70-
registerIndex(
82+
public void pplQueryRoutesToLuceneForLuceneIndex() {
83+
withLuceneIndex(
7184
"lucene_logs",
72-
Settings.builder()
73-
.put(IndexSettings.PLUGGABLE_DATAFORMAT_ENABLED_SETTING.getKey(), true)
74-
.put(IndexSettings.PLUGGABLE_DATAFORMAT_VALUE_SETTING.getKey(), "lucene")
75-
.build());
76-
77-
assertFalse(action.isAnalyticsIndex("source = lucene_logs | fields ts", QueryType.PPL));
85+
index -> {
86+
assertFalse(action.isAnalyticsIndex("source = lucene_logs | fields ts", QueryType.PPL));
87+
});
7888
}
7989

8090
@Test
81-
public void indexWithoutSettingRoutesToLucene() {
82-
registerIndex("plain_logs", Settings.EMPTY);
83-
84-
assertFalse(action.isAnalyticsIndex("source = plain_logs | fields ts", QueryType.PPL));
91+
public void sqlQueryRoutesToLuceneForLuceneIndex() {
92+
withLuceneIndex(
93+
"plain_logs",
94+
index -> {
95+
assertFalse(action.isAnalyticsIndex("SELECT * FROM plain_logs", QueryType.SQL));
96+
});
8597
}
8698

8799
@Test
88100
public void missingIndexRoutesToLucene() {
89101
assertFalse(action.isAnalyticsIndex("source = does_not_exist | fields ts", QueryType.PPL));
102+
assertFalse(action.isAnalyticsIndex("SELECT * FROM does_not_exist", QueryType.SQL));
90103
}
91104

92105
@Test
93106
public void nullAndEmptyQueriesRouteToLucene() {
94107
assertFalse(action.isAnalyticsIndex(null, QueryType.PPL));
95108
assertFalse(action.isAnalyticsIndex("", QueryType.PPL));
109+
assertFalse(action.isAnalyticsIndex(null, QueryType.SQL));
110+
assertFalse(action.isAnalyticsIndex("", QueryType.SQL));
111+
}
112+
113+
@Test
114+
public void showStatementRoutesToLucene() {
115+
withParquetIndex(
116+
"parquet_logs",
117+
index -> {
118+
assertFalse(action.isAnalyticsIndex("SHOW TABLES LIKE 'parquet_logs'", QueryType.SQL));
119+
});
120+
}
121+
122+
@Test
123+
public void describeStatementRoutesToLucene() {
124+
withParquetIndex(
125+
"parquet_logs",
126+
index -> {
127+
assertFalse(
128+
action.isAnalyticsIndex("DESCRIBE TABLES LIKE 'parquet_logs'", QueryType.SQL));
129+
});
130+
}
131+
132+
private void withParquetIndex(String name, Consumer<String> assertions) {
133+
registerIndex(
134+
name,
135+
Settings.builder()
136+
.put(IndexSettings.PLUGGABLE_DATAFORMAT_ENABLED_SETTING.getKey(), true)
137+
.put(IndexSettings.PLUGGABLE_DATAFORMAT_VALUE_SETTING.getKey(), "composite")
138+
.build());
139+
assertions.accept(name);
140+
}
141+
142+
private void withLuceneIndex(String name, Consumer<String> assertions) {
143+
registerIndex(name, Settings.EMPTY);
144+
assertions.accept(name);
96145
}
97146

98147
private void registerIndex(String name, Settings settings) {

0 commit comments

Comments
 (0)