Skip to content

Commit 7ddc3f0

Browse files
mengweiericmengweieric
committed
a few mode cleanup
Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
1 parent daf326e commit 7ddc3f0

5 files changed

Lines changed: 81 additions & 66 deletions

File tree

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

Lines changed: 19 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
import static org.opensearch.rest.RestRequest.Method.GET;
99

10+
import com.google.common.annotations.VisibleForTesting;
1011
import com.google.common.collect.ImmutableList;
1112
import java.io.IOException;
1213
import java.util.List;
@@ -16,7 +17,7 @@
1617
import org.opensearch.rest.BaseRestHandler;
1718
import org.opensearch.rest.BytesRestResponse;
1819
import org.opensearch.rest.RestRequest;
19-
import org.opensearch.sql.executor.autocomplete.GrammarBundle;
20+
import org.opensearch.sql.ppl.autocomplete.GrammarBundle;
2021
import org.opensearch.sql.ppl.autocomplete.PPLGrammarBundleBuilder;
2122
import org.opensearch.transport.client.node.NodeClient;
2223

@@ -47,12 +48,9 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli
4748
return channel -> {
4849
try {
4950
GrammarBundle bundle = getOrBuildBundle();
50-
5151
XContentBuilder builder = channel.newBuilder();
5252
serializeBundle(builder, bundle);
53-
5453
channel.sendResponse(new BytesRestResponse(RestStatus.OK, builder));
55-
5654
} catch (Exception e) {
5755
log.error("Error building or serializing PPL grammar", e);
5856
channel.sendResponse(new BytesRestResponse(channel, RestStatus.INTERNAL_SERVER_ERROR, e));
@@ -68,29 +66,27 @@ private GrammarBundle getOrBuildBundle() {
6866
synchronized (bundleLock) {
6967
// double-check after acquiring lock
7068
if (cachedBundle == null) {
71-
log.info("Building PPL grammar bundle (first request)...");
72-
long startTime = System.currentTimeMillis();
73-
74-
PPLGrammarBundleBuilder builder = new PPLGrammarBundleBuilder();
75-
cachedBundle = builder.build();
76-
77-
long elapsed = System.currentTimeMillis() - startTime;
78-
log.info("Built PPL grammar in {}ms (hash: {})", elapsed, cachedBundle.getGrammarHash());
69+
cachedBundle = buildBundle();
7970
}
8071
return cachedBundle;
8172
}
8273
}
8374

75+
/** Constructs the grammar bundle. Override in tests to inject a custom or failing builder. */
76+
@VisibleForTesting
77+
protected GrammarBundle buildBundle() {
78+
return new PPLGrammarBundleBuilder().build();
79+
}
80+
8481
/** Invalidate the cached bundle, forcing a rebuild on the next request. */
82+
@VisibleForTesting
8583
public void invalidateCache() {
8684
synchronized (bundleLock) {
87-
log.info("Invalidating cached PPL grammar bundle");
8885
cachedBundle = null;
8986
}
9087
}
9188

92-
private void serializeBundle(XContentBuilder builder, GrammarBundle bundle)
93-
throws IOException {
89+
private void serializeBundle(XContentBuilder builder, GrammarBundle bundle) throws IOException {
9490
builder.startObject();
9591

9692
// Identity & versioning
@@ -99,39 +95,18 @@ private void serializeBundle(XContentBuilder builder, GrammarBundle bundle)
9995
builder.field("startRuleIndex", bundle.getStartRuleIndex());
10096

10197
// Lexer ATN & metadata
102-
if (bundle.getLexerSerializedATN() != null) {
103-
builder.field("lexerSerializedATN", bundle.getLexerSerializedATN());
104-
}
105-
106-
if (bundle.getLexerRuleNames() != null) {
107-
builder.field("lexerRuleNames", bundle.getLexerRuleNames());
108-
}
109-
110-
if (bundle.getChannelNames() != null) {
111-
builder.field("channelNames", bundle.getChannelNames());
112-
}
113-
114-
if (bundle.getModeNames() != null) {
115-
builder.field("modeNames", bundle.getModeNames());
116-
}
98+
builder.field("lexerSerializedATN", bundle.getLexerSerializedATN());
99+
builder.field("lexerRuleNames", bundle.getLexerRuleNames());
100+
builder.field("channelNames", bundle.getChannelNames());
101+
builder.field("modeNames", bundle.getModeNames());
117102

118103
// Parser ATN & metadata
119-
if (bundle.getParserSerializedATN() != null) {
120-
builder.field("parserSerializedATN", bundle.getParserSerializedATN());
121-
}
122-
123-
if (bundle.getParserRuleNames() != null) {
124-
builder.field("parserRuleNames", bundle.getParserRuleNames());
125-
}
104+
builder.field("parserSerializedATN", bundle.getParserSerializedATN());
105+
builder.field("parserRuleNames", bundle.getParserRuleNames());
126106

127107
// Vocabulary
128-
if (bundle.getLiteralNames() != null) {
129-
builder.field("literalNames", bundle.getLiteralNames());
130-
}
131-
132-
if (bundle.getSymbolicNames() != null) {
133-
builder.field("symbolicNames", bundle.getSymbolicNames());
134-
}
108+
builder.field("literalNames", bundle.getLiteralNames());
109+
builder.field("symbolicNames", bundle.getSymbolicNames());
135110

136111
builder.endObject();
137112
}

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

Lines changed: 42 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import static org.mockito.Mockito.mock;
1212

1313
import java.io.IOException;
14+
import java.util.concurrent.atomic.AtomicInteger;
1415
import org.junit.Before;
1516
import org.junit.Test;
1617
import org.opensearch.common.io.stream.BytesStreamOutput;
@@ -22,6 +23,7 @@
2223
import org.opensearch.rest.RestChannel;
2324
import org.opensearch.rest.RestRequest;
2425
import org.opensearch.rest.RestResponse;
26+
import org.opensearch.sql.ppl.autocomplete.GrammarBundle;
2527
import org.opensearch.test.rest.FakeRestRequest;
2628
import org.opensearch.transport.client.node.NodeClient;
2729

@@ -78,25 +80,37 @@ public void testGetGrammar_ReturnsBundle() throws Exception {
7880

7981
@Test
8082
public void testGetGrammar_BundleIsCached() throws Exception {
83+
AtomicInteger buildCount = new AtomicInteger(0);
84+
RestPPLGrammarAction countingAction =
85+
new RestPPLGrammarAction() {
86+
@Override
87+
protected GrammarBundle buildBundle() {
88+
buildCount.incrementAndGet();
89+
return super.buildBundle();
90+
}
91+
};
92+
8193
FakeRestRequest request1 =
8294
new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY)
8395
.withMethod(RestRequest.Method.GET)
8496
.withPath("/_plugins/_ppl/_grammar")
8597
.build();
8698
MockRestChannel channel1 = new MockRestChannel(request1, true);
87-
action.handleRequest(request1, channel1, client);
99+
countingAction.handleRequest(request1, channel1, client);
88100

89101
FakeRestRequest request2 =
90102
new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY)
91103
.withMethod(RestRequest.Method.GET)
92104
.withPath("/_plugins/_ppl/_grammar")
93105
.build();
94106
MockRestChannel channel2 = new MockRestChannel(request2, true);
95-
action.handleRequest(request2, channel2, client);
107+
countingAction.handleRequest(request2, channel2, client);
96108

97-
String content1 = channel1.getResponse().content().utf8ToString();
98-
String content2 = channel2.getResponse().content().utf8ToString();
99-
assertEquals("Consecutive requests should return identical content", content1, content2);
109+
assertEquals("Bundle should be built exactly once", 1, buildCount.get());
110+
assertEquals(
111+
"Consecutive requests should return identical content",
112+
channel1.getResponse().content().utf8ToString(),
113+
channel2.getResponse().content().utf8ToString());
100114
}
101115

102116
@Test
@@ -123,7 +137,29 @@ public void testInvalidateCache() throws Exception {
123137

124138
String content1 = channel1.getResponse().content().utf8ToString();
125139
String content2 = channel2.getResponse().content().utf8ToString();
126-
assertEquals("Grammar hash should be identical after cache invalidation and rebuild", content1, content2);
140+
assertEquals(
141+
"Grammar hash should be identical after cache invalidation and rebuild", content1, content2);
142+
}
143+
144+
@Test
145+
public void testGetGrammar_ErrorPath_Returns500() throws Exception {
146+
RestPPLGrammarAction failingAction =
147+
new RestPPLGrammarAction() {
148+
@Override
149+
protected GrammarBundle buildBundle() {
150+
throw new RuntimeException("simulated build failure");
151+
}
152+
};
153+
154+
FakeRestRequest request =
155+
new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY)
156+
.withMethod(RestRequest.Method.GET)
157+
.withPath("/_plugins/_ppl/_grammar")
158+
.build();
159+
MockRestChannel channel = new MockRestChannel(request, true);
160+
failingAction.handleRequest(request, channel, client);
161+
162+
assertEquals(RestStatus.INTERNAL_SERVER_ERROR, channel.getResponse().status());
127163
}
128164

129165
/** Mock RestChannel to capture responses */

core/src/main/java/org/opensearch/sql/executor/autocomplete/GrammarBundle.java renamed to ppl/src/main/java/org/opensearch/sql/ppl/autocomplete/GrammarBundle.java

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,46 +3,47 @@
33
* SPDX-License-Identifier: Apache-2.0
44
*/
55

6-
package org.opensearch.sql.executor.autocomplete;
6+
package org.opensearch.sql.ppl.autocomplete;
77

88
import lombok.Builder;
99
import lombok.Data;
10+
import lombok.NonNull;
1011

11-
/** Response payload for the {@code GET /_plugins/_ppl/_grammar} endpoint. */
12+
/** Serialized ANTLR grammar data served by {@code GET /_plugins/_ppl/_grammar}. */
1213
@Data
1314
@Builder
1415
public class GrammarBundle {
1516

1617
/** Bundle format version. */
17-
private String bundleVersion;
18+
@NonNull private String bundleVersion;
1819

1920
/** SHA-256 hash of the serialized ATN data. Clients may use this to detect grammar changes. */
20-
private String grammarHash;
21+
@NonNull private String grammarHash;
2122

2223
/** Serialized lexer ATN as int array (ATNSerializer output). */
23-
private int[] lexerSerializedATN;
24+
@NonNull private int[] lexerSerializedATN;
2425

2526
/** Lexer rule names. */
26-
private String[] lexerRuleNames;
27+
@NonNull private String[] lexerRuleNames;
2728

2829
/** Channel names (e.g. DEFAULT_TOKEN_CHANNEL, HIDDEN). */
29-
private String[] channelNames;
30+
@NonNull private String[] channelNames;
3031

3132
/** Mode names (e.g. DEFAULT_MODE). */
32-
private String[] modeNames;
33+
@NonNull private String[] modeNames;
3334

3435
/** Serialized parser ATN as int array (ATNSerializer output). */
35-
private int[] parserSerializedATN;
36+
@NonNull private int[] parserSerializedATN;
3637

3738
/** Parser rule names. */
38-
private String[] parserRuleNames;
39+
@NonNull private String[] parserRuleNames;
3940

4041
/** Start rule index (0 = root rule). */
4142
private int startRuleIndex;
4243

4344
/** Literal token names indexed by token type (e.g. "'search'", "'|'"). */
44-
private String[] literalNames;
45+
@NonNull private String[] literalNames;
4546

4647
/** Symbolic token names indexed by token type (e.g. "SEARCH", "PIPE"). */
47-
private String[] symbolicNames;
48+
@NonNull private String[] symbolicNames;
4849
}

‎ppl/src/main/java/org/opensearch/sql/ppl/autocomplete/PPLGrammarBundleBuilder.java‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,13 @@
1212
import org.antlr.v4.runtime.CommonTokenStream;
1313
import org.antlr.v4.runtime.Vocabulary;
1414
import org.antlr.v4.runtime.atn.ATNSerializer;
15-
import org.opensearch.sql.executor.autocomplete.GrammarBundle;
1615
import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLLexer;
1716
import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser;
1817

1918
/** Builds the {@link GrammarBundle} for the PPL language from the generated ANTLR lexer/parser. */
2019
public class PPLGrammarBundleBuilder {
2120

21+
// Keep in sync with the antlr4 version in ppl/build.gradle.
2222
private static final String ANTLR_VERSION = "4.13.2";
2323
private static final String BUNDLE_VERSION = "1.0";
2424

@@ -60,6 +60,8 @@ public GrammarBundle build() {
6060
private static String computeGrammarHash(int[] lexerATN, int[] parserATN) {
6161
try {
6262
MessageDigest digest = MessageDigest.getInstance("SHA-256");
63+
// ANTLR4 serialized ATN values are bounded to 16 bits (unicode char range), so hashing
64+
// 2 bytes per element captures the full value without loss.
6365
for (int v : lexerATN) {
6466
digest.update((byte) (v >> 8));
6567
digest.update((byte) v);
@@ -70,7 +72,9 @@ private static String computeGrammarHash(int[] lexerATN, int[] parserATN) {
7072
}
7173
digest.update(ANTLR_VERSION.getBytes(StandardCharsets.UTF_8));
7274
byte[] hash = digest.digest();
73-
StringBuilder sb = new StringBuilder("sha256:");
75+
// Output is always "sha256:" (7 chars) + 64 hex chars = 71 chars.
76+
StringBuilder sb = new StringBuilder(71);
77+
sb.append("sha256:");
7478
for (byte b : hash) {
7579
sb.append(String.format("%02x", b & 0xFF));
7680
}

‎ppl/src/test/java/org/opensearch/sql/ppl/autocomplete/PPLGrammarBundleBuilderTest.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@
1111

1212
import org.junit.BeforeClass;
1313
import org.junit.Test;
14-
import org.opensearch.sql.executor.autocomplete.GrammarBundle;
1514

1615
public class PPLGrammarBundleBuilderTest {
1716

@@ -37,7 +36,7 @@ public void grammarHashHasExpectedFormat() {
3736
String hash = bundle.getGrammarHash();
3837
assertNotNull(hash);
3938
assertTrue("grammarHash should start with 'sha256:'", hash.startsWith("sha256:"));
40-
assertTrue("grammarHash should be 71 chars (sha256: + 64 hex)", hash.length() == 71);
39+
assertEquals("grammarHash should be 71 chars (sha256: + 64 hex)", 71, hash.length());
4140
}
4241

4342
@Test

0 commit comments

Comments
 (0)