Skip to content

Commit 5f0a84c

Browse files
committed
cleanup on comments and debug logging
Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
1 parent 31511a7 commit 5f0a84c

4 files changed

Lines changed: 25 additions & 97 deletions

File tree

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

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,17 +8,12 @@
88
import lombok.Builder;
99
import lombok.Data;
1010

11-
/**
12-
* Grammar bundle containing everything needed for client-side ANTLR-based parsing.
13-
*
14-
* <p>Includes serialized ATN data (lexer + parser), vocabulary, and rule/channel/mode names.
15-
* Language-agnostic — usable for PPL, SQL, or any ANTLR4 grammar.
16-
*/
11+
/** Response payload for the {@code GET /_plugins/_ppl/_grammar} endpoint. */
1712
@Data
1813
@Builder
1914
public class GrammarBundle {
2015

21-
/** Bundle format version. Increment when the schema changes. */
16+
/** Bundle format version. */
2217
private String bundleVersion;
2318

2419
/** SHA-256 hash of the serialized ATN data. Clients may use this to detect grammar changes. */

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

Lines changed: 5 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -20,21 +20,7 @@
2020
import org.opensearch.sql.ppl.autocomplete.PPLGrammarBundleBuilder;
2121
import org.opensearch.transport.client.node.NodeClient;
2222

23-
/**
24-
* REST handler for PPL grammar metadata endpoint.
25-
*
26-
* <p>Endpoint: GET /_plugins/_ppl/_grammar
27-
*
28-
* <p>Returns grammar data for client-side parsing, autocomplete, syntax highlighting, etc:
29-
*
30-
* <ul>
31-
* <li>Serialized ANTLR ATN data (lexer + parser)
32-
* <li>Vocabulary (literal and symbolic names)
33-
* <li>Rule names (parser and lexer)
34-
* <li>Channel and mode names
35-
* </ul>
36-
*
37-
*/
23+
/** REST handler for {@code GET /_plugins/_ppl/_grammar}. */
3824
@Log4j2
3925
public class RestPPLGrammarAction extends BaseRestHandler {
4026

@@ -58,18 +44,14 @@ public List<Route> routes() {
5844
protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client)
5945
throws IOException {
6046

61-
log.debug("Received PPL grammar request");
62-
6347
return channel -> {
6448
try {
6549
GrammarBundle bundle = getOrBuildBundle();
6650

6751
XContentBuilder builder = channel.newBuilder();
6852
serializeBundle(builder, bundle);
6953

70-
BytesRestResponse response = new BytesRestResponse(RestStatus.OK, builder);
71-
log.info("Returning PPL grammar (size: {} bytes)", response.content().length());
72-
channel.sendResponse(response);
54+
channel.sendResponse(new BytesRestResponse(RestStatus.OK, builder));
7355

7456
} catch (Exception e) {
7557
log.error("Error building or serializing PPL grammar", e);
@@ -78,20 +60,13 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli
7860
};
7961
}
8062

81-
/**
82-
* Get cached bundle or build it if not yet initialized.
83-
*
84-
* <p>Thread-safe lazy initialization with double-checked locking.
85-
*/
63+
// Thread-safe lazy initialization with double-checked locking.
8664
private GrammarBundle getOrBuildBundle() {
87-
// First check without lock (common case: already initialized)
8865
if (cachedBundle != null) {
8966
return cachedBundle;
9067
}
91-
92-
// Acquire lock for initialization
9368
synchronized (bundleLock) {
94-
// Double-check after acquiring lock
69+
// double-check after acquiring lock
9570
if (cachedBundle == null) {
9671
log.info("Building PPL grammar bundle (first request)...");
9772
long startTime = System.currentTimeMillis();
@@ -106,22 +81,14 @@ private GrammarBundle getOrBuildBundle() {
10681
}
10782
}
10883

109-
/**
110-
* Invalidate cached bundle (for testing or grammar updates).
111-
*
112-
* <p>Note: This only affects the current node. In a multi-node cluster, each node maintains its
113-
* own cache.
114-
*/
84+
/** Invalidate the cached bundle, forcing a rebuild on the next request. */
11585
public void invalidateCache() {
11686
synchronized (bundleLock) {
11787
log.info("Invalidating cached PPL grammar bundle");
11888
cachedBundle = null;
11989
}
12090
}
12191

122-
/**
123-
* Serialize {@link GrammarBundle} to JSON.
124-
*/
12592
private void serializeBundle(XContentBuilder builder, GrammarBundle bundle)
12693
throws IOException {
12794
builder.startObject();
@@ -134,7 +101,6 @@ private void serializeBundle(XContentBuilder builder, GrammarBundle bundle)
134101
// Lexer ATN & metadata
135102
if (bundle.getLexerSerializedATN() != null) {
136103
builder.field("lexerSerializedATN", bundle.getLexerSerializedATN());
137-
log.debug("Lexer ATN: {} elements", bundle.getLexerSerializedATN().length);
138104
}
139105

140106
if (bundle.getLexerRuleNames() != null) {
@@ -152,7 +118,6 @@ private void serializeBundle(XContentBuilder builder, GrammarBundle bundle)
152118
// Parser ATN & metadata
153119
if (bundle.getParserSerializedATN() != null) {
154120
builder.field("parserSerializedATN", bundle.getParserSerializedATN());
155-
log.debug("Parser ATN: {} elements", bundle.getParserSerializedATN().length);
156121
}
157122

158123
if (bundle.getParserRuleNames() != null) {

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

Lines changed: 2 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -23,17 +23,7 @@
2323
import org.opensearch.test.rest.FakeRestRequest;
2424
import org.opensearch.transport.client.node.NodeClient;
2525

26-
/**
27-
* Unit tests for RestPPLGrammarAction.
28-
*
29-
* <p>Tests:
30-
*
31-
* <ul>
32-
* <li>200 OK response with grammar bundle
33-
* <li>Grammar structure validation
34-
* <li>Cache behavior
35-
* </ul>
36-
*/
26+
/** Unit tests for {@link RestPPLGrammarAction}. */
3727
public class RestPPLGrammarActionTest extends OpenSearchTestCase {
3828

3929
private RestPPLGrammarAction action;
@@ -83,8 +73,7 @@ public void testGetGrammar_ReturnsBundle() throws Exception {
8373
}
8474

8575
@Test
86-
public void testGetGrammar_ArtifactIsCached() throws Exception {
87-
// Make two requests
76+
public void testGetGrammar_BundleIsCached() throws Exception {
8877
FakeRestRequest request1 =
8978
new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY)
9079
.withMethod(RestRequest.Method.GET)
@@ -96,7 +85,6 @@ public void testGetGrammar_ArtifactIsCached() throws Exception {
9685
action.prepareRequest(request1, client).accept(channel1);
9786
long elapsed1 = System.currentTimeMillis() - startTime1;
9887

99-
// Second request should be faster (cached)
10088
FakeRestRequest request2 =
10189
new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY)
10290
.withMethod(RestRequest.Method.GET)
@@ -108,7 +96,6 @@ public void testGetGrammar_ArtifactIsCached() throws Exception {
10896
action.prepareRequest(request2, client).accept(channel2);
10997
long elapsed2 = System.currentTimeMillis() - startTime2;
11098

111-
// Second request should be faster (bundle cached)
11299
assertTrue(
113100
"Second request should be faster due to caching (elapsed1="
114101
+ elapsed1
@@ -120,7 +107,6 @@ public void testGetGrammar_ArtifactIsCached() throws Exception {
120107

121108
@Test
122109
public void testInvalidateCache() throws Exception {
123-
// Get grammar
124110
FakeRestRequest request1 =
125111
new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY)
126112
.withMethod(RestRequest.Method.GET)
@@ -131,10 +117,8 @@ public void testInvalidateCache() throws Exception {
131117
action.prepareRequest(request1, client).accept(channel1);
132118
assertEquals(RestStatus.OK, channel1.getResponse().status());
133119

134-
// Invalidate cache
135120
action.invalidateCache();
136121

137-
// Get grammar again — should still return 200 OK with same content
138122
FakeRestRequest request2 =
139123
new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY)
140124
.withMethod(RestRequest.Method.GET)

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

Lines changed: 16 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
import java.nio.charset.StandardCharsets;
99
import java.security.MessageDigest;
1010
import java.security.NoSuchAlgorithmException;
11-
import lombok.extern.log4j.Log4j2;
1211
import org.antlr.v4.runtime.CharStreams;
1312
import org.antlr.v4.runtime.CommonTokenStream;
1413
import org.antlr.v4.runtime.Vocabulary;
@@ -18,33 +17,22 @@
1817
import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser;
1918

2019
/** Builds the {@link GrammarBundle} for the PPL language from the generated ANTLR lexer/parser. */
21-
@Log4j2
2220
public class PPLGrammarBundleBuilder {
2321

2422
private static final String ANTLR_VERSION = "4.13.2";
2523
private static final String BUNDLE_VERSION = "1.0";
2624

27-
/**
28-
* Build the PPL grammar bundle.
29-
*
30-
* <p>Uses {@link ATNSerializer} to re-serialize the ATN into the int[] format expected by the
31-
* antlr4ng runtime on the frontend. Do NOT use {@code lexer.getSerializedATN().chars().toArray()}
32-
* — that yields raw UTF-16 char values which cause "state type 65535 is not valid" errors.
33-
*
34-
* @return {@link GrammarBundle} ready for JSON serialization
35-
*/
3625
public GrammarBundle build() {
37-
log.info("Building PPL grammar bundle...");
38-
3926
OpenSearchPPLLexer lexer = new OpenSearchPPLLexer(CharStreams.fromString(""));
4027
CommonTokenStream tokens = new CommonTokenStream(lexer);
4128
OpenSearchPPLParser parser = new OpenSearchPPLParser(tokens);
4229

30+
// ATNSerializer re-serializes the ATN into the int[] format expected by antlr4ng.
31+
// Do not use lexer.getSerializedATN().chars().toArray() — that yields raw UTF-16 char values
32+
// which cause "state type 65535 is not valid" errors in the frontend deserializer.
4333
int[] lexerATN = new ATNSerializer(lexer.getATN()).serialize().toArray();
4434
int[] parserATN = new ATNSerializer(parser.getATN()).serialize().toArray();
4535

46-
log.info("Lexer ATN: {} elements, Parser ATN: {} elements", lexerATN.length, parserATN.length);
47-
4836
Vocabulary vocabulary = parser.getVocabulary();
4937
int maxTokenType = vocabulary.getMaxTokenType();
5038
String[] literalNames = new String[maxTokenType + 1];
@@ -54,23 +42,19 @@ public GrammarBundle build() {
5442
symbolicNames[i] = vocabulary.getSymbolicName(i);
5543
}
5644

57-
GrammarBundle bundle =
58-
GrammarBundle.builder()
59-
.bundleVersion(BUNDLE_VERSION)
60-
.grammarHash(computeGrammarHash(lexerATN, parserATN))
61-
.lexerSerializedATN(lexerATN)
62-
.parserSerializedATN(parserATN)
63-
.lexerRuleNames(lexer.getRuleNames())
64-
.parserRuleNames(parser.getRuleNames())
65-
.channelNames(lexer.getChannelNames())
66-
.modeNames(lexer.getModeNames())
67-
.startRuleIndex(0)
68-
.literalNames(literalNames)
69-
.symbolicNames(symbolicNames)
70-
.build();
71-
72-
log.info("Built PPL grammar bundle: {} tokens", symbolicNames.length);
73-
return bundle;
45+
return GrammarBundle.builder()
46+
.bundleVersion(BUNDLE_VERSION)
47+
.grammarHash(computeGrammarHash(lexerATN, parserATN))
48+
.lexerSerializedATN(lexerATN)
49+
.parserSerializedATN(parserATN)
50+
.lexerRuleNames(lexer.getRuleNames())
51+
.parserRuleNames(parser.getRuleNames())
52+
.channelNames(lexer.getChannelNames())
53+
.modeNames(lexer.getModeNames())
54+
.startRuleIndex(0)
55+
.literalNames(literalNames)
56+
.symbolicNames(symbolicNames)
57+
.build();
7458
}
7559

7660
private static String computeGrammarHash(int[] lexerATN, int[] parserATN) {

0 commit comments

Comments
 (0)