[draft] LBP initial interface

[sandeshkr419]

Description

The Lucene Backend Plugin is a new AnalyticsBackEndPlugin implementation that bridges DataFusion query execution with Lucene text-search capabilities. In the analytics architecture, DataFusion serves as the primary execution engine over Parquet-based DocValues for columnar scans, numeric filtering, and aggregations. Lucene is used strictly for optimized text-search delegation. The plugin operates across two node roles: on the Coordinator Node it converts Calcite RexNode expressions into Lucene QueryBuilder representations, and on the Data Node it executes those queries against Lucene text indices and returns DocIdsBitSet results for DataFusion to consume.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

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.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 090deb7.

Path	Line	Severity	Description
sandbox/plugins/analytics-backend-lucene/build.gradle	27	high	New external dependency added: org.apache.calcite:calcite-core:1.41.0. Artifact authenticity cannot be verified without maintainer review.
sandbox/plugins/analytics-backend-lucene/build.gradle	38	high	New external dependency added: net.jqwik:jqwik-api:1.9.1. Artifact authenticity cannot be verified without maintainer review.
sandbox/plugins/analytics-backend-lucene/build.gradle	39	high	New external dependency added: net.jqwik:jqwik-engine:1.9.1. Artifact authenticity cannot be verified without maintainer review.
sandbox/plugins/analytics-backend-lucene/build.gradle	42	high	New external dependency added: org.junit.platform:junit-platform-launcher:1.11.1. Artifact authenticity cannot be verified without maintainer review.
sandbox/plugins/analytics-backend-lucene/build.gradle	54	high	New external dependency added: org.apache.calcite.avatica:avatica-core:1.27.0. Artifact authenticity cannot be verified without maintainer review.
sandbox/plugins/analytics-backend-lucene/build.gradle	55	high	New external dependency added: org.jooq:joou-java-6:0.9.4. Artifact authenticity cannot be verified without maintainer review.
sandbox/plugins/analytics-backend-lucene/build.gradle	57	high	New external dependency added: com.jayway.jsonpath:json-path:2.9.0. Artifact authenticity cannot be verified without maintainer review.
sandbox/plugins/analytics-backend-lucene/build.gradle	58	high	New external dependency added: org.apache.commons:commons-math3:3.6.1. Artifact authenticity cannot be verified without maintainer review.
sandbox/plugins/analytics-backend-lucene/build.gradle	72	high	Dependency version forced to com.google.guava:guava:33.4.0-jre via resolutionStrategy. Forced version overrides may bypass existing security-vetted version pinning.
sandbox/plugins/analytics-backend-lucene/build.gradle	74	high	Dependency version forced to com.google.errorprone:error_prone_annotations:2.36.0 via resolutionStrategy. Forced version overrides may bypass existing security-vetted version pinning.

The table above displays the top 10 most important findings.

Total: 23 | Critical: 0 | High: 23 | Medium: 0 | Low: 0

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 949e466)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Framework interfaces and shard execution context Relevant files: sandbox/libs/analytics-framework/src/main/java/org/opensearch/analytics/backend/EngineBridge.java sandbox/libs/analytics-framework/src/main/java/org/opensearch/analytics/backend/ShardExecutionContext.java sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/DefaultShardExecutionContext.java Sub-PR theme: Predicate handlers, converter, and serializer Relevant files: sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/PredicateHandler.java sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/PredicateHandlerRegistry.java sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/EqualsPredicateHandler.java sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LikePredicateHandler.java sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/RexToQueryBuilderConverter.java sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/QueryBuilderSerializer.java sandbox/plugins/analytics-backend-lucene/src/test/java/org/opensearch/be/lucene/RexToQueryBuilderConverterPropertyTests.java sandbox/plugins/analytics-backend-lucene/src/test/java/org/opensearch/be/lucene/QueryBuilderSerializerPropertyTests.java Sub-PR theme: Lucene engine bridge, plugin registration, and execution tests Relevant files: sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LuceneEngineBridge.java sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LuceneBackendPlugin.java sandbox/plugins/analytics-backend-lucene/src/main/resources/META-INF/services/org.opensearch.analytics.spi.AnalyticsBackEndPlugin sandbox/plugins/analytics-backend-lucene/src/test/java/org/opensearch/be/lucene/LuceneEngineBridgeTests.java sandbox/plugins/analytics-backend-lucene/src/test/java/org/opensearch/be/lucene/LuceneExecutionWiringTests.java sandbox/plugins/analytics-backend-lucene/DESIGN.md

