[Feature] Add grammar bundle generation API for PPL language features#5162
Open
mengweieric wants to merge 13 commits intoopensearch-project:mainfrom
Open
[Feature] Add grammar bundle generation API for PPL language features#5162mengweieric wants to merge 13 commits intoopensearch-project:mainfrom
mengweieric wants to merge 13 commits intoopensearch-project:mainfrom
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds GET /_plugins/_ppl/_grammar to serve serialized ANTLR grammar metadata with a cached GrammarBundle; introduces GrammarBundle model, PPLGrammarBundleBuilder, RestPPLGrammarAction handler, unit tests, and updates ANTLR third‑party metadata to 4.13.2. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as RestPPLGrammarAction
participant Cache
participant Builder as PPLGrammarBundleBuilder
participant Serializer as XContentBuilder
Client->>Handler: GET /_plugins/_ppl/_grammar
Handler->>Cache: check cached bundle
alt cache hit
Cache-->>Handler: return Bundle
else cache miss
Handler->>Builder: buildBundle()
Builder->>Builder: inspect lexer/parser, serialize ATNs, compute hash
Builder-->>Handler: Bundle
Handler->>Cache: store Bundle
end
Handler->>Serializer: serialize Bundle to JSON
Serializer-->>Handler: JSON payload
Handler-->>Client: HTTP 200 + JSON
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mengweieric
changed the title
mengweieric
force-pushed
the
feature/grammar-bundle
branch
from
d288392 to
eabe8ec
Compare
mengweieric
changed the title
mengweieric
force-pushed
the
feature/grammar-bundle
branch
from
eabe8ec to
eb12632
Compare
Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
mengweieric
force-pushed
the
feature/grammar-bundle
branch
from
73883a5 to
3f36846
Compare
…y, and tests - Hash full 32-bit ints in grammarHash to avoid collisions with ANTLR 4.13.2 ATN serialization - Use RuntimeMetaData.getRuntimeVersion() instead of unreliable JAR manifest lookup - Make GrammarBundle immutable with @value instead of @DaTa - Update THIRD-PARTY to reflect ANTLR 4.13.2 - Harden tests with JSON parsing and add antlrVersion assertion Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
mengweieric
force-pushed
the
feature/grammar-bundle
branch
from
3f36846 to
c838750
Compare
mengweieric
requested review from
YANG-DB,
dai-chen,
derek-ho,
joshuali925,
kavithacm,
ps48 and
vamsimanohar
as code owners
mengweieric
requested review from
GumpacG,
LantaoJin,
MaxKsyunz,
RyanL1997,
Swiddis,
Yury-Fridlyand,
acarbonetto,
anirudha,
forestmvey,
noCharger,
penghuo,
qianheng-aws,
seankao-az,
ykmr1224 and
yuancu
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLGrammarAction.java (1)
87-93:invalidateCache()ispublicbut is a testing-only hook — restrict its visibility.A
publicmethod on a production REST handler that voids the bundle cache can be called inadvertently by any code. Since@VisibleForTestingalone is not a compile-time guard, the visibility should beprotected(or package-private) to matchbuildBundle().🔧 Proposed fix
- public void invalidateCache() { + protected void invalidateCache() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLGrammarAction.java` around lines 87 - 93, The invalidateCache() method is currently public but only intended for tests; change its visibility to a non-public scope (e.g., protected or package-private) to prevent external production code from calling it, while keeping its synchronized body that nulls cachedBundle under bundleLock; this should mirror the visibility of buildBundle() and retain use of the bundleLock and cachedBundle fields.ppl/src/main/java/org/opensearch/sql/ppl/autocomplete/GrammarBundle.java (1)
27-51:@Valuedoes not deep-copy array fields — the cached bundle could be mutated.Lombok's generated constructor stores
int[]/String[]references directly. For the long-lived cached singleton this means any caller holding the same array reference can corrupt the bundle. Thelexer/parserSerializedATNalready arrive as fresh copies fromIntegerList.toArray(), but theString[]fields (lexerRuleNames,channelNames,modeNames,parserRuleNames,literalNames,symbolicNames) are references into static arrays of the generated ANTLR classes.Add
@Builder.Defaultdefensive copies, or add a@EqualsAndHashCode.Includenote. At minimum, document that fields must be treated as read-only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ppl/src/main/java/org/opensearch/sql/ppl/autocomplete/GrammarBundle.java` around lines 27 - 51, The GrammarBundle holds array fields (lexerRuleNames, channelNames, modeNames, parserRuleNames, literalNames, symbolicNames) that Lombok's `@Value` constructor stores by reference, allowing external mutation of the cached singleton; fix by making defensive copies for these String[] fields (and int[] if not already copied) when constructing the bundle (e.g. in a `@Builder.Default` initializer or custom constructor/factory used by GrammarBundle) so each field is Arrays.copyOf(original, original.length) or wrapped as an unmodifiable List, or otherwise ensure immutability; update the GrammarBundle construction path (the class and any builder/factory) to perform the copies and/or add a short Javadoc on the fields if you choose to only document the read-only contract.ppl/src/main/java/org/opensearch/sql/ppl/autocomplete/PPLGrammarBundleBuilder.java (1)
29-30: Prefer the staticATNSerializer.getSerialized()over the instance constructor.The ANTLR 4.13.2 API exposes
public static IntegerList getSerialized(ATN atn)as the canonical convenience entry point. Using it removes the need to instantiateATNSerializermanually.♻️ Proposed refactor
- int[] lexerATN = new ATNSerializer(lexer.getATN()).serialize().toArray(); - int[] parserATN = new ATNSerializer(parser.getATN()).serialize().toArray(); + int[] lexerATN = ATNSerializer.getSerialized(lexer.getATN()).toArray(); + int[] parserATN = ATNSerializer.getSerialized(parser.getATN()).toArray();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ppl/src/main/java/org/opensearch/sql/ppl/autocomplete/PPLGrammarBundleBuilder.java` around lines 29 - 30, Update PPLGrammarBundleBuilder to use the static convenience method instead of constructing ATNSerializer instances: replace the two lines that call new ATNSerializer(lexer.getATN()).serialize().toArray() and new ATNSerializer(parser.getATN()).serialize().toArray() with calls to ATNSerializer.getSerialized(lexer.getATN()).toArray() and ATNSerializer.getSerialized(parser.getATN()).toArray() respectively so the serialized ATNs are obtained via the static API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLGrammarAction.java`:
- Around line 25-29: The class-level comment for RestPPLGrammarAction is using a
block comment (/* ... */) so the `@opensearch.experimental` tag is not emitted as
Javadoc; change the top-of-class comment to a Javadoc comment (/** ... */) so
the `@opensearch.experimental` annotation is processed correctly — locate the
comment immediately above the RestPPLGrammarAction class declaration and replace
the opening /* with /** (keeping the existing text and tag intact).
In
`@plugin/src/test/java/org/opensearch/sql/plugin/rest/RestPPLGrammarActionTest.java`:
- Around line 242-245: The test's override of bytesOutput() in
RestPPLGrammarAction returns null which can cause a NullPointerException if any
code (e.g., BaseRestHandler or test logic) calls channel.bytesOutput(); replace
the null return with a real BytesStreamOutput instance (e.g., new
BytesStreamOutput()) so channel.bytesOutput() is safe to use; update the
bytesOutput() method in RestPPLGrammarAction to return a non-null
BytesStreamOutput to prevent latent NPEs.
In
`@ppl/src/main/java/org/opensearch/sql/ppl/autocomplete/PPLGrammarBundleBuilder.java`:
- Around line 33-39: The code writes vocabulary.getLiteralName(i) /
vocabulary.getSymbolicName(i) into arrays that will contain null entries for
tokens without a literal/symbolic form; update the contract by adding
documentation on GrammarBundle.literalNames and GrammarBundle.symbolicNames (or
a Javadoc comment on the class/fields) stating that array elements may be null
and clients must handle sparse arrays, and reference the behavior coming from
PPLGrammarBundleBuilder (use the symbols vocabulary.getLiteralName,
vocabulary.getSymbolicName, literalNames, symbolicNames, and the
GrammarBundle.literalNames/GrammarBundle.symbolicNames fields to locate where to
document).
In
`@ppl/src/test/java/org/opensearch/sql/ppl/autocomplete/PPLGrammarBundleBuilderTest.java`:
- Around line 24-107: Rename the test methods in PPLGrammarBundleBuilderTest so
each follows the test<Functionality><Condition> convention (prefix with "test");
e.g., rename bundleIsNotNull -> testBundleIsNotNull, bundleVersionIsSet ->
testBundleVersionIsSet, antlrVersionIsSet -> testAntlrVersionIsSet,
grammarHashHasExpectedFormat -> testGrammarHashHasExpectedFormat,
startRuleIndexIsZero -> testStartRuleIndexIsZero, lexerATNIsNonEmpty ->
testLexerATNIsNonEmpty, parserATNIsNonEmpty -> testParserATNIsNonEmpty,
lexerRuleNamesAreNonEmpty -> testLexerRuleNamesAreNonEmpty,
parserRuleNamesAreNonEmpty -> testParserRuleNamesAreNonEmpty,
channelNamesAreNonEmpty -> testChannelNamesAreNonEmpty, modeNamesAreNonEmpty ->
testModeNamesAreNonEmpty, vocabularyIsNonEmpty -> testVocabularyIsNonEmpty,
buildIsDeterministic -> testBuildIsDeterministic; update any references to these
methods (e.g., test runner annotations or logs) so PPLGrammarBundleBuilderTest
and references to bundle/second/GrammarBundle remain consistent.
---
Nitpick comments:
In
`@plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLGrammarAction.java`:
- Around line 87-93: The invalidateCache() method is currently public but only
intended for tests; change its visibility to a non-public scope (e.g., protected
or package-private) to prevent external production code from calling it, while
keeping its synchronized body that nulls cachedBundle under bundleLock; this
should mirror the visibility of buildBundle() and retain use of the bundleLock
and cachedBundle fields.
In `@ppl/src/main/java/org/opensearch/sql/ppl/autocomplete/GrammarBundle.java`:
- Around line 27-51: The GrammarBundle holds array fields (lexerRuleNames,
channelNames, modeNames, parserRuleNames, literalNames, symbolicNames) that
Lombok's `@Value` constructor stores by reference, allowing external mutation of
the cached singleton; fix by making defensive copies for these String[] fields
(and int[] if not already copied) when constructing the bundle (e.g. in a
`@Builder.Default` initializer or custom constructor/factory used by
GrammarBundle) so each field is Arrays.copyOf(original, original.length) or
wrapped as an unmodifiable List, or otherwise ensure immutability; update the
GrammarBundle construction path (the class and any builder/factory) to perform
the copies and/or add a short Javadoc on the fields if you choose to only
document the read-only contract.
In
`@ppl/src/main/java/org/opensearch/sql/ppl/autocomplete/PPLGrammarBundleBuilder.java`:
- Around line 29-30: Update PPLGrammarBundleBuilder to use the static
convenience method instead of constructing ATNSerializer instances: replace the
two lines that call new ATNSerializer(lexer.getATN()).serialize().toArray() and
new ATNSerializer(parser.getATN()).serialize().toArray() with calls to
ATNSerializer.getSerialized(lexer.getATN()).toArray() and
ATNSerializer.getSerialized(parser.getATN()).toArray() respectively so the
serialized ATNs are obtained via the static API.
plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLGrammarAction.java
Show resolved
Hide resolved
plugin/src/test/java/org/opensearch/sql/plugin/rest/RestPPLGrammarActionTest.java
Show resolved
Hide resolved
ppl/src/main/java/org/opensearch/sql/ppl/autocomplete/PPLGrammarBundleBuilder.java
Show resolved
Hide resolved
ppl/src/test/java/org/opensearch/sql/ppl/autocomplete/PPLGrammarBundleBuilderTest.java
Show resolved
Hide resolved
- Assert ATN serialization version 4 for both lexer and parser to enforce antlr4ng compatibility contract - Resolve startRuleIndex by looking up "root" rule name instead of hardcoding 0 - Fix MockRestChannel.bytesOutput() to return real BytesStreamOutput - Document nullable elements in literalNames/symbolicNames Javadoc - Rename test methods to follow testXxx() convention per ppl/plugin modules Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
mengweieric
force-pushed
the
feature/grammar-bundle
branch
from
fd3529d to
fe41aea
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLGrammarAction.java (1)
87-93:@VisibleForTesting+publicis contradictory — useprotected.
publicmakesinvalidateCache()part of the effective API surface, allowing any production code holding aRestPPLGrammarActionreference to force cache invalidation and trigger an expensivePPLGrammarBundleBuilder.build()on the next request.buildBundle()correctly usesprotectedfor the same testing purpose —invalidateCache()should match.♻️ Proposed fix
- public void invalidateCache() { + protected void invalidateCache() { synchronized (bundleLock) { cachedBundle = null; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLGrammarAction.java` around lines 87 - 93, Change the visibility of invalidateCache() from public to protected to match the testing-only intent indicated by `@VisibleForTesting` and to avoid exposing cache invalidation in the public API; locate the method named invalidateCache in class RestPPLGrammarAction and change its modifier to protected, preserving the synchronized(bundleLock) block and the existing assignment cachedBundle = null so tests can still call it (and buildBundle() remains protected).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLGrammarAction.java
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLGrammarAction.java`:
- Line 62: The method visibility for invalidateCache in RestPPLGrammarAction
should be narrowed from public to protected since it is only exposed for tests;
update the method signature to protected while keeping the `@VisibleForTesting`
annotation and the synchronized (bundleLock) { cachedBundle = null; } body
intact so RestPPLGrammarActionTest can still call it but it is no longer part of
the public API surface.
---
Nitpick comments:
In
`@plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLGrammarAction.java`:
- Around line 87-93: Change the visibility of invalidateCache() from public to
protected to match the testing-only intent indicated by `@VisibleForTesting` and
to avoid exposing cache invalidation in the public API; locate the method named
invalidateCache in class RestPPLGrammarAction and change its modifier to
protected, preserving the synchronized(bundleLock) block and the existing
assignment cachedBundle = null so tests can still call it (and buildBundle()
remains protected).
| channel.sendResponse(new BytesRestResponse(RestStatus.OK, builder)); | ||
| } catch (Exception e) { | ||
| log.error("Error building or serializing PPL grammar", e); | ||
| channel.sendResponse(new BytesRestResponse(channel, RestStatus.INTERNAL_SERVER_ERROR, e)); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. Search for all BytesRestResponse usages in the Java codebase
echo "=== BytesRestResponse usages in the codebase ==="
rg -n --type java 'new BytesRestResponse' -C 2
echo ""
echo "=== Build configuration files ==="
# 2. Find build configuration to identify OpenSearch version
fd -e gradle -e xml | head -10
echo ""
echo "=== OpenSearch dependency declaration ==="
# 3. Check for OpenSearch version in gradle files
rg -n 'opensearch|OpenSearch' --type gradle | grep -i 'version\|dependencies' | head -20Repository: opensearch-project/sql
Length of output: 36203
🏁 Script executed:
#!/bin/bash
# Find OpenSearch version in gradle.properties or build.gradle
echo "=== OpenSearch version ==="
fd 'gradle.properties' -o build.gradle | xargs grep -h 'opensearch_version' | head -5
echo ""
echo "=== Identifying which constructor forms are used in the codebase ==="
# Find patterns of BytesRestResponse(channel, RestStatus, Exception)
rg 'new BytesRestResponse\(channel\s*,' --type java -A 1 | head -20
echo ""
# Compare with 2-arg form BytesRestResponse(channel, e)
echo "=== 2-arg form with channel and exception ==="
rg 'new BytesRestResponse\(channel\s*,\s*[a-zA-Z]' --type java | grep -v RestStatus | head -10Repository: opensearch-project/sql
Length of output: 1865
🏁 Script executed:
#!/bin/bash
# Find gradle.properties properly
echo "=== Find gradle.properties ==="
find . -maxdepth 2 -name 'gradle.properties' -type f
echo ""
echo "=== Check for OpenSearch version in root gradle.properties ==="
cat gradle.properties 2>/dev/null | grep -i opensearch | head -10
echo ""
echo "=== Check imports and actual usages of BytesRestResponse in RestPPLGrammarAction ==="
head -30 plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLGrammarAction.java
echo ""
echo "=== Check the exact line 62 and surrounding context ==="
sed -n '50,65p' plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLGrammarAction.javaRepository: opensearch-project/sql
Length of output: 1916
🏁 Script executed:
#!/bin/bash
# Get the full gradle.properties content to find OpenSearch version
echo "=== Full gradle.properties ==="
cat gradle.properties
echo ""
echo "=== Check if there's version info in the main build.gradle ==="
head -50 build.gradle | grep -i versionRepository: opensearch-project/sql
Length of output: 1158
🌐 Web query:
OpenSearch 3.6.0 BytesRestResponse constructor signature
💡 Result:
OpenSearch core doesn’t have a 3.6.0 release yet (it’s scheduled for Mar 24–Apr 7, 2026), so there’s no 3.6.0 BytesRestResponse to reference. [2] The latest released 3.x core version shown in the docs is 3.4.0. [3]
In OpenSearch 3.4.0, org.opensearch.rest.BytesRestResponse has these constructors: [1]
public BytesRestResponse(RestStatus status, XContentBuilder builder)
public BytesRestResponse(RestStatus status, String content)
public BytesRestResponse(RestStatus status, String contentType, String content)
public BytesRestResponse(RestStatus status, String contentType, byte[] content)
public BytesRestResponse(RestStatus status, String contentType, BytesReference content)
public BytesRestResponse(RestChannel channel, Exception e) throws IOException
public BytesRestResponse(RestChannel channel, RestStatus status, Exception e) throws IOExceptionSources: [1] [2] [3]
🏁 Script executed:
#!/bin/bash
echo "=== Check invalidateCache() method definition and visibility ==="
sed -n '87,93p' plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLGrammarAction.java
echo ""
echo "=== Check if invalidateCache() is called anywhere outside the class ==="
rg 'invalidateCache' --type java -B 2 -A 2Repository: opensearch-project/sql
Length of output: 1529
Change invalidateCache() visibility from public to protected.
The @VisibleForTesting annotation (line 87) indicates that the method's visibility was widened solely for testing purposes. However, public is the widest possible scope and unnecessarily exposes the method as part of the REST handler's API surface. Since it is only called from tests (RestPPLGrammarActionTest.java), change it to protected:
`@VisibleForTesting`
protected void invalidateCache() {
synchronized (bundleLock) {
cachedBundle = null;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLGrammarAction.java`
at line 62, The method visibility for invalidateCache in RestPPLGrammarAction
should be narrowed from public to protected since it is only exposed for tests;
update the method signature to protected while keeping the `@VisibleForTesting`
annotation and the synchronized (bundleLock) { cachedBundle = null; } body
intact so RestPPLGrammarActionTest can still call it but it is no longer part of
the public API surface.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
plugin/src/test/java/org/opensearch/sql/plugin/rest/RestPPLGrammarActionTest.java (1)
163-182: Consider adding a test forbuildBundle()returningnull.The current error path only tests an exception. If
buildBundle()returnsnull(e.g., an implementation that silently returnsnullinstead of throwing), the action's behavior is untested and could produce an NPE or an unexpected response rather than a clean 500. A one-line variant of the failing action covering that path would close the gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/src/test/java/org/opensearch/sql/plugin/rest/RestPPLGrammarActionTest.java` around lines 163 - 182, Add a test variant to cover the case where RestPPLGrammarAction.buildBundle() returns null: create a failingAction subclass of RestPPLGrammarAction that overrides buildBundle() to return null, invoke failingAction.handleRequest(...) with the same FakeRestRequest/MockRestChannel setup used in testGetGrammar_ErrorPath_Returns500, and assert that channel.getResponse().status() equals RestStatus.INTERNAL_SERVER_ERROR to ensure a null bundle produces a 500 response rather than an NPE or other status.ppl/src/main/java/org/opensearch/sql/ppl/autocomplete/GrammarBundle.java (1)
27-57: Shallow immutability: array fields are mutable through getters.
@Valueonly ensures "no setters are generated and the fields are defined as final so that their references can't be changed" — but the contents ofint[]andString[]fields remain fully mutable. Since the PR caches a singleGrammarBundlefor the entire node lifecycle, a caller that mutatesgetLexerSerializedATN()[0] = …would silently corrupt the cached bundle for all future requests.In practice, current consumers only read (serialize) the arrays, so the immediate risk is low; but the class uses
@Valueto signal immutability. Consider wrapping arrays withArrays.copyOfin the generated getter or storing unmodifiable wrappers:♻️ Suggested defensive-copy approach
- `@NonNull` private int[] lexerSerializedATN; + `@NonNull` `@Getter`(AccessLevel.NONE) private int[] lexerSerializedATN;Then add an explicit getter:
public int[] getLexerSerializedATN() { return Arrays.copyOf(lexerSerializedATN, lexerSerializedATN.length); }Apply the same pattern for all other array fields (
parserSerializedATN,lexerRuleNames,channelNames,modeNames,parserRuleNames,literalNames,symbolicNames).Alternatively, switch array types to
List<Integer>/List<String>(wrapped withCollections.unmodifiableListorList.of) to gain true shallow immutability with no extra boilerplate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ppl/src/main/java/org/opensearch/sql/ppl/autocomplete/GrammarBundle.java` around lines 27 - 57, The array fields (lexerSerializedATN, parserSerializedATN, lexerRuleNames, channelNames, modeNames, parserRuleNames, literalNames, symbolicNames) are mutable via generated getters; make them defensively immutable by returning copies or unmodifiable collections: either replace array types with List<Integer>/List<String> and store them as unmodifiable lists (Collections.unmodifiableList or List.of) or add explicit getters (e.g., getLexerSerializedATN, getParserSerializedATN, getLexerRuleNames, getChannelNames, getModeNames, getParserRuleNames, getLiteralNames, getSymbolicNames) that return Arrays.copyOf(...) / Arrays.copyOf(...) converted to arrays or return the unmodifiable List view, and ensure the constructor/state stores copies as well to avoid exposing internal arrays.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
plugin/src/test/java/org/opensearch/sql/plugin/rest/RestPPLGrammarActionTest.javappl/src/main/java/org/opensearch/sql/ppl/autocomplete/GrammarBundle.javappl/src/main/java/org/opensearch/sql/ppl/autocomplete/PPLGrammarBundleBuilder.javappl/src/test/java/org/opensearch/sql/ppl/autocomplete/PPLGrammarBundleBuilderTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- ppl/src/test/java/org/opensearch/sql/ppl/autocomplete/PPLGrammarBundleBuilderTest.java
- ppl/src/main/java/org/opensearch/sql/ppl/autocomplete/PPLGrammarBundleBuilder.java
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@plugin/src/test/java/org/opensearch/sql/plugin/rest/RestPPLGrammarActionTest.java`:
- Around line 133-161: Modify testInvalidateCache to assert that buildBundle()
is invoked twice by instrumenting the bundle builder with an AtomicInteger
counter (same approach as testGetGrammar_BundleIsCached): add an AtomicInteger,
increment it inside the component/method that constructs the grammar bundle
(buildBundle), call action.handleRequest(...) once, call
action.invalidateCache(), call action.handleRequest(...) again, and then assert
the counter equals 2 to prove rebuild occurred; reference testInvalidateCache,
action.invalidateCache(), and buildBundle() when making the change.
- Around line 1-30: The plugin module's JUnit4 tests (e.g.,
RestPPLGrammarActionTest using `@Before`, `@Test`, and org.junit.Assert) are skipped
because only JUnit Jupiter is declared; add the JUnit Vintage engine to the
plugin's build.gradle test runtime so JUnit 4 tests run on the JUnit 5 platform
by adding testRuntimeOnly("org.junit.vintage:junit-vintage-engine") with the
exclusion of org.hamcrest:hamcrest-core as shown in the review comment.
---
Nitpick comments:
In
`@plugin/src/test/java/org/opensearch/sql/plugin/rest/RestPPLGrammarActionTest.java`:
- Around line 163-182: Add a test variant to cover the case where
RestPPLGrammarAction.buildBundle() returns null: create a failingAction subclass
of RestPPLGrammarAction that overrides buildBundle() to return null, invoke
failingAction.handleRequest(...) with the same FakeRestRequest/MockRestChannel
setup used in testGetGrammar_ErrorPath_Returns500, and assert that
channel.getResponse().status() equals RestStatus.INTERNAL_SERVER_ERROR to ensure
a null bundle produces a 500 response rather than an NPE or other status.
In `@ppl/src/main/java/org/opensearch/sql/ppl/autocomplete/GrammarBundle.java`:
- Around line 27-57: The array fields (lexerSerializedATN, parserSerializedATN,
lexerRuleNames, channelNames, modeNames, parserRuleNames, literalNames,
symbolicNames) are mutable via generated getters; make them defensively
immutable by returning copies or unmodifiable collections: either replace array
types with List<Integer>/List<String> and store them as unmodifiable lists
(Collections.unmodifiableList or List.of) or add explicit getters (e.g.,
getLexerSerializedATN, getParserSerializedATN, getLexerRuleNames,
getChannelNames, getModeNames, getParserRuleNames, getLiteralNames,
getSymbolicNames) that return Arrays.copyOf(...) / Arrays.copyOf(...) converted
to arrays or return the unmodifiable List view, and ensure the constructor/state
stores copies as well to avoid exposing internal arrays.
Comment on lines
+1
to
+30
| /* | ||
| * Copyright OpenSearch Contributors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package org.opensearch.sql.plugin.rest; | ||
|
|
||
| import static org.junit.Assert.assertEquals; | ||
| import static org.junit.Assert.assertNotNull; | ||
| import static org.junit.Assert.assertTrue; | ||
| import static org.mockito.Mockito.mock; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.concurrent.atomic.AtomicInteger; | ||
| import org.json.JSONObject; | ||
| import org.junit.Before; | ||
| import org.junit.Test; | ||
| import org.opensearch.common.io.stream.BytesStreamOutput; | ||
| import org.opensearch.common.xcontent.XContentType; | ||
| import org.opensearch.core.rest.RestStatus; | ||
| import org.opensearch.core.xcontent.MediaType; | ||
| import org.opensearch.core.xcontent.NamedXContentRegistry; | ||
| import org.opensearch.core.xcontent.XContentBuilder; | ||
| import org.opensearch.rest.RestChannel; | ||
| import org.opensearch.rest.RestRequest; | ||
| import org.opensearch.rest.RestResponse; | ||
| import org.opensearch.sql.ppl.autocomplete.GrammarBundle; | ||
| import org.opensearch.test.rest.FakeRestRequest; | ||
| import org.opensearch.transport.client.node.NodeClient; | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check other tests in the plugin module for JUnit usage patterns
rg -n --type java "import org\.junit\.(Before|Test|Assert|After)\b|import org\.junit\.jupiter" \
plugin/src/test/java --include='*.java' | head -50Repository: opensearch-project/sql
Length of output: 507
🏁 Script executed:
# Also check for JUnit 5 annotations
rg -n --type java "@BeforeEach|@AfterEach|@org\.junit\.jupiter" \
plugin/src/test/java --include='*.java' | head -30Repository: opensearch-project/sql
Length of output: 507
🏁 Script executed:
# Find gradle build files to check JUnit dependencies
find . -name "build.gradle*" -o -name "pom.xml" | grep -E "(build\.gradle|pom\.xml)"Repository: opensearch-project/sql
Length of output: 598
🏁 Script executed:
# Search for JUnit imports in plugin test directory
rg "import org\.junit\.(Before|Test|Assert|After)" plugin/src/test/java -t java -cRepository: opensearch-project/sql
Length of output: 336
🏁 Script executed:
# Search for JUnit 5 imports in plugin test directory
rg "import org\.junit\.jupiter" plugin/src/test/java -t java -cRepository: opensearch-project/sql
Length of output: 48
🏁 Script executed:
# Check the plugin/build.gradle for JUnit configuration
cat plugin/build.gradleRepository: opensearch-project/sql
Length of output: 11415
🏁 Script executed:
# Also check the root build.gradle for test dependencies
head -100 build.gradleRepository: opensearch-project/sql
Length of output: 4014
🏁 Script executed:
# Check for junit-vintage-engine in all build files
rg "junit-vintage-engine" . -t gradleRepository: opensearch-project/sql
Length of output: 441
🏁 Script executed:
# Check all test files to see JUnit 4 vs JUnit 5 usage across entire project
rg "import org\.junit\.(Before|Test|Assert|After)" . -t java -c | wc -lRepository: opensearch-project/sql
Length of output: 69
🏁 Script executed:
# Count JUnit 5 imports across the project
rg "import org\.junit\.jupiter" . -t java -c | wc -lRepository: opensearch-project/sql
Length of output: 69
🏁 Script executed:
# Verify plugin/build.gradle does not have junit-vintage-engine by checking the exact test dependencies section
sed -n '/^dependencies {/,/^}/p' plugin/build.gradle | grep -i "junit\|vintage"Repository: opensearch-project/sql
Length of output: 235
🏁 Script executed:
# Check one of the modules that has junit-vintage-engine to confirm the pattern
sed -n '/^dependencies {/,/^}/p' async-query/build.gradle | grep -A 2 "junit-vintage"Repository: opensearch-project/sql
Length of output: 198
Add junit-vintage-engine dependency to plugin/build.gradle to enable JUnit 4 tests on JUnit 5 platform.
The plugin module declares org.junit.jupiter:junit-jupiter:5.9.3 but all three tests use JUnit 4 APIs (@Before, @Test, org.junit.Assert). Without the junit-vintage-engine compatibility layer, these tests will be silently skipped at runtime. Other modules in the project (async-query, async-query-core, direct-query, direct-query-core) properly include this dependency. Add to the dependencies block in plugin/build.gradle:
testRuntimeOnly("org.junit.vintage:junit-vintage-engine") {
exclude group: 'org.hamcrest', module: 'hamcrest-core'
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@plugin/src/test/java/org/opensearch/sql/plugin/rest/RestPPLGrammarActionTest.java`
around lines 1 - 30, The plugin module's JUnit4 tests (e.g.,
RestPPLGrammarActionTest using `@Before`, `@Test`, and org.junit.Assert) are skipped
because only JUnit Jupiter is declared; add the JUnit Vintage engine to the
plugin's build.gradle test runtime so JUnit 4 tests run on the JUnit 5 platform
by adding testRuntimeOnly("org.junit.vintage:junit-vintage-engine") with the
exclusion of org.hamcrest:hamcrest-core as shown in the review comment.
plugin/src/test/java/org/opensearch/sql/plugin/rest/RestPPLGrammarActionTest.java
Outdated
Show resolved
Hide resolved
…mmarActionTest.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Eric Wei <menwe@amazon.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Implements the backend grammar metadata API for PPL autocomplete support.
This endpoint serves a versioned grammar bundle containing all ANTLR metadata required for downstream consumers (e.g. OpenSearch Dashboards) to reconstruct a functional PPL lexer and parser at runtime using antlr4ng's interpreter APIs. This enables full client-side parsing and autocomplete with zero per-keystroke server calls, with the backend as the single source of truth for the PPL grammar.
What the bundle contains:
Backend behavior:
Also included:
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
Check List
--signoffor-s.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.