⚡ Recommended focus areas for review Thread Safety LuceneEngineBridge stores mutable state (engineSearcher, queryShardContext, allocator, weightCache) in instance fields with no synchronization. The DESIGN.md acknowledges "single-threaded per bridge instance," but there is no enforcement (e.g., no assertion, no volatile, no lock). If the same bridge instance is accidentally shared across threads (e.g., during concurrent shard execution), silent data corruption or use-after-close of the Engine.Searcher will occur. At minimum, a comment or assertion should guard against concurrent use. public class LuceneEngineBridge implements EngineBridge<byte[], Iterator<VectorSchemaRoot>, RelNode> { /** Column name for the document ID bitset in the result schema. */ public static final String DOC_IDS_COLUMN = "doc_ids"; private static final Schema DOC_IDS_SCHEMA = new Schema( List.of(Field.nullable(DOC_IDS_COLUMN, new ArrowType.Bool())) ); // Cached shard-level resources, set by initialize(), cleared by close() private Engine.Searcher engineSearcher; private QueryShardContext queryShardContext; private BufferAllocator allocator; // Cached Weight per query fragment, avoids recomputing toQuery/rewrite/createWeight per segment/batch private final Map<ByteBuffer, Weight> weightCache = new HashMap<>(); @Override public void initialize(ShardExecutionContext context) { if (context instanceof DefaultShardExecutionContext shardCtx == false) { throw new IllegalArgumentException( "LuceneEngineBridge requires DefaultShardExecutionContext, got: " + (context == null ? "null" : context.getClass().getSimpleName()) ); } DefaultShardExecutionContext shardCtx = (DefaultShardExecutionContext) context; Engine.Searcher searcher = shardCtx.indexShard().acquireSearcher("lucene-analytics"); try { this.queryShardContext = shardCtx.createQueryShardContext(searcher); this.engineSearcher = searcher; } catch (Exception e) { searcher.close(); throw e; } } @Override public void close() { if (engineSearcher != null) { engineSearcher.close(); engineSearcher = null; queryShardContext = null; } weightCache.clear(); if (allocator != null) { allocator.close(); allocator = null; } } Resource Leak In createEmptyResult() and createResultFromBitSet(), a VectorSchemaRoot is created and returned inside a Collections.singletonList(...).iterator(). The caller is responsible for closing the root, but there is no contract or AutoCloseable wrapper enforcing this. If the caller iterates but never calls root.close() (e.g., due to an exception after results.next()), the Arrow off-heap memory leaks. The tests do call root.close() manually, but production callers have no compile-time guarantee. private Iterator<VectorSchemaRoot> createEmptyResult() { BufferAllocator alloc = getAllocator(); VectorSchemaRoot root = VectorSchemaRoot.create(DOC_IDS_SCHEMA, alloc); BitVector docIds = (BitVector) root.getVector(DOC_IDS_COLUMN); docIds.allocateNew(0); docIds.setValueCount(0); root.setRowCount(0); return Collections.singletonList(root).iterator(); } private Iterator<VectorSchemaRoot> createResultFromBitSet(FixedBitSet bitSet, int totalMaxDoc) { BufferAllocator alloc = getAllocator(); VectorSchemaRoot root = VectorSchemaRoot.create(DOC_IDS_SCHEMA, alloc); BitVector docIds = (BitVector) root.getVector(DOC_IDS_COLUMN); docIds.allocateNew(totalMaxDoc); for (int i = 0; i < totalMaxDoc; i++) { docIds.setSafe(i, bitSet.get(i) ? 1 : 0); } docIds.setValueCount(totalMaxDoc); root.setRowCount(totalMaxDoc); return Collections.singletonList(root).iterator(); } Double Deserialization In execute(byte[]), QueryBuilderSerializer.deserialize(fragment) is called purely for validation (line 168), and then getOrCreateWeight calls deserialize again internally (line 125). This means every first call to execute deserializes the fragment twice. The result of the first deserialization should be used or the validation call removed. public Iterator<VectorSchemaRoot> execute(byte[] fragment) { if (fragment == null || fragment.length == 0) { throw new IllegalArgumentException("Fragment byte array must not be null or empty"); } // Validate the bytes are a well-formed QueryBuilder before proceeding QueryBuilderSerializer.deserialize(fragment); if (engineSearcher == null) { return createEmptyResult(); } return executeWithCachedSearcher(fragment); } Duplicate Pattern In initialize(), the pattern-matching instanceof check on line 75 (if (context instanceof DefaultShardExecutionContext shardCtx == false)) is followed immediately by a redundant old-style cast on line 81 (DefaultShardExecutionContext shardCtx = (DefaultShardExecutionContext) context). The pattern variable from the instanceof check is already in scope and should be used directly, eliminating the second cast. if (context instanceof DefaultShardExecutionContext shardCtx == false) { throw new IllegalArgumentException( "LuceneEngineBridge requires DefaultShardExecutionContext, got: " + (context == null ? "null" : context.getClass().getSimpleName()) ); } DefaultShardExecutionContext shardCtx = (DefaultShardExecutionContext) context; Engine.Searcher searcher = shardCtx.indexShard().acquireSearcher("lucene-analytics"); Escape Handling Missing translateSqlWildcards converts SQL % → * and _ → ? but does not handle the SQL LIKE escape character (e.g., LIKE 'foo\_bar' ESCAPE '\'). A literal underscore or percent escaped in SQL will be incorrectly translated into a Lucene wildcard character, producing wrong query results. SQL LIKE with an ESCAPE clause is a valid Calcite RexCall with 3 operands; canHandle does not check for this and will accept it, leading to silent mis-translation. static String translateSqlWildcards(String sqlPattern) { StringBuilder sb = new StringBuilder(sqlPattern.length()); for (int i = 0; i < sqlPattern.length(); i++) { char c = sqlPattern.charAt(i); if (c == '%') { sb.append('*'); } else if (c == '_') { sb.append('?'); } else { sb.append(c); } } return sb.toString(); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 949e466

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix inconsistent negated pattern binding and redundant cast The pattern binding context instanceof DefaultShardExecutionContext shardCtx == false is a negated pattern match, but the variable shardCtx is only in scope inside the if block (the false branch), so the subsequent cast (DefaultShardExecutionContext) context is redundant and inconsistent. Use a positive pattern match instead to bind the variable for use after the guard. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LuceneEngineBridge.java [75-81] -if (context instanceof DefaultShardExecutionContext shardCtx == false) { +if (!(context instanceof DefaultShardExecutionContext shardCtx)) { throw new IllegalArgumentException( "LuceneEngineBridge requires DefaultShardExecutionContext, got: " + (context == null ? "null" : context.getClass().getSimpleName()) ); } -DefaultShardExecutionContext shardCtx = (DefaultShardExecutionContext) context; Suggestion importance[1-10]: 7 __ Why: The negated pattern binding context instanceof DefaultShardExecutionContext shardCtx == false leaves shardCtx out of scope after the guard, making the subsequent (DefaultShardExecutionContext) context cast redundant. Using !(context instanceof DefaultShardExecutionContext shardCtx) is cleaner and avoids the unnecessary cast.	Medium
	Handle SQL escape sequences and Lucene special character escaping The translation does not handle SQL LIKE escape characters (e.g., ESCAPE ''). A literal % or _ escaped in SQL (e.g., %) would be incorrectly translated to a Lucene wildcard or ? instead of being treated as a literal character. Additionally, Lucene wildcard special characters in the literal portion of the pattern (e.g., , ?, </code>) are not escaped, which could produce unintended wildcard matches. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LikePredicateHandler.java [100-113] static String translateSqlWildcards(String sqlPattern) { - StringBuilder sb = new StringBuilder(sqlPattern.length()); + StringBuilder sb = new StringBuilder(sqlPattern.length() * 2); for (int i = 0; i < sqlPattern.length(); i++) { char c = sqlPattern.charAt(i); + if (c == '\\' && i + 1 < sqlPattern.length()) { + char next = sqlPattern.charAt(i + 1); + if (next == '%' || next == '_') { + // Escaped SQL wildcard → escape as Lucene literal + sb.append('\\').append(next); + i++; + continue; + } + } if (c == '%') { sb.append('*'); } else if (c == '_') { sb.append('?'); + } else if (c == '*' || c == '?' || c == '\\') { + // Escape Lucene special chars in literal portions + sb.append('\\').append(c); } else { sb.append(c); } } return sb.toString(); } Suggestion importance[1-10]: 6 __ Why: The current translateSqlWildcards does not handle SQL escape characters or escape Lucene special characters (*, ?, \) in literal portions of the pattern, which could produce incorrect wildcard matches. The improved code correctly handles these edge cases.	Low
	Prevent cache corruption from mutable array-backed cache keys ByteBuffer.wrap(fragment) creates a view over the original array without copying it. If the caller mutates the fragment array after calling execute, the ByteBuffer key's content changes, breaking cache lookups and potentially corrupting the cache. Use ByteBuffer.wrap(fragment.clone()) or copy the bytes when constructing the cache key to ensure immutability. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LuceneEngineBridge.java [118-131] private Weight getOrCreateWeight(byte[] fragment) throws IOException { - ByteBuffer key = ByteBuffer.wrap(fragment); + ByteBuffer key = ByteBuffer.wrap(fragment.clone()); Weight cached = weightCache.get(key); if (cached != null) { return cached; } QueryBuilder queryBuilder = QueryBuilderSerializer.deserialize(fragment); Query query = queryBuilder.toQuery(queryShardContext); query = engineSearcher.rewrite(query); Weight weight = engineSearcher.createWeight(query, ScoreMode.COMPLETE_NO_SCORES, 1.0f); weightCache.put(key, weight); return weight; } Suggestion importance[1-10]: 5 __ Why: Using ByteBuffer.wrap(fragment) creates a view over the original array, so if the caller mutates the array, the cache key changes. Cloning the array for the key prevents this potential cache corruption, though in practice the callers in this codebase don't mutate the fragment after passing it.	Low
General	Document Arrow memory ownership to prevent off-heap leaks The returned VectorSchemaRoot (and its underlying Arrow buffers) is never closed by the caller in the production path — only in tests. The Iterator contract should document that the caller is responsible for closing each VectorSchemaRoot, or the bridge should implement AutoCloseable and track open roots. Without this, Arrow off-heap memory will leak whenever the iterator is consumed but the roots are not explicitly closed. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LuceneEngineBridge.java [290-303] +/** + * NOTE: Callers are responsible for closing each {@link VectorSchemaRoot} + * returned by the iterator to release Arrow off-heap memory. + */ private Iterator<VectorSchemaRoot> createResultFromBitSet(FixedBitSet bitSet, int totalMaxDoc) { BufferAllocator alloc = getAllocator(); VectorSchemaRoot root = VectorSchemaRoot.create(DOC_IDS_SCHEMA, alloc); BitVector docIds = (BitVector) root.getVector(DOC_IDS_COLUMN); docIds.allocateNew(totalMaxDoc); for (int i = 0; i < totalMaxDoc; i++) { docIds.setSafe(i, bitSet.get(i) ? 1 : 0); } docIds.setValueCount(totalMaxDoc); root.setRowCount(totalMaxDoc); return Collections.singletonList(root).iterator(); } Suggestion importance[1-10]: 2 __ Why: The suggestion only adds a comment/docstring to document caller responsibility for closing VectorSchemaRoot. This is a documentation-only change and does not fix any code logic, so it scores low per the guidelines.	Low

---

Previous suggestions

Suggestions up to commit 604c419

Category	Suggestion	Impact
Possible issue	Fix incorrect pattern-matching instanceof usage The pattern-matching instanceof with == false assigns shardCtx only in the false branch (i.e., when the cast fails), so the variable is out of scope after the if block. The subsequent cast (DefaultShardExecutionContext) context is redundant and confusing. Use a standard instanceof check followed by a direct cast, or use the Java 16+ pattern variable correctly. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LuceneEngineBridge.java [75-81] -if (context instanceof DefaultShardExecutionContext shardCtx == false) { +if (!(context instanceof DefaultShardExecutionContext)) { throw new IllegalArgumentException( "LuceneEngineBridge requires DefaultShardExecutionContext, got: " + (context == null ? "null" : context.getClass().getSimpleName()) ); } DefaultShardExecutionContext shardCtx = (DefaultShardExecutionContext) context; Suggestion importance[1-10]: 7 __ Why: The pattern context instanceof DefaultShardExecutionContext shardCtx == false is a valid Java 16+ pattern that binds shardCtx only when the condition is false (i.e., when the cast succeeds but the negation makes it the else branch). Actually, this syntax is incorrect — the pattern variable shardCtx would only be in scope in the false branch, making the subsequent explicit cast redundant and confusing. The suggestion correctly identifies this issue and proposes a cleaner standard instanceof check.	Medium
	Escape Lucene metacharacters in wildcard translation The translation does not escape Lucene wildcard metacharacters (, ?, </code>) that may appear literally in the SQL pattern. A SQL LIKE pattern where the user intends a literal or ? character (which are not SQL wildcards) will be incorrectly treated as Lucene wildcards, producing wrong results. Literal * and ? in the SQL pattern should be escaped with a backslash in the Lucene pattern. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LikePredicateHandler.java [104-117] static String translateSqlWildcards(String sqlPattern) { - StringBuilder sb = new StringBuilder(sqlPattern.length()); + StringBuilder sb = new StringBuilder(sqlPattern.length() + 4); for (int i = 0; i < sqlPattern.length(); i++) { char c = sqlPattern.charAt(i); if (c == '%') { sb.append('*'); } else if (c == '_') { sb.append('?'); + } else if (c == '*' || c == '?' || c == '\\') { + // Escape Lucene metacharacters that are not SQL wildcards + sb.append('\\'); + sb.append(c); } else { sb.append(c); } } return sb.toString(); } Suggestion importance[1-10]: 7 __ Why: This is a valid correctness concern — SQL LIKE patterns containing literal * or ? characters (which are not SQL wildcards) would be incorrectly interpreted as Lucene wildcards, producing wrong query results. The fix properly escapes these Lucene metacharacters.	Medium
General	Prevent Arrow buffer leaks on allocation failure The VectorSchemaRoot objects returned in the iterators are never closed by the bridge itself — the caller must close them. However, if an exception occurs between creating root and returning it, the VectorSchemaRoot (and its underlying Arrow buffers) will be leaked. Wrap the allocation in a try-catch to close on failure. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LuceneEngineBridge.java [280-303] private Iterator<VectorSchemaRoot> createEmptyResult() { BufferAllocator alloc = getAllocator(); VectorSchemaRoot root = VectorSchemaRoot.create(DOC_IDS_SCHEMA, alloc); - BitVector docIds = (BitVector) root.getVector(DOC_IDS_COLUMN); - docIds.allocateNew(0); - docIds.setValueCount(0); - root.setRowCount(0); - return Collections.singletonList(root).iterator(); + try { + BitVector docIds = (BitVector) root.getVector(DOC_IDS_COLUMN); + docIds.allocateNew(0); + docIds.setValueCount(0); + root.setRowCount(0); + return Collections.singletonList(root).iterator(); + } catch (Exception e) { + root.close(); + throw e; + } } private Iterator<VectorSchemaRoot> createResultFromBitSet(FixedBitSet bitSet, int totalMaxDoc) { BufferAllocator alloc = getAllocator(); VectorSchemaRoot root = VectorSchemaRoot.create(DOC_IDS_SCHEMA, alloc); - ... - return Collections.singletonList(root).iterator(); + try { + BitVector docIds = (BitVector) root.getVector(DOC_IDS_COLUMN); + docIds.allocateNew(totalMaxDoc); + for (int i = 0; i < totalMaxDoc; i++) { + docIds.setSafe(i, bitSet.get(i) ? 1 : 0); + } + docIds.setValueCount(totalMaxDoc); + root.setRowCount(totalMaxDoc); + return Collections.singletonList(root).iterator(); + } catch (Exception e) { + root.close(); + throw e; + } } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies a potential resource leak if an exception occurs after VectorSchemaRoot.create but before returning. However, in practice these methods are simple and unlikely to throw, making this a minor defensive improvement rather than a critical fix.	Low
	Eliminate redundant deserialization in execute method The execute method eagerly deserializes the fragment for validation even when engineSearcher == null, and then executeWithCachedSearcher will deserialize it again via getOrCreateWeight. This double-deserialization is wasteful. More importantly, the early validation deserialization result is discarded, so it provides no benefit over letting getOrCreateWeight handle it. Remove the redundant deserialization call. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LuceneEngineBridge.java [162-175] @Override public Iterator<VectorSchemaRoot> execute(byte[] fragment) { if (fragment == null || fragment.length == 0) { throw new IllegalArgumentException("Fragment byte array must not be null or empty"); } - // Validate the bytes are a well-formed QueryBuilder before proceeding - QueryBuilderSerializer.deserialize(fragment); - if (engineSearcher == null) { + // Still validate the bytes are well-formed even without a searcher + QueryBuilderSerializer.deserialize(fragment); return createEmptyResult(); } return executeWithCachedSearcher(fragment); } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that the validation deserialization is performed even when engineSearcher != null, causing double-deserialization. However, the improved code moves validation only to the engineSearcher == null branch, which means when a searcher is present, validation is skipped entirely and errors surface later in getOrCreateWeight. This is a minor optimization with a tradeoff in validation behavior.	Low

Suggestions up to commit fb880d1

Category	Suggestion	Impact
Possible issue	Handle escaped wildcard characters in LIKE patterns The translateSqlWildcards method does not handle SQL LIKE escape characters (e.g., ESCAPE ''). A literal % or in the pattern (escaped as % or _) would be incorrectly translated to * or ? in the Lucene wildcard pattern, producing wrong query results. The method should handle escape sequences to correctly pass through literal % and characters. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LikePredicateHandler.java [104-117] static String translateSqlWildcards(String sqlPattern) { StringBuilder sb = new StringBuilder(sqlPattern.length()); for (int i = 0; i < sqlPattern.length(); i++) { char c = sqlPattern.charAt(i); + if (c == '\\' && i + 1 < sqlPattern.length()) { + char next = sqlPattern.charAt(i + 1); + if (next == '%' || next == '_') { + sb.append(next); // literal % or _ + i++; + continue; + } + } if (c == '%') { sb.append('*'); } else if (c == '_') { sb.append('?'); } else { sb.append(c); } } return sb.toString(); } Suggestion importance[1-10]: 7 __ Why: The translateSqlWildcards method does not handle SQL escape sequences, so escaped \% or \_ would be incorrectly translated to Lucene wildcards * or ?, producing incorrect query results for patterns with literal % or _ characters.	Medium
	Fix negated pattern variable binding redundancy The pattern context instanceof DefaultShardExecutionContext shardCtx == false is a negated pattern variable binding, but the variable shardCtx is then re-declared via a cast below. This is redundant and potentially confusing. Use a standard instanceof check and a single cast, or use the pattern variable directly in the else branch. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LuceneEngineBridge.java [75-81] -if (context instanceof DefaultShardExecutionContext shardCtx == false) { +if (!(context instanceof DefaultShardExecutionContext)) { throw new IllegalArgumentException( "LuceneEngineBridge requires DefaultShardExecutionContext, got: " + (context == null ? "null" : context.getClass().getSimpleName()) ); } DefaultShardExecutionContext shardCtx = (DefaultShardExecutionContext) context; Suggestion importance[1-10]: 6 __ Why: The code uses a negated pattern variable shardCtx in the if condition but then re-declares it via a cast on line 81, which is redundant and confusing. The suggested fix is cleaner and avoids the unusual negated pattern binding syntax.	Low
General	Avoid redundant deserialization on every execute call The execute method deserializes the fragment twice: once for validation here, and again inside getOrCreateWeight via executeWithCachedSearcher. This is wasteful and can be avoided since getOrCreateWeight already handles deserialization with caching. The upfront validation deserialization should be removed, as executeWithCachedSearcher will throw an IllegalArgumentException on corrupt bytes anyway. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LuceneEngineBridge.java [161-168] -// Validate the bytes are a well-formed QueryBuilder before proceeding -QueryBuilderSerializer.deserialize(fragment); - if (engineSearcher == null) { + // Still validate bytes are well-formed before returning empty result + QueryBuilderSerializer.deserialize(fragment); return createEmptyResult(); } return executeWithCachedSearcher(fragment); Suggestion importance[1-10]: 5 __ Why: The fragment is deserialized twice when engineSearcher is not null — once for validation and again inside getOrCreateWeight. The improved code moves validation only to the engineSearcher == null branch, avoiding the redundant deserialization in the common path.	Low
	Simplify total doc count calculation The totalMaxDoc is computed by summing maxDoc() across all leaves, but the FixedBitSet is indexed by leaf.docBase + doc. The last leaf's docBase + maxDoc equals the index-level maxDoc, which is also available directly as engineSearcher.getIndexReader().maxDoc(). Using the reader's maxDoc() directly is simpler and avoids the manual summation loop. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LuceneEngineBridge.java [218-237] private Iterator<VectorSchemaRoot> executeWithCachedSearcher(byte[] fragment) { try { Weight weight = getOrCreateWeight(fragment); - int totalMaxDoc = 0; - for (LeafReaderContext leaf : engineSearcher.getIndexReader().leaves()) { - totalMaxDoc += leaf.reader().maxDoc(); - } - + int totalMaxDoc = engineSearcher.getIndexReader().maxDoc(); FixedBitSet bitSet = new FixedBitSet(totalMaxDoc); for (LeafReaderContext leaf : engineSearcher.getIndexReader().leaves()) { Scorer scorer = weight.scorer(leaf); if (scorer != null) { DocIdSetIterator it = scorer.iterator(); for (int doc = it.nextDoc(); doc != DocIdSetIterator.NO_MORE_DOCS; doc = it.nextDoc()) { bitSet.set(leaf.docBase + doc); } } } Suggestion importance[1-10]: 4 __ Why: Using engineSearcher.getIndexReader().maxDoc() directly is simpler and equivalent to the manual summation loop, reducing code complexity without changing behavior.	Low

Suggestions up to commit 4bf3e64

Category	Suggestion	Impact
Possible issue	Fix invalid negated pattern variable binding syntax The pattern context instanceof DefaultShardExecutionContext shardCtx == false is a negated pattern variable binding, which is not valid Java syntax and will fail to compile. The pattern variable shardCtx is only in scope in the false branch (the throw), not after the block. Use a standard instanceof check followed by a cast, or restructure the logic. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LuceneEngineBridge.java [74-80] -if (context instanceof DefaultShardExecutionContext shardCtx == false) { +if (!(context instanceof DefaultShardExecutionContext)) { throw new IllegalArgumentException( "LuceneEngineBridge requires DefaultShardExecutionContext, got: " + (context == null ? "null" : context.getClass().getSimpleName()) ); } DefaultShardExecutionContext shardCtx = (DefaultShardExecutionContext) context; Suggestion importance[1-10]: 8 __ Why: The pattern context instanceof DefaultShardExecutionContext shardCtx == false is not valid Java syntax — pattern variables in negated instanceof checks are not in scope after the block. This would cause a compilation error. The fix correctly uses !(context instanceof DefaultShardExecutionContext) followed by a cast.	Medium
	Escape Lucene metacharacters in wildcard translation The translateSqlWildcards method does not handle Lucene special characters (e.g., , ?, </code>) that may appear in the SQL literal value. If the input pattern contains a literal or ? (not as SQL wildcards), they will be misinterpreted by Lucene as wildcards, leading to incorrect query results. Characters that are Lucene wildcard metacharacters in the non-wildcard positions should be escaped with a backslash. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LikePredicateHandler.java [103-117] static String translateSqlWildcards(String sqlPattern) { - StringBuilder sb = new StringBuilder(sqlPattern.length()); + StringBuilder sb = new StringBuilder(sqlPattern.length() * 2); for (int i = 0; i < sqlPattern.length(); i++) { char c = sqlPattern.charAt(i); if (c == '%') { sb.append('*'); } else if (c == '_') { sb.append('?'); + } else if (c == '*' || c == '?' || c == '\\') { + // Escape Lucene wildcard metacharacters that are literal in SQL + sb.append('\\'); + sb.append(c); } else { sb.append(c); } } return sb.toString(); } Suggestion importance[1-10]: 7 __ Why: This is a real correctness issue: if a SQL LIKE pattern contains literal *, ?, or \ characters (not as SQL wildcards), they would be misinterpreted by Lucene's WildcardQueryBuilder, producing incorrect query results. The fix correctly escapes these Lucene metacharacters.	Medium
General	Document Arrow memory ownership for returned results The VectorSchemaRoot objects created in createEmptyResult and createResultFromBitSet are returned to the caller as iterators but are never closed by the bridge itself. The caller in tests calls root.close(), but there is no contract enforcing this, and if an exception occurs after retrieval the memory will leak. Consider wrapping the iterator in a closeable or documenting clearly that the caller must close each VectorSchemaRoot. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LuceneEngineBridge.java [183-206] +// Document clearly in the method Javadoc that callers are responsible for closing each VectorSchemaRoot +/** + * Caller is responsible for closing each {@link VectorSchemaRoot} returned by the iterator + * to release Arrow memory. + */ private Iterator<VectorSchemaRoot> createEmptyResult() { BufferAllocator alloc = getAllocator(); VectorSchemaRoot root = VectorSchemaRoot.create(DOC_IDS_SCHEMA, alloc); BitVector docIds = (BitVector) root.getVector(DOC_IDS_COLUMN); docIds.allocateNew(0); docIds.setValueCount(0); root.setRowCount(0); return Collections.singletonList(root).iterator(); } -private Iterator<VectorSchemaRoot> createResultFromBitSet(FixedBitSet bitSet, int totalMaxDoc) { - BufferAllocator alloc = getAllocator(); - VectorSchemaRoot root = VectorSchemaRoot.create(DOC_IDS_SCHEMA, alloc); - ... -} - Suggestion importance[1-10]: 3 __ Why: This is a valid concern about memory ownership for VectorSchemaRoot objects, but the suggestion only adds a comment/Javadoc without changing the actual code logic. The improved_code is essentially the same as existing_code with a comment added, making the impact minimal.	Low
	Clarify coordinator-side null MapperService usage On the coordinator side, mapperService is null (it is only set during initialize). The RexToQueryBuilderConverter accepts a null mapperService, but if any handler internally uses it (e.g., future handlers), a NullPointerException will occur silently. More critically, the convertFragment method is documented as coordinator-side, but it will also be callable after initialize on the data node where mapperService is set — this dual use may cause inconsistent behavior. Consider explicitly passing null or a dedicated coordinator-mode factory to make the intent clear and guard against accidental data-node-side calls. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LuceneEngineBridge.java [113-131] @Override public byte[] convertFragment(RelNode fragment) { Objects.requireNonNull(fragment, "RelNode fragment must not be null"); - ... - RexToQueryBuilderConverter converter = new RexToQueryBuilderConverter(inputRowType, mapperService); + + if (!(fragment instanceof LogicalFilter)) { + throw new IllegalArgumentException( + "Lucene backend expects a LogicalFilter, got: " + fragment.getClass().getSimpleName() + ); + } + + LogicalFilter filter = (LogicalFilter) fragment; + RexNode condition = filter.getCondition(); + RelDataType inputRowType = filter.getInput().getRowType(); + + // convertFragment is coordinator-side: mapperService is intentionally null here. + // Field type validation falls back to Calcite SqlTypeName checks. + RexToQueryBuilderConverter converter = new RexToQueryBuilderConverter(inputRowType, null); QueryBuilder queryBuilder = converter.convert(condition); + return QueryBuilderSerializer.serialize(queryBuilder); } Suggestion importance[1-10]: 2 __ Why: The suggestion only changes mapperService to null explicitly in convertFragment, but the existing code already passes mapperService which is null on the coordinator side. The improved_code is functionally equivalent to the existing code, making this a documentation-only change with minimal impact.	Low

[github-actions]

❌ Gradle check result for 4bf3e64: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit fb880d1

[github-actions]

❌ Gradle check result for fb880d1: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 604c419

[github-actions]

❌ Gradle check result for 604c419: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 949e466

[github-actions]

❌ Gradle check result for 949e466: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[sandeshkr419]

Closing in lieu of #21555