Implement spath command with field resolution #5028
Conversation
ykmr1224
added
PPL
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a field-resolution framework (context, result model, visitor) integrated into Calcite planning for SPATH extract-all flows, AST and debug utilities, append UDF refactor and registration, JSON_EXTRACT_ALL input handling improvements, parser/docs/test updates, and many new/expanded tests. Changes
Sequence Diagram(s)sequenceDiagram
participant QS as QueryService
participant CP as CalcitePlanContext
participant FRV as FieldResolutionVisitor
participant CRV as CalciteRelNodeVisitor
participant AST as UnresolvedPlan (root)
participant ES as OpenSearch
QS->>CP: setRootNode(plan)
QS->>CRV: analyze(plan)
CRV->>CP: resolveFields(target)
CP->>FRV: analyze(rootNode)
FRV->>AST: traverse -> compute Map<UnresolvedPlan,FieldResolutionResult>
FRV-->>CP: return map
CP-->>CRV: FieldResolutionResult for target
alt spath without path
CRV->>CRV: build extract-all projection from resolved regular fields
CRV->>ES: execute JSON_EXTRACT_ALL + ITEM + CAST per projected field
ES-->>CRV: rows
else spath with path
CRV->>ES: execute path-based extraction
ES-->>CRV: rows
end
CRV-->>QS: generated RelNode / plan
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
Comment |
ykmr1224
force-pushed
the
spath/field-resolution
branch
2 times, most recently
from
15b3138 to
a90e097
Compare
ykmr1224
requested review from
GumpacG,
LantaoJin,
MaxKsyunz,
RyanL1997,
Swiddis,
YANG-DB,
Yury-Fridlyand,
acarbonetto,
anirudha,
dai-chen,
derek-ho,
forestmvey,
joshuali925,
kavithacm,
mengweieric,
noCharger,
penghuo,
ps48,
qianheng-aws,
seankao-az,
vamsimanohar and
yuancu
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
integ-test/src/test/java/org/opensearch/sql/calcite/standalone/MapAppendFunctionIT.java (1)
86-93: Test method has unused parameters and will fail to run.The
@Testannotated methodtestMapAppendWithSingleNullhas parameters(RexNode map1, RexNode map2)which are never used. JUnit 4/5 cannot inject these parameters for a regular@Testmethod, causing the test to fail or be skipped. The method body creates its ownmapandnullMapvariables.🐛 Proposed fix
@Test - public void testMapAppendWithSingleNull(RexNode map1, RexNode map2) throws Exception { + public void testMapAppendWithSingleNull() throws Exception { RelDataType mapType = createMapType(context.rexBuilder); RexNode nullMap = context.rexBuilder.makeNullLiteral(mapType); RexNode map = createMap("key1", "value1"); testWithSingleNull(map, nullMap); testWithSingleNull(nullMap, map); }
🤖 Fix all issues with AI agents
In @common/src/main/java/org/opensearch/sql/common/utils/DebugUtils.java:
- Around line 29-33: getCalledFrom currently constructs a RuntimeException and
indexes e.getStackTrace()[pos + 1] which can throw
ArrayIndexOutOfBoundsException and is expensive; replace the
RuntimeException-based stack capture with
Thread.currentThread().getStackTrace(), add a bounds check verifying (pos + 1) <
stack.length and return a safe fallback string (e.g., "unknown") when out of
range, and add a brief comment on performance; also review callers like debug()
to avoid invoking getCalledFrom in hot paths or make its use conditional so you
don't pay the stack-capture cost unnecessarily.
In
@core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java:
- Around line 312-314: The isPrefixed method in FieldResolutionVisitor
incorrectly treats any dotted name (like nested JSON path "address.city") as
table-qualified; change isPrefixed to only return true for explicit
table-qualified identifiers by matching a leading identifier plus a dot (e.g.
using a regex like "^[A-Za-z_][A-Za-z0-9_]*\\.") so only names that start with a
table/alias and a dot are considered prefixed; update references to isPrefixed
in the join field filtering logic so nested JSON paths are not dropped.
In @core/src/main/java/org/opensearch/sql/ast/AstNodeUtils.java:
- Around line 1-39: Add unit tests for AstNodeUtils.containsSubqueryExpression
by creating AstNodeUtilsTest.java that exercises: null input returns false;
passing a SubqueryExpression instance returns true; a Let instance wrapping a
SubqueryExpression is unwrapped (create Let l with getExpression() returning a
SubqueryExpression and assert true); recursive detection through children
(create a mock or simple Node with getChild() returning a list containing a
SubqueryExpression and assert true); and nested/edge cases (deeply nested Nodes
without subqueries return false, and mixed nested structures with one subquery
return true). Use existing Node, Let, and SubqueryExpression
classes/constructors or minimal test stubs/mocks and JUnit assertions to
validate each scenario.
In @core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java:
- Around line 216-223: Add JavaDoc to the public method resolveFields describing
purpose, parameters, return type and that it either returns the
FieldResolutionResult for the given UnresolvedPlan or throws when the plan is
not part of the analyzed tree; then add explicit null/absent handling after the
visitor.analyze(rootNode) call: validate that target is non-null, check
result.containsKey(target) and if absent throw an IllegalArgumentException (or
another documented runtime exception) with a clear message, otherwise return
result.get(target). Reference: CalcitePlanContext.resolveFields,
FieldResolutionVisitor.analyze, FieldResolutionResult, UnresolvedPlan, rootNode.
In
@core/src/main/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImpl.java:
- Line 46: Remove the unused constant EMPTY from the JsonExtractAllFunctionImpl
class: locate the field declaration "private static final String EMPTY = \"\";"
in JsonExtractAllFunctionImpl and delete it, ensuring no other references to
EMPTY exist in the class; run a build or search to confirm there are no
remaining usages and commit the simplified class.
- Around line 63-66: Add a JavaDoc comment to the public method
getOperandMetadata() in JsonExtractAllFunctionImpl: document that it returns
metadata specifying the function accepts STRING or ARRAY operands, describe how
STRING and ARRAY inputs are treated (strings directly, arrays concatenated to
strings), and include an @return tag stating it returns UDFOperandMetadata
specifying STRING or ARRAY operand types; place the Javadoc immediately above
the getOperandMetadata() method declaration and keep wording consistent with
other public method docs in the codebase.
In
@core/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.java:
- Line 395: Function registration names in PPLBuiltinOperators (e.g.,
MAP_APPEND, MVAPPEND, MAP_REMOVE) are inconsistently capitalized versus
documentation (lowercase) which will break case-sensitive SQL matching; update
the .toUDF(...) calls for these operators to use lowercase names (e.g., change
MAP_APPEND -> "map_append", MVAPPEND -> "mvappend", MAP_REMOVE -> "map_remove")
and ensure all other registrations in PPLBuiltinOperators.java follow the same
lowercase convention so registration matches the documented examples.
In
@core/src/test/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImplTest.java:
- Line 87: The testNullInput method in JsonExtractAllFunctionImplTest is empty;
either implement meaningful assertions for the intended behavior of
jsonExtractAll when passed null (e.g., assert it returns an empty list or null
as per spec) or remove the method; if the intent is to assert that passing null
does not throw, call the target method inside the test and assert no exception
(or use an assertion like assertDoesNotThrow) and include a clear assertion
about the expected return value or state.
In @integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java:
- Around line 53-54: In PPLIntegTestCase remove the unconditional
System.out.println calls (the two lines printing query and
explainQueryYaml(query)) or replace them with controlled logging; specifically,
either delete those lines or call LOG.debug(...) instead of System.out and avoid
calling explainQueryYaml(query) unconditionally (only call it when a debug
flag/system property is set) so you do not trigger extra HTTP explain requests
for every test; ensure you reference the existing LOG logger and gate the
explainQueryYaml(query) invocation behind a boolean system property or
environment variable if diagnostic output is required.
In @integ-test/src/test/java/org/opensearch/sql/util/MatcherUtils.java:
- Around line 304-306: The public helper method array(Object... objects) lacks
JavaDoc and has a generic name; add a concise JavaDoc above the method
describing that it creates and returns an org.json.JSONArray from the given
varargs and documents parameters/return value and any null/empty behavior, and
optionally rename the method to jsonArray(Object... objects) for clarity; if you
rename it, update all references/usages throughout the codebase to use jsonArray
and keep the original behavior (new JSONArray(objects)).
In
@ppl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.java:
- Around line 256-261: Replace the Java language assertion in
FieldResolutionVisitorTest (the line using assert(fields.contains("parent_id")))
with a JUnit assertion to ensure runtime consistency; specifically, use
assertTrue from the test class (or org.junit.Assert.assertTrue) to check
fields.contains("parent_id") in the same test method so it behaves like the
other assertions (e.g., assertEquals) in that test.
🧹 Nitpick comments (19)
docs/user/ppl/cmd/spath.md (1)
18-26: Consider adding an example for the filtering limitation (line 25).The field resolution syntax lists four limitations, but Example 8 only demonstrates two of them (missing field identification and wildcard restrictions). The filtering limitation stated in line 25 ("Filter with query (
where <field> in/exists [...]) is not supported afterspathcommand") would benefit from a concrete example showing what fails and why, similar to the pattern used in Example 8.If space permits, consider expanding Example 8 with a third error case:
source=structured | spath input=doc_multi | where a > 10 # ERROR: Filtering after spath is not supportedAlternatively, clarify whether this limitation applies only to field resolution mode or both modes, as the current wording suggests it applies universally to all spath commands.
common/src/main/java/org/opensearch/sql/common/utils/DebugUtils.java (2)
15-17: Replace System.out with proper logging framework.The
System.out.println, which bypasses the application's logging configuration and cannot be controlled or filtered in production environments. Since this codebase uses Log4j (as seen in other files), consider using a proper logger instance.♻️ Refactor to use Log4j
+import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + public class DebugUtils { + private static final Logger LOG = LogManager.getLogger(); - private static void print(String format, Object... args) { - System.out.println(String.format(format, args)); + private static void print(String format, Object... args) { + LOG.debug(String.format(format, args)); }
35-72: Consider improvements to stringify methods.A few optional enhancements for robustness:
Inconsistent null handling: Collections return
"null"(line 37) while Maps return"[[null]]"(line 51).Circular reference protection: Recursive calls to
stringify(lines 44, 60) could causeStackOverflowErrorif objects contain circular references. Consider adding a visited set or depth limit.Raw type casts: Lines 67 and 69 use unchecked casts
(Collection)and(Map)which generate compiler warnings. These can be replaced with(Collection<?>)and(Map<?, ?>).Array handling: Arrays are not handled specially and will print poorly (hashcode instead of contents). Consider adding
if (obj.getClass().isArray())check.core/src/main/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImpl.java (1)
102-111: Consider adding a clarifying comment.The method correctly filters nulls and concatenates array elements without a delimiter. While the implementation is sound, the concatenation behavior (joining with no delimiter) is non-obvious and specific to handling JSON fragments split across array elements.
📝 Suggested documentation addition
+ /** + * Converts a list to a concatenated string by joining elements without delimiter. + * This supports JSON fragments split across array elements (e.g., ["{\"a\":", "\"b\"}"]). + * Null elements are filtered out. + */ private static String convertArrayToString(List<?> array) {core/src/main/java/org/opensearch/sql/executor/QueryService.java (1)
255-258: Correct integration of rootNode for field resolution.The addition of
context.setRootNode(plan)before analysis correctly enables field-resolution-aware analysis as described in the PR objectives. The placement is appropriate and the implementation is clean.For improved maintainability, consider adding a note to the method's JavaDoc documenting that this method mutates the
CalcitePlanContextby storing the root plan node:📝 Optional JavaDoc enhancement
+ /** + * Analyze an UnresolvedPlan and convert it to a Calcite RelNode. + * + * @param plan the unresolved plan to analyze + * @param context the Calcite plan context (will be mutated to store the root node) + * @return the analyzed RelNode + */ public RelNode analyze(UnresolvedPlan plan, CalcitePlanContext context) { context.setRootNode(plan); return getRelNodeVisitor().analyze(plan, context); }core/src/test/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImplTest.java (1)
48-53: Optional: Consider testing deeply nested array scenarios.While array flattening is tested, consider adding a test case for deeply nested arrays to verify the flattening behavior at multiple levels.
💡 Optional test case for nested arrays
@Test public void testAppendWithDeeplyNestedArrays() { List<Object> nested = Arrays.asList(Arrays.asList(1, 2), 3); Object result = AppendFunctionImpl.append(nested, 4); // Define expected behavior for nested structures // assertEquals(expected, result); }core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendFunctionImpl.java (1)
103-106: Fix JavaDoc reference syntax.The
@reftag is not a valid JavaDoc tag. Use{@link}instead.📝 Suggested fix
/** * Collect non-null elements from `args`. If an item is a list, it will collect non-null elements - * of the list. See {@ref MVAppendFunctionImplTest} for detailed behavior. + * of the list. See {@link MVAppendFunctionImplTest} for detailed behavior. */core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendCore.java (1)
16-19: Fix JavaDoc reference syntax.Same as in
MVAppendFunctionImpl, use{@link}instead of@ref.📝 Suggested fix
/** * Collect non-null elements from `args`. If an item is a list, it will collect non-null elements - * of the list. See {@ref AppendFunctionImplTest} for detailed behavior. + * of the list. See {@link AppendFunctionImplTest} for detailed behavior. */core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImpl.java (2)
47-50: Consider documenting whygetOperandMetadatareturns null.Returning
nullfor operand metadata suggests no constraints on operand types/count, but this isn't explicitly documented. A brief comment or JavaDoc would clarify this is intentional (accepts any number of arguments of any type).💡 Suggested improvement
+ /** + * Returns null to indicate no operand constraints - accepts any number of arguments of any type. + */ @Override public UDFOperandMetadata getOperandMetadata() { return null; }
62-64: Consider adding JavaDoc for the publicappendmethod.Per coding guidelines, public methods in core/src/main/java should have JavaDoc with @param, @return, and @throws. This static method is part of the public API surface.
💡 Suggested JavaDoc
+ /** + * Collects non-null elements from arguments into a result. + * + * @param args variable arguments to collect + * @return null if no elements, single element if one element, or List if multiple elements + */ public static Object append(Object... args) { return AppendCore.collectElements(args); }core/src/test/java/org/opensearch/sql/ast/analysis/FieldResolutionResultTest.java (1)
227-233: PreferassertThrowsover try-catch for expected exceptions.JUnit 5's
assertThrowsprovides a cleaner and more idiomatic way to test expected exceptions.♻️ Suggested refactor
- try { - unmodifiable.add("field3"); - assertTrue(false, "Should throw UnsupportedOperationException"); - } catch (UnsupportedOperationException e) { - // Expected - } + assertThrows(UnsupportedOperationException.class, () -> unmodifiable.add("field3"));core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionContext.java (3)
34-36:popRequirementsmay throwNoSuchElementExceptionon empty stack.Unlike
getCurrentRequirements, this method doesn't validate stack state before popping. If the stack is empty,ArrayDeque.pop()throwsNoSuchElementException. Consider adding validation for consistency.💡 Suggested improvement
public FieldResolutionResult popRequirements() { + if (requirementsStack.isEmpty()) { + throw new IllegalStateException("Cannot pop from empty requirements stack"); + } return requirementsStack.pop(); }
38-44: Use a more specific exception type.
RuntimeExceptionis too generic. ConsiderIllegalStateExceptionwhich better conveys that the method was called in an invalid state.♻️ Suggested fix
public FieldResolutionResult getCurrentRequirements() { if (requirementsStack.isEmpty()) { - throw new RuntimeException("empty stack"); + throw new IllegalStateException("Requirements stack is empty"); } else { return requirementsStack.peek(); } }
57-65: Consider movingmergeWildcardPatternsto a utility class.This static method appears to be a general-purpose utility that doesn't depend on instance state. It might be better placed in a utility class (e.g.,
WildcardUtils) for reusability and cohesion.core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java (3)
354-357: Potential null pointer or class cast issue invisitRex.The pattern value is cast directly to
Stringwithout null or type checking. Ifnode.getPattern()orgetValue()returns null, or if the value isn't a String, this will fail at runtime.💡 Suggested defensive check
public Node visitRex(Rex node, FieldResolutionContext context) { Set<String> rexFields = extractFieldsFromExpression(node.getField()); - String patternStr = (String) node.getPattern().getValue(); + Literal pattern = node.getPattern(); + if (pattern == null || pattern.getValue() == null) { + throw new IllegalArgumentException("Rex pattern cannot be null"); + } + String patternStr = pattern.getValue().toString(); List<String> namedGroups = RegexCommonUtils.getNamedGroupCandidates(patternStr);
1-613: Consider splitting this visitor into smaller components.At 613 lines, this file exceeds the 500-line guideline for organization review. Consider grouping related visit methods into helper classes or extracting the field extraction logic into a separate utility class. For example:
- Field extraction methods could be in a
FieldExtractionUtilsclass- Pass-through visit methods could potentially use a default mechanism
222-250:extractFieldsFromExpressionhandles many cases - consider documenting.This method handles Field, QualifiedName, Alias, Function, Span, Literal, and generic child traversal. A brief JavaDoc explaining the extraction strategy would improve maintainability.
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionResult.java (2)
125-126: Consider makingANY_WILDCARDandNULL_WILDCARDpublic.These wildcards are referenced in tests via the class name (
FieldResolutionResult.ANY_WILDCARD). While package-private access works since tests are in the same package, making thempublic static finalwould be more explicit and allow broader reuse.💡 Suggested change
- static Wildcard ANY_WILDCARD = + public static final Wildcard ANY_WILDCARD = new Wildcard() { // ... }; - static Wildcard NULL_WILDCARD = + public static final Wildcard NULL_WILDCARD = new Wildcard() { // ... };Also applies to: 148-149
170-184: Consider makingSingleWildcardpublic for external use.The test file directly uses
SingleWildcardin assertions. While this works due to package-level access, if external consumers need to create wildcard patterns, making this class public would be necessary.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
common/src/main/java/org/opensearch/sql/common/utils/DebugUtils.javacore/src/main/java/org/opensearch/sql/analysis/Analyzer.javacore/src/main/java/org/opensearch/sql/ast/AstNodeUtils.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionContext.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionResult.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendCore.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.javacore/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.javacore/src/main/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImpl.javacore/src/test/java/org/opensearch/sql/ast/analysis/FieldResolutionResultTest.javacore/src/test/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImplTest.javacore/src/test/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImplTest.javacore/src/test/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImplTest.javadocs/dev/ppl-commands.mddocs/user/ppl/cmd/spath.mddoctest/test_data/structured.jsoninteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/standalone/MapAppendFunctionIT.javainteg-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.javainteg-test/src/test/java/org/opensearch/sql/util/MatcherUtils.javainteg-test/src/test/resources/expectedOutput/calcite/explain_spath_without_path.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_spath_without_path.yamlppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorCoverageTest.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.java
🧰 Additional context used
📓 Path-based instructions (12)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
core/src/test/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImplTest.javacore/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.javainteg-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.javacore/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.javacore/src/test/java/org/opensearch/sql/ast/analysis/FieldResolutionResultTest.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendCore.javainteg-test/src/test/java/org/opensearch/sql/calcite/standalone/MapAppendFunctionIT.javappl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorCoverageTest.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionContext.javacore/src/test/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImplTest.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionResult.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImpl.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.javacore/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javacore/src/main/java/org/opensearch/sql/ast/AstNodeUtils.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImpl.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/analysis/Analyzer.javacore/src/test/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImplTest.javainteg-test/src/test/java/org/opensearch/sql/util/MatcherUtils.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.javacommon/src/main/java/org/opensearch/sql/common/utils/DebugUtils.javainteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
core/src/test/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImplTest.javacore/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.javainteg-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.javacore/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.javacore/src/test/java/org/opensearch/sql/ast/analysis/FieldResolutionResultTest.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendCore.javainteg-test/src/test/java/org/opensearch/sql/calcite/standalone/MapAppendFunctionIT.javappl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorCoverageTest.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionContext.javacore/src/test/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImplTest.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionResult.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImpl.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.javacore/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javacore/src/main/java/org/opensearch/sql/ast/AstNodeUtils.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImpl.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/analysis/Analyzer.javacore/src/test/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImplTest.javainteg-test/src/test/java/org/opensearch/sql/util/MatcherUtils.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.javacommon/src/main/java/org/opensearch/sql/common/utils/DebugUtils.javainteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.java
**/*Test.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*Test.java: All new business logic requires unit tests
Name unit tests with*Test.javasuffix in OpenSearch SQL
Files:
core/src/test/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImplTest.javacore/src/test/java/org/opensearch/sql/ast/analysis/FieldResolutionResultTest.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorCoverageTest.javacore/src/test/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImplTest.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.javacore/src/test/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImplTest.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
core/src/test/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImplTest.javainteg-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.javacore/src/test/java/org/opensearch/sql/ast/analysis/FieldResolutionResultTest.javainteg-test/src/test/java/org/opensearch/sql/calcite/standalone/MapAppendFunctionIT.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorCoverageTest.javacore/src/test/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImplTest.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javacore/src/test/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImplTest.javainteg-test/src/test/java/org/opensearch/sql/util/MatcherUtils.javainteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.java
core/src/main/java/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/**/*.java: - New functions MUST have unit tests in the same commit
Files:
core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.javacore/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendCore.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionContext.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionResult.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImpl.javacore/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javacore/src/main/java/org/opensearch/sql/ast/AstNodeUtils.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/analysis/Analyzer.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java
core/src/main/java/org/opensearch/sql/expression/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/org/opensearch/sql/expression/**/*.java: - New expression implementations must follow existing patterns
- Type handling must be consistent with project type system
- Error handling must use appropriate exception types
- Null handling must be explicit and documented
Files:
core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.javacore/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendCore.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImpl.javacore/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendFunctionImpl.java
integ-test/src/test/resources/**/*
⚙️ CodeRabbit configuration file
integ-test/src/test/resources/**/*: - Verify test data is realistic and representative
- Check data format matches expected schema
- Ensure test data covers edge cases and boundary conditions
Files:
integ-test/src/test/resources/expectedOutput/calcite/explain_spath_without_path.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_spath_without_path.yaml
**/ppl/**/*.java
⚙️ CodeRabbit configuration file
**/ppl/**/*.java: - For PPL parser changes, verify grammar tests with positive/negative cases
- Check AST generation for new syntax
- Ensure corresponding AST builder classes are updated
- Validate edge cases and boundary conditions
Files:
integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.javappl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorCoverageTest.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.java
integ-test/**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
End-to-end scenarios need integration tests in
integ-test/module
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/standalone/MapAppendFunctionIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.java
⚙️ CodeRabbit configuration file
integ-test/**/*IT.java: - Integration tests MUST use valid test data from resources
- Verify test data files exist in integ-test/src/test/resources/
- Check test assertions are meaningful and specific
- Validate tests clean up resources after execution
- Ensure tests are independent and can run in any order
- Flag tests that reference non-existent indices (e.g., EMP)
- Verify integration tests are in correct module (integ-test/)
- Check tests can be run with ./gradlew :integ-test:integTest
- Ensure proper test data setup and teardown
- Validate end-to-end scenario coverage
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/standalone/MapAppendFunctionIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.java
**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
Name integration tests with
*IT.javasuffix in OpenSearch SQL
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/standalone/MapAppendFunctionIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.java
**/calcite/**/*.java
⚙️ CodeRabbit configuration file
**/calcite/**/*.java: - Follow existing Calcite integration patterns
- Verify RelNode visitor implementations are complete
- Check RexNode handling follows project conventions
- Validate SQL generation is correct and optimized
- Ensure Calcite version compatibility
- Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor
- Document any Calcite-specific workarounds
- Test compatibility with Calcite version constraints
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javainteg-test/src/test/java/org/opensearch/sql/calcite/standalone/MapAppendFunctionIT.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.java
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java
⚙️ CodeRabbit configuration file
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java: - Flag methods >50 lines - this file is known to be hard to read
- Suggest extracting complex logic into helper methods
- Check for code organization and logical grouping
- Validate all RelNode types are handled
Files:
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java
core/src/main/java/org/opensearch/sql/ast/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/org/opensearch/sql/ast/**/*.java: - AST nodes must be immutable where possible
- Follow visitor pattern for AST traversal
- Ensure proper toString() implementation for debugging
Files:
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionContext.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionResult.javacore/src/main/java/org/opensearch/sql/ast/AstNodeUtils.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java
🧠 Learnings (9)
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration
Applied to files:
docs/dev/ppl-commands.mdcore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorCoverageTest.javacore/src/main/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImpl.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.javainteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
docs/dev/ppl-commands.mdinteg-test/src/test/resources/expectedOutput/calcite/explain_spath_without_path.yamlinteg-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.javainteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_spath_without_path.yamlcore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javainteg-test/src/test/java/org/opensearch/sql/calcite/standalone/MapAppendFunctionIT.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorCoverageTest.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javainteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.java
📚 Learning: 2025-12-11T05:27:39.856Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 0
File: :0-0
Timestamp: 2025-12-11T05:27:39.856Z
Learning: In opensearch-project/sql, for SEMI and ANTI join types in CalciteRelNodeVisitor.java, the `max` option has no effect because these join types only use the left side to filter records based on the existence of matches in the right side. The join results are identical regardless of max value (max=1, max=2, or max=∞). The early return for SEMI/ANTI joins before processing the `max` option is intentional and correct behavior.
Applied to files:
integ-test/src/test/resources/expectedOutput/calcite/explain_spath_without_path.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_spath_without_path.yamlinteg-test/src/test/java/org/opensearch/sql/calcite/standalone/MapAppendFunctionIT.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*.java : Document Calcite-specific workarounds in code
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javainteg-test/src/test/java/org/opensearch/sql/calcite/standalone/MapAppendFunctionIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.java
📚 Learning: 2025-12-29T05:32:03.491Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:03.491Z
Learning: For any custom Calcite RelNode class (e.g., ones that extend EnumerableLimitSort or other Calcite RelNode types), always override the copy method. If copy is not overridden, cloning/copy operations may downgrade the instance to the parent class type, losing the custom behavior. In your implementation, ensure copy returns a new instance of the concrete class with all relevant fields and traits preserved, mirroring the current instance state.
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*Test.java : Name unit tests with `*Test.java` suffix in OpenSearch SQL
Applied to files:
core/src/test/java/org/opensearch/sql/ast/analysis/FieldResolutionResultTest.javacore/src/test/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImplTest.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Update corresponding AST builder classes when making PPL grammar changes
Applied to files:
ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java
📚 Learning: 2025-12-29T05:32:11.893Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:11.893Z
Learning: In opensearch-project/sql, when creating custom Calcite RelNode classes that extend EnumerableLimitSort or other Calcite RelNode types, always override the `copy` method. Without overriding copy, the class will downgrade to its parent class type during copy operations, losing the custom implementation.
Applied to files:
ppl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorCoverageTest.java
🧬 Code graph analysis (11)
core/src/test/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImplTest.java (1)
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImpl.java (1)
AppendFunctionImpl(27-65)
core/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.java (1)
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImpl.java (1)
AppendFunctionImpl(27-65)
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java (2)
core/src/main/java/org/opensearch/sql/ast/EmptySourcePropagateVisitor.java (1)
EmptySourcePropagateVisitor(20-121)core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java (1)
PPLFuncImpTable(299-1539)
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionResult.java (1)
core/src/main/java/org/opensearch/sql/calcite/utils/WildcardUtils.java (1)
WildcardUtils(13-233)
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImpl.java (1)
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendCore.java (1)
AppendCore(14-59)
core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java (1)
core/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.java (1)
PPLBuiltinOperators(111-514)
core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java (1)
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java (1)
FieldResolutionVisitor(73-613)
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImpl.java (2)
core/src/main/java/org/opensearch/sql/expression/function/ImplementorUDF.java (1)
ImplementorUDF(17-40)core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendCore.java (1)
AppendCore(14-59)
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendFunctionImpl.java (1)
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendCore.java (1)
AppendCore(14-59)
core/src/main/java/org/opensearch/sql/analysis/Analyzer.java (1)
common/src/main/java/org/opensearch/sql/common/patterns/PatternUtils.java (1)
PatternUtils(15-170)
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java (2)
core/src/main/java/org/opensearch/sql/calcite/utils/WildcardUtils.java (1)
WildcardUtils(13-233)common/src/main/java/org/opensearch/sql/common/patterns/PatternUtils.java (1)
PatternUtils(15-170)
🪛 Biome (2.1.2)
doctest/test_data/structured.json
[error] 1-2: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 2-3: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (26)
- GitHub Check: security-it-linux (25)
- GitHub Check: security-it-linux (21)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (25, integration)
- GitHub Check: build-linux (25, doc)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (21, unit)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: test-sql-cli-integration (21)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImpl.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImpl.java
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.java
Outdated
Show resolved
Hide resolved
...test/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImplTest.java
Outdated
Show resolved
Hide resolved
integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java
Outdated
Show resolved
Hide resolved
ppl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@ppl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.java:
- Around line 25-345: Add null-input validation tests and a defensive null check
for FieldResolutionVisitor.analyze(): update FieldResolutionVisitorTest to
include tests that call visitor.analyze(null) and assert a clear
IllegalArgumentException (or NPE) with a meaningful message, and modify
FieldResolutionVisitor.analyze(UnresolvedPlan) to check for null at the top and
throw that same exception/message; reference the analyze method in
FieldResolutionVisitor and the test class FieldResolutionVisitorTest so tests
and implementation are consistent.
🧹 Nitpick comments (3)
core/src/test/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImplTest.java (2)
86-96: Consider renaming for clarity.The test method
testEmptyStringactually covers various null and invalid input scenarios beyond just empty strings (null, whitespace, empty objects, primitives). Consider a more descriptive name liketestNullAndInvalidInputsor splitting into separate test methods for better clarity.♻️ Suggested rename
@Test - public void testEmptyString() { + public void testNullAndInvalidInputs() { assertEvalNull(); assertEvalNull((String) null); assertEvalNull(""); assertEvalNull("", ""); assertEvalNull(" "); assertEvalNull("{}"); assertEvalNull("\"just a string\""); assertEvalNull("123"); }
366-371: Consider adding edge case for arrays with empty strings.The implementation's
convertArrayToStringfilters null elements but not empty strings. Consider adding a test case forArrays.asList("", "")orArrays.asList("{\"x\":1}", "", "{\"y\":2}")to verify the expected behavior when empty strings are present in the array.🧪 Suggested additional test
@Test public void testNullAndEmptyArray() { assertEvalNull(Arrays.asList(null, null, null)); assertEvalNull(Arrays.asList()); assertEvalNull((List<?>) null); + assertEvalNull(Arrays.asList("", "")); + assertEvalNull(Arrays.asList("", "", "")); }ppl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.java (1)
239-263: Consider simplifying the self-join assertion logic.The conditional assertion logic (lines 256-261) correctly validates the two Relation instances in a self-join scenario, but the branching based on field content could be clearer. Consider extracting this into a helper method or adding comments explaining why one instance has an empty wildcard (affected by
fieldscommand) while the other has "*" (not affected).♻️ Optional refactor to improve clarity
Add a comment explaining the expected behavior:
assertEquals("logs", tableName); Set<String> fields = result.getRegularFields(); assertEquals(1, fields.size()); + // Self-join creates two Relation instances for the same table: + // - Left side (with fields command): {id} with empty wildcard + // - Right side (without fields command): {parent_id} with "*" wildcard if (fields.contains("id")) { assertEquals("", result.getWildcard().toString()); } else { assertTrue(fields.contains("parent_id")); assertEquals("*", result.getWildcard().toString()); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/src/main/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImpl.javacore/src/test/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImplTest.javainteg-test/src/test/java/org/opensearch/sql/calcite/standalone/JsonExtractAllFunctionIT.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImpl.java
🧰 Additional context used
📓 Path-based instructions (7)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/standalone/JsonExtractAllFunctionIT.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.javacore/src/test/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImplTest.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/standalone/JsonExtractAllFunctionIT.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.javacore/src/test/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImplTest.java
integ-test/**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
End-to-end scenarios need integration tests in
integ-test/module
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/standalone/JsonExtractAllFunctionIT.java
⚙️ CodeRabbit configuration file
integ-test/**/*IT.java: - Integration tests MUST use valid test data from resources
- Verify test data files exist in integ-test/src/test/resources/
- Check test assertions are meaningful and specific
- Validate tests clean up resources after execution
- Ensure tests are independent and can run in any order
- Flag tests that reference non-existent indices (e.g., EMP)
- Verify integration tests are in correct module (integ-test/)
- Check tests can be run with ./gradlew :integ-test:integTest
- Ensure proper test data setup and teardown
- Validate end-to-end scenario coverage
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/standalone/JsonExtractAllFunctionIT.java
**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
Name integration tests with
*IT.javasuffix in OpenSearch SQL
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/standalone/JsonExtractAllFunctionIT.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/standalone/JsonExtractAllFunctionIT.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.javacore/src/test/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImplTest.java
**/calcite/**/*.java
⚙️ CodeRabbit configuration file
**/calcite/**/*.java: - Follow existing Calcite integration patterns
- Verify RelNode visitor implementations are complete
- Check RexNode handling follows project conventions
- Validate SQL generation is correct and optimized
- Ensure Calcite version compatibility
- Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor
- Document any Calcite-specific workarounds
- Test compatibility with Calcite version constraints
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/standalone/JsonExtractAllFunctionIT.java
**/*Test.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*Test.java: All new business logic requires unit tests
Name unit tests with*Test.javasuffix in OpenSearch SQL
Files:
ppl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.javacore/src/test/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImplTest.java
**/ppl/**/*.java
⚙️ CodeRabbit configuration file
**/ppl/**/*.java: - For PPL parser changes, verify grammar tests with positive/negative cases
- Check AST generation for new syntax
- Ensure corresponding AST builder classes are updated
- Validate edge cases and boundary conditions
Files:
ppl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.java
🧠 Learnings (5)
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/standalone/JsonExtractAllFunctionIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/standalone/JsonExtractAllFunctionIT.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*Test.java : Name unit tests with `*Test.java` suffix in OpenSearch SQL
Applied to files:
ppl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.javacore/src/test/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImplTest.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration
Applied to files:
ppl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.java
📚 Learning: 2025-12-11T05:27:39.856Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 0
File: :0-0
Timestamp: 2025-12-11T05:27:39.856Z
Learning: In opensearch-project/sql, for SEMI and ANTI join types in CalciteRelNodeVisitor.java, the `max` option has no effect because these join types only use the left side to filter records based on the existence of matches in the right side. The join results are identical regardless of max value (max=1, max=2, or max=∞). The early return for SEMI/ANTI joins before processing the `max` option is intentional and correct behavior.
Applied to files:
ppl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.java
🧬 Code graph analysis (1)
core/src/test/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImplTest.java (1)
core/src/main/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImpl.java (1)
JsonExtractAllFunctionImpl(43-242)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: build-linux (25, doc)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (21, unit)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (25, integration)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: security-it-linux (21)
- GitHub Check: security-it-linux (25)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: test-sql-cli-integration (21)
- GitHub Check: CodeQL-Scan (java)
🔇 Additional comments (10)
integ-test/src/test/java/org/opensearch/sql/calcite/standalone/JsonExtractAllFunctionIT.java (1)
226-251: LGTM! Behavior change correctly tested.The test now validates that
JSON_EXTRACT_ALLreturns null for an empty JSON object{}, which aligns with the implementation'sconvertEmptyMapToNulllogic. This is a sensible semantic change that treats empty results consistently.core/src/test/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImplTest.java (2)
75-77: Good refactoring to reduce duplication.The helper method consolidates null-assertion logic across multiple test cases, improving maintainability and readability.
344-353: Verify if fragmented JSON input is a realistic scenario.The test
testArrayInputWithMultipleJsonFragmentsvalidates that JSON fragments in separate array elements (["{\"name\": \"John\"", ", \"age\": 30}"]) are concatenated and parsed. While this works due to the implementation joining array elements, it's unclear whether this reflects intended real-world usage or is just testing an implementation detail.If this is intentional behavior that users might rely on, consider:
- Adding a comment explaining the use case
- Documenting this capability in user-facing documentation
Otherwise, you may want to reconsider whether supporting this pattern is desirable, as it could lead to unexpected behavior.
ppl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.java (7)
25-75: LGTM! Well-structured test setup and helper methods.The setup properly initializes dependencies with mocked Settings, and the helper methods effectively reduce duplication across test cases. The assertion helpers appropriately handle both single-relation and multi-relation (join) scenarios.
77-194: LGTM! Comprehensive single-relation test coverage.The test cases systematically cover PPL commands (where, fields, stats, sort, eval) and their combinations, including edge cases like nested fields, wildcard patterns, and function calls in filters. Test naming follows conventions and assertions are clear.
196-237: LGTM! Thorough multi-relation and join test coverage.The tests properly validate field resolution across multiple relations, including basic joins and combinations with filters and projections. The use of
assertJoinRelationFieldshelper keeps the tests clean and readable.
265-281: LGTM! Join with aggregation and subsearch tests validate advanced scenarios.These tests confirm that field resolution works correctly when joins are combined with aggregations and subsearch syntax.
283-293: LGTM! Error case properly validated.The test confirms that subqueries in filter clauses throw an appropriate exception with a clear error message.
338-344: LGTM! Unimplemented command detection works correctly.The test validates that attempting to use an unsupported command (kmeans) throws an exception, preventing silent failures.
295-336: LGTM! Comprehensive coverage of additional PPL commands.The tests validate field resolution for regex/rex/patterns, dedup, reverse, head, and rename commands. Tests correctly expect wildcard-only results for commands that don't restrict fields (reverse, head, rename).
ppl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java:
- Around line 78-82: Add JavaDoc to the public method analyze in class
FieldResolutionVisitor: document the parameter plan with @param, describe the
returned Map with @return, and list any runtime exceptions that can be thrown
(e.g., from acceptAndVerifyNodeVisited) using @throws; mention what the
FieldResolutionContext does and that the method returns context.getResults()
after visiting the plan. Ensure the JavaDoc is concise, follows project style,
and references UnresolvedPlan and FieldResolutionResult types.
🧹 Nitpick comments (2)
ppl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.java (1)
250-273: Consider extracting assertion logic for better readability.The self-join test manually iterates and uses conditional logic to validate results. While correct, this approach is less clear than the pattern used in other join tests.
♻️ Optional refactor to improve readability
Consider adding a specialized helper method for self-join assertions:
private void assertSelfJoinFields( String query, Set<String> leftFields, String leftWildcard, Set<String> rightFields, String rightWildcard) { UnresolvedPlan plan = parse(query); Map<UnresolvedPlan, FieldResolutionResult> results = visitor.analyze(plan); assertEquals(2, results.size()); List<FieldResolutionResult> resultList = new ArrayList<>(results.values()); // Verify first relation assertEquals(leftFields, resultList.get(0).getRegularFields()); assertEquals(leftWildcard, resultList.get(0).getWildcard().toString()); // Verify second relation assertEquals(rightFields, resultList.get(1).getRegularFields()); assertEquals(rightWildcard, resultList.get(1).getWildcard().toString()); }Then simplify the test:
@Test public void testSelfJoin() { // Note: The order of results may vary, so this approach assumes stable ordering // or needs additional logic to identify which is which assertSelfJoinFields( "source=logs | fields id | join left=l right=r ON l.id = r.parent_id logs", Set.of("id"), "", Set.of("parent_id"), "*"); }However, the current implementation is acceptable if result ordering is non-deterministic.
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java (1)
1-617: Class size exceeds guideline but structure is sound.The class is 617 lines, exceeding the 500-line guideline for maintainability review. However, the structure is well-organized:
- Clear separation between visit methods for different AST node types
- Small, focused helper methods
- Consistent patterns throughout
- The Visitor pattern requires all visit methods to be in one class
Potential refactoring options (if needed in the future):
- Extract join-related logic (lines 255-318) to a JoinFieldResolver helper
- Extract field extraction utilities (lines 225-253, 601-616) to a FieldExtractionUtils class
For now, the current implementation is acceptable given the architectural constraints of the Visitor pattern.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.javacore/src/test/java/org/opensearch/sql/ast/AstNodeUtilsTest.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.java
🧰 Additional context used
📓 Path-based instructions (6)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
core/src/test/java/org/opensearch/sql/ast/AstNodeUtilsTest.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
core/src/test/java/org/opensearch/sql/ast/AstNodeUtilsTest.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java
**/*Test.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*Test.java: All new business logic requires unit tests
Name unit tests with*Test.javasuffix in OpenSearch SQL
Files:
core/src/test/java/org/opensearch/sql/ast/AstNodeUtilsTest.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
core/src/test/java/org/opensearch/sql/ast/AstNodeUtilsTest.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.java
**/ppl/**/*.java
⚙️ CodeRabbit configuration file
**/ppl/**/*.java: - For PPL parser changes, verify grammar tests with positive/negative cases
- Check AST generation for new syntax
- Ensure corresponding AST builder classes are updated
- Validate edge cases and boundary conditions
Files:
ppl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.java
core/src/main/java/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/**/*.java: - New functions MUST have unit tests in the same commit
Files:
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java
core/src/main/java/org/opensearch/sql/ast/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/org/opensearch/sql/ast/**/*.java: - AST nodes must be immutable where possible
- Follow visitor pattern for AST traversal
- Ensure proper toString() implementation for debugging
Files:
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
core/src/test/java/org/opensearch/sql/ast/AstNodeUtilsTest.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*Test.java : Name unit tests with `*Test.java` suffix in OpenSearch SQL
Applied to files:
core/src/test/java/org/opensearch/sql/ast/AstNodeUtilsTest.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL
Applied to files:
ppl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration
Applied to files:
ppl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java
📚 Learning: 2025-12-11T05:27:39.856Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 0
File: :0-0
Timestamp: 2025-12-11T05:27:39.856Z
Learning: In opensearch-project/sql, for SEMI and ANTI join types in CalciteRelNodeVisitor.java, the `max` option has no effect because these join types only use the left side to filter records based on the existence of matches in the right side. The join results are identical regardless of max value (max=1, max=2, or max=∞). The early return for SEMI/ANTI joins before processing the `max` option is intentional and correct behavior.
Applied to files:
ppl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.java
🧬 Code graph analysis (3)
core/src/test/java/org/opensearch/sql/ast/AstNodeUtilsTest.java (1)
legacy/src/main/java/org/opensearch/sql/legacy/domain/Field.java (1)
Field(17-147)
ppl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.java (2)
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java (1)
FieldResolutionVisitor(76-617)common/src/main/java/org/opensearch/sql/common/setting/Settings.java (1)
Settings(17-105)
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java (3)
core/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.java (1)
AbstractNodeVisitor(94-464)core/src/main/java/org/opensearch/sql/calcite/utils/WildcardUtils.java (1)
WildcardUtils(13-233)core/src/main/java/org/opensearch/sql/expression/parse/RegexCommonUtils.java (1)
RegexCommonUtils(20-143)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (25, doc)
- GitHub Check: build-linux (25, integration)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (21, unit)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: build-linux (21, doc)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: security-it-linux (21)
- GitHub Check: security-it-linux (25)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: test-sql-cli-integration (21)
- GitHub Check: WhiteSource Security Check
🔇 Additional comments (14)
core/src/test/java/org/opensearch/sql/ast/AstNodeUtilsTest.java (4)
1-48: Well-structured test class with proper setup.The test class follows best practices:
- Proper JUnit 5 and Mockito integration
- Clear separation of mock and concrete test objects
- Fresh subquery instances initialized before each test
- Good use of
@BeforeEachto ensure test independence
50-68: Excellent coverage of null and direct subquery cases.These tests properly validate:
- Null-safety (returns false for null input)
- All three concrete subquery types (Scalar, Exists, In)
The test assertions are clear and appropriate.
70-89: Good coverage of Let expressions and simple expressions.These tests validate:
- Let expressions containing subqueries are detected
- Let expressions without subqueries return false
- Simple expressions (Literal) return false
The use of
new Literal(42, null)for test data is acceptable, where null represents an unspecified DataType for testing purposes.
91-163: Comprehensive coverage of node hierarchies and nested structures.Excellent test coverage validating:
- Single and multiple children scenarios
- Empty children edge case
- Various nesting depths (2-level, 3-level)
- Recursive detection through nested structures
- Complex Let nesting patterns
The use of Mockito's
doReturn().when()pattern is appropriate for simulating node hierarchies. These tests thoroughly validate the recursive subquery detection logic.ppl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.java (3)
31-75: LGTM! Well-structured test helpers.The setup and helper methods are well-designed and reduce code duplication across test methods. The mocked Settings configuration is appropriate for unit testing.
77-354: Comprehensive test coverage for field resolution.The test suite covers a wide range of PPL commands and scenarios including positive cases, error conditions, and edge cases like wildcards and self-joins. The test methods are well-organized and clearly named.
294-303: Good error condition coverage.The error tests validate that unsupported scenarios (subquery in filter, unimplemented commands) are properly detected and throw meaningful exceptions.
Also applies to: 349-354
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java (7)
92-102: Excellent defensive programming pattern.The
acceptAndVerifyNodeVisitedmethod ensures that all AST node types are explicitly handled by this visitor, preventing silent failures when new node types are added.
104-133: Clean implementation of project field resolution.The method appropriately handles both wildcard projections and explicit field lists, correctly merging requirements with the current context.
135-148: Well-implemented constraint enforcement.Both
visitFilterandvisitSpathappropriately enforce field resolution constraints with clear error messages when unsupported scenarios are encountered.Also applies to: 169-188
225-253: Solid recursive field extraction logic.The method appropriately handles various expression types using pattern matching and recursive traversal with proper null handling.
255-318: Well-architected join field resolution.The join visitor and its helper methods are cleanly designed with appropriate separation of concerns. The use of functional interfaces (UnaryOperator, Predicate) makes the field filtering and mapping logic clear and reusable.
320-599: Consistent and appropriate visitor implementations.All visitor methods follow clear patterns:
- Commands that don't affect field requirements use passthrough
- Commands that need fields extract them and merge with current requirements
- Aggregation-like commands (chart, rareTopN) correctly reset requirements
- Complex commands (patterns, streamWindow) appropriately handle their specific field semantics
601-616: Clean aggregation field extraction.The method appropriately handles both aliased and direct aggregate function expressions, recursively extracting field references.
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java:
- Around line 224-231: The method resolveFields currently returns
FieldResolutionResult but may return null when result.get(target) misses; change
its signature to return Optional<FieldResolutionResult>, and after running
FieldResolutionVisitor.analyze(rootNode) return
Optional.ofNullable(result.get(target)) instead of raw get; keep the existing
rootNode null check/IllegalStateException, and update all callers of
CalcitePlanContext.resolveFields to handle Optional (or, if you prefer throwing,
replace the final return with a clear IllegalStateException when
result.get(target) is null); refer to resolveFields,
FieldResolutionVisitor.analyze, rootNode and the local variable result when
making the edits.
🧹 Nitpick comments (1)
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java (1)
76-76: Consider refactoring large visitor class for maintainability.The file is 624 lines, exceeding the 500-line maintainability guideline. While the visitor pattern naturally requires many methods (one per node type), consider extracting helper utilities or grouping related visits into smaller focused visitors to improve readability and maintenance.
Potential refactoring approaches:
- Extract field extraction logic (
extractFieldsFromExpression,extractFieldsFromAggregation) into a separate utility class- Group related visit methods (e.g., all transform commands, all aggregation commands) into focused sub-visitors
- Create a base class for common patterns like
pushRequirements -> visitChildren -> popRequirements
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javacore/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.java
🧰 Additional context used
📓 Path-based instructions (5)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
core/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
core/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java
core/src/main/java/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/**/*.java: - New functions MUST have unit tests in the same commit
Files:
core/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java
core/src/main/java/org/opensearch/sql/expression/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/org/opensearch/sql/expression/**/*.java: - New expression implementations must follow existing patterns
- Type handling must be consistent with project type system
- Error handling must use appropriate exception types
- Null handling must be explicit and documented
Files:
core/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.java
core/src/main/java/org/opensearch/sql/ast/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/org/opensearch/sql/ast/**/*.java: - AST nodes must be immutable where possible
- Follow visitor pattern for AST traversal
- Ensure proper toString() implementation for debugging
Files:
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java
**/calcite/**/*.java
⚙️ CodeRabbit configuration file
**/calcite/**/*.java: - Follow existing Calcite integration patterns
- Verify RelNode visitor implementations are complete
- Check RexNode handling follows project conventions
- Validate SQL generation is correct and optimized
- Ensure Calcite version compatibility
- Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor
- Document any Calcite-specific workarounds
- Test compatibility with Calcite version constraints
Files:
core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*.java : Use `camelCase` for method and variable names (e.g., `executeQuery`)
Applied to files:
core/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*.java : Use `PascalCase` for class names (e.g., `QueryExecutor`)
Applied to files:
core/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration
Applied to files:
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Flag missing JavaDoc on public APIs during code review
Applied to files:
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java
📚 Learning: 2025-12-29T05:32:03.491Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:03.491Z
Learning: For any custom Calcite RelNode class (e.g., ones that extend EnumerableLimitSort or other Calcite RelNode types), always override the copy method. If copy is not overridden, cloning/copy operations may downgrade the instance to the parent class type, losing the custom behavior. In your implementation, ensure copy returns a new instance of the concrete class with all relevant fields and traits preserved, mirroring the current instance state.
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java
🧬 Code graph analysis (2)
core/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.java (1)
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImpl.java (1)
AppendFunctionImpl(27-65)
core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java (1)
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java (1)
FieldResolutionVisitor(76-624)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: security-it-linux (25)
- GitHub Check: security-it-linux (21)
- GitHub Check: build-linux (25, doc)
- GitHub Check: build-linux (21, unit)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (25, integration)
- GitHub Check: build-linux (21, integration)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: test-sql-cli-integration (21)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: CodeQL-Scan (java)
🔇 Additional comments (3)
core/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.java (3)
45-45: LGTM!The import is correctly placed and necessary for the new
INTERNAL_APPENDoperator.
396-396: No issues identified. The code correctly follows the established pattern for collection functions, where the Java constant usesUPPER_SNAKE_CASE(e.g.,MAP_REMOVE) and the query-invocable function name uses lowercase (e.g.,map_remove). This is consistent with other collection operators (MAP_APPEND,MVAPPEND,MVZIP,MVFIND). TheFunctionName.of()method normalizes all function names to lowercase, so no breaking change exists. Tests correctly referenceBuiltinFunctionName.MAP_REMOVE.
398-398: No action needed. The operator is correctly designed as internal-only.
INTERNAL_APPENDis intentionally marked as hidden (via thetrueparameter inBuiltinFunctionName) and used internally byCalciteRelNodeVisitorfor automatic rewriting. The naming follows project conventions for internal functions, and the behavior—returning scalars for single elements versusmvappendwhich always returns arrays—is correct and well-tested.Likely an incorrect or invalid review comment.
| * @throws IllegalStateException if root node not set via {@link #setRootNode} | ||
| */ | ||
| public FieldResolutionResult resolveFields(UnresolvedPlan target) { | ||
| if (rootNode == null) { |
There was a problem hiding this comment.
error message will return to user? it is not actionable.
There was a problem hiding this comment.
It would be 5xx error as it throws IllegalStateException.
There was a problem hiding this comment.
5xx error indicates service issue, and we shouldn't need to tag it as [BUG]. We already have bunch of codes to throw IllegalStateException with Failed to xxxx message.
| * @return field resolution result with regular fields and wildcard patterns | ||
| * @throws IllegalStateException if root node not set via {@link #setRootNode} | ||
| */ | ||
| public FieldResolutionResult resolveFields(UnresolvedPlan target) { |
There was a problem hiding this comment.
This function not use CalcitePlanContext info. Could fit into analyzer module.
There was a problem hiding this comment.
This function refers rootNode in CalcitePlanContext, and that's why I put it here.
| } | ||
|
|
||
| @Override | ||
| public Node visitProject(Project node, FieldResolutionContext context) { |
There was a problem hiding this comment.
Consider extracting per‑command “required field” logic to reducing duplication when new commands are added.
There was a problem hiding this comment.
Per‑command “required field” logic depends on the command logic, which specific part do you expect to be extracted?
There was a problem hiding this comment.
When new AST node introduced, it need to add field resolve logic FieldResolutionVisitor.
One workaround is and define a interface on UnresolvedPlan to resolve field, then each UnresolvedPlan need to implement it.
There was a problem hiding this comment.
I wanted to avoid that as answered at #5028 (comment)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java:
- Line 34: The JSON passed to sj in the putItem call has an invalid trailing
comma; update the third argument in the putItem(7, "overwrap", sj(...))
invocation to remove the extra comma so the string becomes well-formed (e.g.,
change "{'a.b': 1, 'a': {'b': 2, 'c': 3}, }" to "{'a.b': 1, 'a': {'b': 2, 'c':
3}}").
🧹 Nitpick comments (1)
core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java (1)
224-234: Consider caching field resolution results.Each call to
resolveFieldscreates a newFieldResolutionVisitorand re-analyzes the entire AST fromrootNode, but only returns one entry from the result map. IfresolveFieldsis called multiple times during query planning (e.g., for multiplespathcommands), this could be inefficient.Consider caching the analysis result:
♻️ Optional: Cache field resolution results
+ /** Cached field resolution results. Invalidated when rootNode changes. */ + private Map<UnresolvedPlan, FieldResolutionResult> fieldResolutionCache; + + public void setRootNode(UnresolvedPlan rootNode) { + this.rootNode = rootNode; + this.fieldResolutionCache = null; // Invalidate cache + } + public FieldResolutionResult resolveFields(UnresolvedPlan target) { if (rootNode == null) { throw new IllegalStateException("Failed to resolve fields. Root node is not set."); } - FieldResolutionVisitor visitor = new FieldResolutionVisitor(); - Map<UnresolvedPlan, FieldResolutionResult> result = visitor.analyze(rootNode); + if (fieldResolutionCache == null) { + FieldResolutionVisitor visitor = new FieldResolutionVisitor(); + fieldResolutionCache = visitor.analyze(rootNode); + } + Map<UnresolvedPlan, FieldResolutionResult> result = fieldResolutionCache; if (!result.containsKey(target)) { throw new IllegalStateException("Failed to resolve fields for node: " + target.toString()); } return result.get(target); }Note: This would require removing the Lombok
@Setterand implementing the setter manually.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javadocs/user/ppl/cmd/spath.mddoctest/test_data/structured.jsoninteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/user/ppl/cmd/spath.md
🧰 Additional context used
📓 Path-based instructions (6)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
core/src/main/java/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/**/*.java: - New functions MUST have unit tests in the same commit
Files:
core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java
**/calcite/**/*.java
⚙️ CodeRabbit configuration file
**/calcite/**/*.java: - Follow existing Calcite integration patterns
- Verify RelNode visitor implementations are complete
- Check RexNode handling follows project conventions
- Validate SQL generation is correct and optimized
- Ensure Calcite version compatibility
- Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor
- Document any Calcite-specific workarounds
- Test compatibility with Calcite version constraints
Files:
core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
integ-test/**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
End-to-end scenarios need integration tests in
integ-test/module
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
⚙️ CodeRabbit configuration file
integ-test/**/*IT.java: - Integration tests MUST use valid test data from resources
- Verify test data files exist in integ-test/src/test/resources/
- Check test assertions are meaningful and specific
- Validate tests clean up resources after execution
- Ensure tests are independent and can run in any order
- Flag tests that reference non-existent indices (e.g., EMP)
- Verify integration tests are in correct module (integ-test/)
- Check tests can be run with ./gradlew :integ-test:integTest
- Ensure proper test data setup and teardown
- Validate end-to-end scenario coverage
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
Name integration tests with
*IT.javasuffix in OpenSearch SQL
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*.java : Prefer `Optional<T>` for nullable returns in Java
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java
📚 Learning: 2025-12-29T05:32:03.491Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:03.491Z
Learning: For any custom Calcite RelNode class (e.g., ones that extend EnumerableLimitSort or other Calcite RelNode types), always override the copy method. If copy is not overridden, cloning/copy operations may downgrade the instance to the parent class type, losing the custom behavior. In your implementation, ensure copy returns a new instance of the concrete class with all relevant fields and traits preserved, mirroring the current instance state.
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*.java : Document Calcite-specific workarounds in code
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*Test.java : Name unit tests with `*Test.java` suffix in OpenSearch SQL
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to integ-test/**/*IT.java : End-to-end scenarios need integration tests in `integ-test/` module
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
🧬 Code graph analysis (1)
core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java (1)
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java (1)
FieldResolutionVisitor(76-624)
🪛 Biome (2.1.2)
doctest/test_data/structured.json
[error] 1-2: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 2-3: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
🔇 Additional comments (9)
doctest/test_data/structured.json (1)
1-3: LGTM — test data additions for spath field resolution.The new
doc_multianddoc_dottedfields appropriately cover multi-field extraction and dotted-key vs nested-path scenarios needed for spath testing.The Biome static analysis errors are false positives — this file uses NDJSON (newline-delimited JSON) format where each line is a standalone JSON object, which is standard for test data and bulk ingestion scenarios.
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java (6)
20-22: LGTM!The class follows the
*IT.javanaming convention for integration tests and is correctly located in theinteg-test/module. TheINDEXconstant is properly defined usingUPPER_SNAKE_CASE.
37-53: LGTM!The helper methods are well-organized with clear single responsibilities. The
sj()method provides a convenient way to write JSON strings without escaping quotes, and the separation of concerns betweenputItem,docWithJson, andescapeimproves readability.
55-90: LGTM!Good coverage of both happy path (
testSimpleSpath) and error conditions. The error message constants are descriptive, and error tests properly verify that the expected exceptions are raised for unsupported operations (no fields command, wildcard usage, subquery filtering).
92-129: LGTM!Excellent edge case coverage including:
- Null handling for absent fields
- Overlapping field name resolution (
a.bas both a key and nested path)- Array accumulation behavior when
spathis invoked twiceThe assertions properly validate both schema types and data values.
131-149: LGTM!Good integration testing of
spathwith other PPL commands (evalandstats). The tests verify that extracted string values can be used in arithmetic expressions and aggregations, confirming proper type coercion behavior.
151-171: LGTM!Comprehensive integration tests covering:
- Nested field extraction with array notation (
nested.d{})- Complex join scenarios using spath-extracted fields from both sides
The join test correctly verifies that only matching keys produce output rows.
core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java (2)
25-28: LGTM!All new imports are necessary and correctly used by the new
rootNodefield andresolveFieldsmethod.
78-79: No action needed:rootNodeintentionality confirmed.The
rootNodefield is correctly left unset in cloned contexts. Lambda contexts (created viaclone()) are exclusively used for analyzing lambda expressions at the Calcite RexNode level (seeCalciteRexNodeVisitor:414,CalciteRexNodeVisitor:423), where field resolution is not required. Field resolution viaresolveFields()operates at the AST level and is only called on the original parent context (seeCalciteRelNodeVisitor:716). The omission is intentional design.
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java:
- Around line 47-49: The escape(String json) method only escapes double quotes;
update it to first escape backslashes and then escape double quotes so embedded
JSON is safe (e.g., perform json.replace("\\", "\\\\").replace("\"", "\\\"")
within the escape method), ensuring backslashes are handled before quotes to
avoid double-escaping issues in CalcitePPLSpathCommandIT.
- Around line 23-35: Add explicit index cleanup to CalcitePPLSpathCommandIT by
implementing an @After teardown method that deletes the test_spath index created
in init(); create a public void tearDown/cleanup() method annotated with @After
that calls the existing test cluster/client index delete routine (e.g.,
deleteIndex("test_spath") or using the cluster admin client) and handle
missing-index cases gracefully (ignore/notFound) and surface errors otherwise so
each test method starts with a clean state.
🧹 Nitpick comments (3)
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java (3)
20-22: Consider adding class-level JavaDoc.Per coding guidelines, public classes should have JavaDoc documentation. A brief description of what this integration test covers (spath command with field resolution) would improve maintainability.
📝 Suggested JavaDoc
+/** + * Integration tests for the PPL spath command with field resolution + * using the Calcite engine. + */ public class CalcitePPLSpathCommandIT extends PPLIntegTestCase {
121-129: Clarify the expected behavior for duplicate spath invocation.The test verifies that invoking spath twice on the same input results in an array type for field
a. This behavior (merging duplicate extractions into arrays) should be documented or have a comment explaining the expected semantics, as it may be non-obvious to maintainers.@Test public void testSpathTwice() throws IOException { + // When spath is called twice on the same input, extracted fields are appended + // using the internal `append` function, resulting in array values. JSONObject result =
92-100: Consider adding boundary and NULL input tests.Per coding guidelines, tests should include NULL input tests and boundary conditions. Consider adding tests for:
- NULL value in the
docfield- Empty JSON object
{}- Malformed JSON handling (if applicable)
@Test public void testSpathWithNullInput() throws IOException { // Add test data with null doc putItem(100, "null_doc", null); // or appropriate null representation // Test and verify behavior } @Test public void testSpathWithEmptyJson() throws IOException { putItem(101, "empty", sj("{}")); JSONObject result = executeQuery( "source=test_spath | where testCase='empty' | spath input=doc | fields a | head 1"); verifyDataRows(result, rows((Object) null)); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
🧰 Additional context used
📓 Path-based instructions (5)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
integ-test/**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
End-to-end scenarios need integration tests in
integ-test/module
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
⚙️ CodeRabbit configuration file
integ-test/**/*IT.java: - Integration tests MUST use valid test data from resources
- Verify test data files exist in integ-test/src/test/resources/
- Check test assertions are meaningful and specific
- Validate tests clean up resources after execution
- Ensure tests are independent and can run in any order
- Flag tests that reference non-existent indices (e.g., EMP)
- Verify integration tests are in correct module (integ-test/)
- Check tests can be run with ./gradlew :integ-test:integTest
- Ensure proper test data setup and teardown
- Validate end-to-end scenario coverage
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
Name integration tests with
*IT.javasuffix in OpenSearch SQL
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
**/calcite/**/*.java
⚙️ CodeRabbit configuration file
**/calcite/**/*.java: - Follow existing Calcite integration patterns
- Verify RelNode visitor implementations are complete
- Check RexNode handling follows project conventions
- Validate SQL generation is correct and optimized
- Ensure Calcite version compatibility
- Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor
- Document any Calcite-specific workarounds
- Test compatibility with Calcite version constraints
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*.java : Document Calcite-specific workarounds in code
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*Test.java : Name unit tests with `*Test.java` suffix in OpenSearch SQL
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
📚 Learning: 2025-12-29T05:32:03.491Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:03.491Z
Learning: For any custom Calcite RelNode class (e.g., ones that extend EnumerableLimitSort or other Calcite RelNode types), always override the copy method. If copy is not overridden, cloning/copy operations may downgrade the instance to the parent class type, losing the custom behavior. In your implementation, ensure copy returns a new instance of the concrete class with all relevant fields and traits preserved, mirroring the current instance state.
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
🧬 Code graph analysis (1)
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java (1)
integ-test/src/test/java/org/opensearch/sql/util/MatcherUtils.java (1)
MatcherUtils(43-506)
🔇 Additional comments (4)
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java (4)
55-63: LGTM!The test properly validates basic spath functionality with explicit path, meaningful assertions on both schema and data rows.
65-90: LGTM!Error path tests properly validate the documented constraints: no arbitrary field extraction without explicit projection, no wildcards in field-resolution mode, and no subquery support. These align with the spath documentation requirements.
131-159: LGTM!These tests provide good coverage for:
- Expression evaluation on extracted fields (implicit type coercion)
- Aggregation support verifying stats can identify fields for resolution
- Nested field extraction with special syntax
161-171: LGTM!The join test validates field resolution in a complex multi-source query scenario, ensuring spath works correctly on both sides of a join operation.
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
Show resolved
Hide resolved
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
Show resolved
Hide resolved
Signed-off-by: Tomoyuki Morita <[email protected]>
Signed-off-by: Tomoyuki Morita <[email protected]>
Signed-off-by: Tomoyuki Morita <[email protected]>
Signed-off-by: Tomoyuki Morita <[email protected]>
Signed-off-by: Tomoyuki Morita <[email protected]>
Signed-off-by: Tomoyuki Morita <[email protected]>
Signed-off-by: Tomoyuki Morita <[email protected]>
Signed-off-by: Tomoyuki Morita <[email protected]>
Signed-off-by: Tomoyuki Morita <[email protected]>
Signed-off-by: Tomoyuki Morita <[email protected]>
ykmr1224
force-pushed
the
spath/field-resolution
branch
from
9244d6d to
162fc0b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
integ-test/src/test/java/org/opensearch/sql/calcite/standalone/MapAppendFunctionIT.java (1)
86-93: Test method has unused parameters causing test failure.The
testMapAppendWithSingleNullmethod declares parameters(RexNode map1, RexNode map2)but JUnit 5 cannot inject these values. This test will fail at runtime with aParameterResolutionException. The parameters appear to be copy-paste artifacts sincemap1andmap2are defined inside the method body.🐛 Proposed fix
@Test - public void testMapAppendWithSingleNull(RexNode map1, RexNode map2) throws Exception { + public void testMapAppendWithSingleNull() throws Exception { RelDataType mapType = createMapType(context.rexBuilder); RexNode nullMap = context.rexBuilder.makeNullLiteral(mapType); RexNode map = createMap("key1", "value1"); testWithSingleNull(map, nullMap); testWithSingleNull(nullMap, map); }
🤖 Fix all issues with AI agents
In @common/src/main/java/org/opensearch/sql/common/utils/DebugUtils.java:
- Around line 15-17: The DebugUtils.print method currently writes to System.out
which bypasses the project logging framework; replace direct System.out usage in
DebugUtils by adding an SLF4J Logger (e.g., private static final Logger LOGGER =
LoggerFactory.getLogger(DebugUtils.class)) and call LOGGER.debug/INFO in the
print method instead of System.out.println, and if this utility is only for
local debugging consider moving DebugUtils to the test source tree or add a
runtime guard/comment to prevent production use.
In
@core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendCore.java:
- Around line 16-19: Update the JavaDoc in class AppendCore: replace the invalid
tag {@ref AppendFunctionImplTest} with a valid {@link AppendFunctionImplTest}
reference so the cross-reference compiles correctly; locate the JavaDoc block
above AppendCore (the comment starting "Collect non-null elements from
`args`...") and change the tag accordingly.
🧹 Nitpick comments (18)
core/src/main/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImpl.java (1)
101-110: Consider using method reference for null filtering.The implementation is correct for concatenating JSON fragments. A minor style improvement would be to use
Objects::nonNullinstead of the lambda.♻️ Optional style improvement
+import java.util.Objects; ... private static String convertArrayToString(List<?> array) { if (array == null || array.isEmpty()) { return null; } return array.stream() - .filter(element -> element != null) + .filter(Objects::nonNull) .map(Object::toString) .collect(Collectors.joining()); }core/src/test/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImplTest.java (1)
86-96: Test method name is misleading.The test
testEmptyStringactually covers multiple scenarios: no args, null, empty string, whitespace, empty JSON object, and JSON primitives. Consider renaming totestNullAndInvalidInputsor splitting into focused tests for better maintainability and clearer failure diagnostics.docs/user/ppl/cmd/spath.md (3)
36-43: Fix grammar and improve clarity in limitation notes.Several issues need correction:
- Line 38: "downstream commands requirements" → "downstream command requirements"
- Line 39: "It raises error" → "It raises an error"
- Line 39: Comma placement: "(i.e.
fields, orstatscommand is needed)" → "(i.e.,fieldsorstatscommand is needed)"- Line 42: "Filter with query" is unclear—consider rephrasing to "Filtering using
whereclauses withinorexistsoperators is not supported afterspathcommand"📝 Proposed grammar and clarity fixes
-* Extracts only required fields based on downstream commands requirements (interim solution until full fields extraction is implemented) -* **Limitation**: It raises error if extracted fields cannot be identified by following commands (i.e. `fields`, or `stats` command is needed) +* Extracts only required fields based on downstream command requirements (interim solution until full fields extraction is implemented) +* **Limitation**: It raises an error if extracted fields cannot be identified by following commands (i.e., `fields` or `stats` command is needed) * **Limitation**: Cannot use wildcards (`*`) in field selection - only explicit field names are supported * **Limitation**: All extracted fields are returned as STRING type -* **Limitation**: Filter with query (`where <field> in/exists [...]` ) is not supported after `spath` command +* **Limitation**: Filtering using `where` clauses with `in` or `exists` operators is not supported after `spath` command
142-242: Fix grammar and improve consistency in Examples 5-8.Issues to address:
Line 166: Awkward phrasing. Current: "As
cin the example, extracted value is appended to organize an array if an extracted field already exists."
Suggested: "As shown with fieldcin the example, if an extracted field already exists, the extracted value is appended to form an array."Formatting inconsistency: Examples 5-8 use "Expected output:" while Examples 1-4 use "The query returns the following results:". Consider standardizing to one format throughout the document.
Line 166: "All extracted fields are returned as STRING type" repeats line 41. Consider removing this repetition or referring back to the limitations section.
📝 Proposed fixes
For line 166:
-This extracts only fields `a`, `b`, and `c` from the JSON in `doc_multi` field, even though the JSON contains fields `d` as well. All extracted fields are returned as STRING type. As `c` in the example, extracted value is appended to organize an array if an extracted field already exists. +This extracts only fields `a`, `b`, and `c` from the JSON in `doc_multi` field, even though the JSON contains field `d` as well. As shown with field `c` in the example, if an extracted field already exists, the extracted value is appended to form an array.For consistency, either update Examples 1-4 or Examples 5-8 to use the same output header format.
243-258: Fix grammar and consider showing actual error output.Issues:
Line 245: "It raises error" → "It raises an error" (missing article)
Lines 250, 258: The code blocks include comments like
# ERROR: Requires field selection...and# ERROR: Spath command cannot extract arbitrary fields, but no actual error output is shown. Consider either:
- Adding expected error output blocks (similar to how Examples 1-8 show expected output)
- Or clarifying that these are illustrative comments indicating the queries will fail
📝 Proposed fix
-**Important**: It raises error if extracted fields cannot be identified by following commands +**Important**: It raises an error if extracted fields cannot be identified by following commandsFor the error examples, consider adding error output blocks like:
Error: spath command requires explicit field selection. Add a fields or stats command after spath.common/src/main/java/org/opensearch/sql/common/utils/DebugUtils.java (4)
19-27: Missing JavaDoc on public methods.Per coding guidelines, public methods should have proper JavaDoc documentation. These are the only public API of this class and would benefit from documenting their purpose, parameters, and return values.
📝 Suggested JavaDoc
+ /** + * Logs a debug message with the object's string representation and caller location. + * + * @param obj the object to debug + * @param message a descriptive message to include in the output + * @param <T> the type of the object + * @return the original object, enabling fluent chaining + */ public static <T> T debug(T obj, String message) { print("### %s: %s (at %s)", message, stringify(obj), getCalledFrom(1)); return obj; } + /** + * Logs a debug message with the object's string representation and caller location. + * + * @param obj the object to debug + * @param <T> the type of the object + * @return the original object, enabling fluent chaining + */ public static <T> T debug(T obj) {
29-33: Consider usingThread.currentThread().getStackTrace()instead of creating an exception.Creating a
RuntimeExceptionsolely to obtain the stack trace works but is unconventional. UsingThread.currentThread().getStackTrace()is more idiomatic and avoids instantiating an exception object.♻️ Suggested refactor
private static String getCalledFrom(int pos) { - RuntimeException e = new RuntimeException(); - StackTraceElement item = e.getStackTrace()[pos + 1]; + StackTraceElement item = Thread.currentThread().getStackTrace()[pos + 2]; return item.getClassName() + "." + item.getMethodName() + ":" + item.getLineNumber(); }Note: The offset changes from
pos + 1topos + 2becausegetStackTrace()includes itself in the trace.
65-72: Raw types used ininstanceofchecks.The casts to
CollectionandMapuse raw types, which generate compiler warnings. Use wildcard types for type safety.♻️ Suggested fix
private static String stringify(Object obj) { if (obj instanceof Collection) { - return stringify((Collection) obj); + return stringify((Collection<?>) obj); } else if (obj instanceof Map) { - return stringify((Map) obj); + return stringify((Map<?, ?>) obj); } return String.valueOf(obj); }
35-63: Inconsistent representation for null values between Collection and Map.
stringify(Collection)returns"null"for null input, whilestringify(Map)returns"[[null]]". Similarly, empty representations differ:"()"vs"[[EMPTY]]". This inconsistency could confuse developers reading debug output.Consider using consistent formatting, e.g.,
"[null]"and"[]"for both, or document the intentional difference.core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImpl.java (2)
47-50: Consider documenting why operand metadata is null.Returning
nullfromgetOperandMetadata()implies no operand validation. A brief comment explaining this is intentional (variadic with any types) would improve clarity.📝 Suggested improvement
@Override public UDFOperandMetadata getOperandMetadata() { + // No operand validation - accepts any number of arguments of any type return null; }
62-64: Consider adding JavaDoc for the public static method.Per coding guidelines, public methods should have JavaDoc with
@paramand@return. This method is called via reflection from Calcite.📝 Suggested JavaDoc
+ /** + * Appends all non-null elements from arguments. Called via Calcite reflection. + * + * @param args variable number of arguments to append + * @return null if no elements, single element if one, or List if multiple + */ public static Object append(Object... args) { return AppendCore.collectElements(args); }core/src/test/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImplTest.java (1)
47-53: Consider adding a test for nested lists.The current tests cover single-level list flattening. Consider adding a test to document behavior when a list contains another list (nested arrays), as this edge case may arise in real JSON data.
@Test public void testAppendWithNestedArray() { List<Object> nested = Arrays.asList(Arrays.asList(1, 2), 3); Object result = AppendFunctionImpl.append(nested, 4); // Document expected behavior - does it flatten one level or recursively? // Current implementation flattens one level only assertEquals(Arrays.asList(Arrays.asList(1, 2), 3, 4), result); }integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java (1)
111-119: Consider renaming "overwrap" to "overlap" or "pathConflict"."Overwrap" is not a standard English word. The test demonstrates behavior when a dotted key (
a.b) conflicts with a nested path (a.binsidea). Consider renaming totestOverlaportestPathConflictfor clarity.Suggested rename
- @Test - public void testOverwrap() throws IOException { + @Test + public void testPathOverlap() throws IOException { JSONObject result = executeQuery( - "source=test_spath | where testCase='overwrap' | spath input=doc | fields a.b | head" + "source=test_spath | where testCase='overlap' | spath input=doc | fields a.b | head" + " 1");Also update the test data at line 34:
- putItem(7, "overwrap", sj("{'a.b': 1, 'a': {'b': 2, 'c': 3}}")); + putItem(7, "overlap", sj("{'a.b': 1, 'a': {'b': 2, 'c': 3}}"));ppl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorCoverageTest.java (1)
20-57: Useful coverage validation test.This reflection-based test ensures
FieldResolutionVisitorkeeps parity withCalciteRelNodeVisitor. The approach is pragmatic and will catch missing implementations during CI.Note: The
unsupportedCalciteCommandsset (lines 40-48) requires manual maintenance when new commands are added. Consider adding a comment explaining why each command is excluded to help future maintainers understand the exclusion rationale.📝 Suggested documentation improvement
// Only allow unsupported Calcite commands to be missing Set<String> unsupportedCalciteCommands = Set.of( - "visitAD", - "visitCloseCursor", - "visitFetchCursor", - "visitML", - "visitPaginate", - "visitKmeans", - "visitTableFunction"); + "visitAD", // ML command - not supported in field resolution + "visitCloseCursor", // Cursor management - not applicable + "visitFetchCursor", // Cursor management - not applicable + "visitML", // ML command - not supported in field resolution + "visitPaginate", // Pagination - handled at execution level + "visitKmeans", // ML command - not supported in field resolution + "visitTableFunction"); // Table functions have dynamic schemasdocs/dev/ppl-commands.md (1)
30-30: Consider generalizing the checklist item.The current wording "for
spathcommand support" is specific to this PR. SinceFieldResolutionVisitorneeds to be updated for any new command that participates in field resolution (not just spath), consider making this guidance more general.📝 Suggested wording
- - Override `visit*` in `FieldResolutionVisitor` for `spath` command support. + - Override `visit*` in `FieldResolutionVisitor` if the command affects field resolution (required fields, wildcards).core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java (1)
1-623: Class exceeds 500-line guideline but is cohesive.The class is 623 lines, exceeding the guideline threshold. However, the visitor pattern implementation requires explicit handlers for all node types, making this length justifiable. The pass-through methods (e.g.,
visitHead,visitRename) could potentially be consolidated into a default handler, but the current explicit approach ensures completeness verification viaacceptAndVerifyNodeVisited.core/src/test/java/org/opensearch/sql/ast/analysis/FieldResolutionResultTest.java (1)
219-233: Usefail()instead ofassertTrue(false, ...).At line 229, use
Assertions.fail("message")for cleaner test failure assertion.Suggested fix
try { unmodifiable.add("field3"); - assertTrue(false, "Should throw UnsupportedOperationException"); + fail("Should throw UnsupportedOperationException"); } catch (UnsupportedOperationException e) { // Expected }Also add the import:
import static org.junit.jupiter.api.Assertions.fail;core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionResult.java (1)
22-26: Class-level JavaDoc could be expanded.The brief one-liner at line 22 is minimal. Per coding guidelines, public classes should have proper JavaDoc explaining the class purpose, key methods, and usage patterns.
Suggested enhancement
-/** Field resolution result separating regular fields from wildcard patterns. */ +/** + * Represents the result of field resolution, separating explicit field references + * from wildcard patterns. Used by {@link FieldResolutionVisitor} to track field + * requirements at each AST node for schema-on-read features like spath. + * + * <p>Supports set operations (and, or, exclude) for composing field requirements + * through AST traversal, with wildcard algebra for pattern composition. + */
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
common/src/main/java/org/opensearch/sql/common/utils/DebugUtils.javacore/src/main/java/org/opensearch/sql/analysis/Analyzer.javacore/src/main/java/org/opensearch/sql/ast/AstNodeUtils.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionContext.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionResult.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendCore.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.javacore/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.javacore/src/main/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImpl.javacore/src/test/java/org/opensearch/sql/ast/AstNodeUtilsTest.javacore/src/test/java/org/opensearch/sql/ast/analysis/FieldResolutionResultTest.javacore/src/test/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImplTest.javacore/src/test/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImplTest.javacore/src/test/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImplTest.javadocs/dev/ppl-commands.mddocs/user/ppl/cmd/spath.mddoctest/test_data/structured.jsoninteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/standalone/JsonExtractAllFunctionIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/standalone/MapAppendFunctionIT.javainteg-test/src/test/java/org/opensearch/sql/util/MatcherUtils.javainteg-test/src/test/resources/expectedOutput/calcite/explain_spath_without_path.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_spath_without_path.yamlppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorCoverageTest.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.java
✅ Files skipped from review due to trivial changes (1)
- integ-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_spath_without_path.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
- integ-test/src/test/resources/expectedOutput/calcite/explain_spath_without_path.yaml
- core/src/main/java/org/opensearch/sql/ast/AstNodeUtils.java
- integ-test/src/test/java/org/opensearch/sql/calcite/standalone/JsonExtractAllFunctionIT.java
- core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionContext.java
- core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.java
- core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java
- integ-test/src/test/java/org/opensearch/sql/util/MatcherUtils.java
- core/src/test/java/org/opensearch/sql/ast/AstNodeUtilsTest.java
🧰 Additional context used
📓 Path-based instructions (10)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
core/src/test/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImplTest.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendFunctionImpl.javacommon/src/main/java/org/opensearch/sql/common/utils/DebugUtils.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/analysis/Analyzer.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImpl.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.javainteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.javacore/src/test/java/org/opensearch/sql/ast/analysis/FieldResolutionResultTest.javainteg-test/src/test/java/org/opensearch/sql/calcite/standalone/MapAppendFunctionIT.javappl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javacore/src/main/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImpl.javacore/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendCore.javacore/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.javacore/src/test/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImplTest.javacore/src/test/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImplTest.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionResult.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorCoverageTest.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
core/src/test/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImplTest.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendFunctionImpl.javacommon/src/main/java/org/opensearch/sql/common/utils/DebugUtils.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/analysis/Analyzer.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImpl.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.javainteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.javacore/src/test/java/org/opensearch/sql/ast/analysis/FieldResolutionResultTest.javainteg-test/src/test/java/org/opensearch/sql/calcite/standalone/MapAppendFunctionIT.javappl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javacore/src/main/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImpl.javacore/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendCore.javacore/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.javacore/src/test/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImplTest.javacore/src/test/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImplTest.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionResult.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorCoverageTest.java
**/*Test.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*Test.java: All new business logic requires unit tests
Name unit tests with*Test.javasuffix in OpenSearch SQL
Files:
core/src/test/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImplTest.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.javacore/src/test/java/org/opensearch/sql/ast/analysis/FieldResolutionResultTest.javacore/src/test/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImplTest.javacore/src/test/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImplTest.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorCoverageTest.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
core/src/test/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImplTest.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.javainteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.javacore/src/test/java/org/opensearch/sql/ast/analysis/FieldResolutionResultTest.javainteg-test/src/test/java/org/opensearch/sql/calcite/standalone/MapAppendFunctionIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javacore/src/test/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImplTest.javacore/src/test/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImplTest.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorCoverageTest.java
core/src/main/java/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/**/*.java: - New functions MUST have unit tests in the same commit
Files:
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/analysis/Analyzer.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javacore/src/main/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImpl.javacore/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendCore.javacore/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionResult.java
core/src/main/java/org/opensearch/sql/expression/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/org/opensearch/sql/expression/**/*.java: - New expression implementations must follow existing patterns
- Type handling must be consistent with project type system
- Error handling must use appropriate exception types
- Null handling must be explicit and documented
Files:
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImpl.javacore/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendCore.javacore/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.java
**/ppl/**/*.java
⚙️ CodeRabbit configuration file
**/ppl/**/*.java: - For PPL parser changes, verify grammar tests with positive/negative cases
- Check AST generation for new syntax
- Ensure corresponding AST builder classes are updated
- Validate edge cases and boundary conditions
Files:
ppl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.javappl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorCoverageTest.java
core/src/main/java/org/opensearch/sql/ast/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/org/opensearch/sql/ast/**/*.java: - AST nodes must be immutable where possible
- Follow visitor pattern for AST traversal
- Ensure proper toString() implementation for debugging
Files:
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionResult.java
integ-test/**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
End-to-end scenarios need integration tests in
integ-test/module
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/standalone/MapAppendFunctionIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
⚙️ CodeRabbit configuration file
integ-test/**/*IT.java: - Integration tests MUST use valid test data from resources
- Verify test data files exist in integ-test/src/test/resources/
- Check test assertions are meaningful and specific
- Validate tests clean up resources after execution
- Ensure tests are independent and can run in any order
- Flag tests that reference non-existent indices (e.g., EMP)
- Verify integration tests are in correct module (integ-test/)
- Check tests can be run with ./gradlew :integ-test:integTest
- Ensure proper test data setup and teardown
- Validate end-to-end scenario coverage
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/standalone/MapAppendFunctionIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
Name integration tests with
*IT.javasuffix in OpenSearch SQL
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/standalone/MapAppendFunctionIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
**/calcite/**/*.java
⚙️ CodeRabbit configuration file
**/calcite/**/*.java: - Follow existing Calcite integration patterns
- Verify RelNode visitor implementations are complete
- Check RexNode handling follows project conventions
- Validate SQL generation is correct and optimized
- Ensure Calcite version compatibility
- Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor
- Document any Calcite-specific workarounds
- Test compatibility with Calcite version constraints
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/standalone/MapAppendFunctionIT.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
🧠 Learnings (15)
📓 Common learnings
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration
Applied to files:
docs/dev/ppl-commands.mdcore/src/main/java/org/opensearch/sql/executor/QueryService.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.javacore/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javacore/src/main/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImpl.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorCoverageTest.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
docs/dev/ppl-commands.mdcore/src/main/java/org/opensearch/sql/executor/QueryService.javainteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/standalone/MapAppendFunctionIT.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.javappl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorCoverageTest.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*Test.java : Name unit tests with `*Test.java` suffix in OpenSearch SQL
Applied to files:
ppl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.javacore/src/test/java/org/opensearch/sql/ast/analysis/FieldResolutionResultTest.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL
Applied to files:
ppl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.javainteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
📚 Learning: 2025-12-11T05:27:39.856Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 0
File: :0-0
Timestamp: 2025-12-11T05:27:39.856Z
Learning: In opensearch-project/sql, for SEMI and ANTI join types in CalciteRelNodeVisitor.java, the `max` option has no effect because these join types only use the left side to filter records based on the existence of matches in the right side. The join results are identical regardless of max value (max=1, max=2, or max=∞). The early return for SEMI/ANTI joins before processing the `max` option is intentional and correct behavior.
Applied to files:
ppl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.javainteg-test/src/test/java/org/opensearch/sql/calcite/standalone/MapAppendFunctionIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*.java : All public classes and methods must have proper JavaDoc
Applied to files:
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Flag missing JavaDoc on public APIs during code review
Applied to files:
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*.java : Document Calcite-specific workarounds in code
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/standalone/MapAppendFunctionIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
📚 Learning: 2025-12-29T05:32:03.491Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:03.491Z
Learning: For any custom Calcite RelNode class (e.g., ones that extend EnumerableLimitSort or other Calcite RelNode types), always override the copy method. If copy is not overridden, cloning/copy operations may downgrade the instance to the parent class type, losing the custom behavior. In your implementation, ensure copy returns a new instance of the concrete class with all relevant fields and traits preserved, mirroring the current instance state.
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Update corresponding AST builder classes when making PPL grammar changes
Applied to files:
ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*.java : Prefer `Optional<T>` for nullable returns in Java
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*.java : Use `camelCase` for method and variable names (e.g., `executeQuery`)
Applied to files:
core/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*.java : Use `PascalCase` for class names (e.g., `QueryExecutor`)
Applied to files:
core/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.java
📚 Learning: 2025-12-29T05:32:11.893Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:11.893Z
Learning: In opensearch-project/sql, when creating custom Calcite RelNode classes that extend EnumerableLimitSort or other Calcite RelNode types, always override the `copy` method. Without overriding copy, the class will downgrade to its parent class type during copy operations, losing the custom implementation.
Applied to files:
ppl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorCoverageTest.java
🧬 Code graph analysis (12)
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendFunctionImpl.java (1)
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendCore.java (1)
AppendCore(14-59)
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImpl.java (1)
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendCore.java (1)
AppendCore(14-59)
core/src/main/java/org/opensearch/sql/analysis/Analyzer.java (1)
common/src/main/java/org/opensearch/sql/common/patterns/PatternUtils.java (1)
PatternUtils(15-170)
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImpl.java (2)
core/src/main/java/org/opensearch/sql/expression/function/ImplementorUDF.java (1)
ImplementorUDF(17-40)core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendCore.java (1)
AppendCore(14-59)
core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java (1)
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java (1)
FieldResolutionVisitor(76-624)
core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java (1)
core/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.java (1)
PPLBuiltinOperators(111-514)
core/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.java (1)
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImpl.java (1)
AppendFunctionImpl(27-65)
core/src/test/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImplTest.java (1)
core/src/main/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImpl.java (1)
JsonExtractAllFunctionImpl(43-242)
core/src/test/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImplTest.java (1)
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImpl.java (1)
AppendFunctionImpl(27-65)
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionResult.java (1)
core/src/main/java/org/opensearch/sql/calcite/utils/WildcardUtils.java (1)
WildcardUtils(13-233)
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java (1)
integ-test/src/test/java/org/opensearch/sql/util/MatcherUtils.java (1)
MatcherUtils(43-506)
ppl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorCoverageTest.java (3)
core/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.java (1)
AbstractNodeVisitor(94-464)core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java (1)
FieldResolutionVisitor(76-624)core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java (1)
CalciteRelNodeVisitor(168-3490)
🪛 Biome (2.1.2)
doctest/test_data/structured.json
[error] 1-2: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 2-3: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: build-linux (21, unit)
- GitHub Check: build-linux (25, integration)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (21, doc)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (25, doc)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: security-it-linux (21)
- GitHub Check: security-it-linux (25)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: test-sql-cli-integration (21)
- GitHub Check: CodeQL-Scan (java)
common/src/main/java/org/opensearch/sql/common/utils/DebugUtils.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendCore.java
Show resolved
Hide resolved
Signed-off-by: Tomoyuki Morita <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImpl.java:
- Around line 27-28: JavaDoc link syntax in MapAppendFunctionImpl's class
comment is wrong; update the reference from {@link: AppendCore} to the correct
{@link AppendCore} form so the JavaDoc generates properly and links to the
AppendCore class; locate the class-level comment in MapAppendFunctionImpl and
remove the colon inside the {@link ...} tag.
🧹 Nitpick comments (1)
opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchRequest.java (1)
56-62: Javadoc tag fix is good, but method reference syntax should use#instead of.Changing
@refto@linkis correct since@refis not a valid Javadoc tag. However, Javadoc method references should use the#notation (ClassName#methodName) rather than dot notation for proper linking.📝 Suggested fix
/** * Check if there is more data to get from OpenSearch. * - * @return True if calling {@link OpenSearchClient.search} with this request will return non-empty + * @return True if calling {@link OpenSearchClient#search} with this request will return non-empty * response. */
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendCore.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/expression/function/FunctionResolver.javacore/src/main/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImpl.javainteg-test/src/test/resources/expectedOutput/calcite/explain_spath_without_path.yamlopensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchRequest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/main/java/org/opensearch/sql/expression/function/jsonUDF/JsonExtractAllFunctionImpl.java
- core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendCore.java
🧰 Additional context used
📓 Path-based instructions (4)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/expression/function/FunctionResolver.javaopensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchRequest.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendFunctionImpl.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/expression/function/FunctionResolver.javaopensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchRequest.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendFunctionImpl.java
core/src/main/java/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/**/*.java: - New functions MUST have unit tests in the same commit
Files:
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/expression/function/FunctionResolver.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendFunctionImpl.java
core/src/main/java/org/opensearch/sql/expression/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/org/opensearch/sql/expression/**/*.java: - New expression implementations must follow existing patterns
- Type handling must be consistent with project type system
- Error handling must use appropriate exception types
- Null handling must be explicit and documented
Files:
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImpl.javacore/src/main/java/org/opensearch/sql/expression/function/FunctionResolver.javacore/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendFunctionImpl.java
integ-test/src/test/resources/**/*
⚙️ CodeRabbit configuration file
integ-test/src/test/resources/**/*: - Verify test data is realistic and representative
- Check data format matches expected schema
- Ensure test data covers edge cases and boundary conditions
Files:
integ-test/src/test/resources/expectedOutput/calcite/explain_spath_without_path.yaml
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
integ-test/src/test/resources/expectedOutput/calcite/explain_spath_without_path.yaml
📚 Learning: 2025-12-11T05:27:39.856Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 0
File: :0-0
Timestamp: 2025-12-11T05:27:39.856Z
Learning: In opensearch-project/sql, for SEMI and ANTI join types in CalciteRelNodeVisitor.java, the `max` option has no effect because these join types only use the left side to filter records based on the existence of matches in the right side. The join results are identical regardless of max value (max=1, max=2, or max=∞). The early return for SEMI/ANTI joins before processing the `max` option is intentional and correct behavior.
Applied to files:
integ-test/src/test/resources/expectedOutput/calcite/explain_spath_without_path.yaml
🧬 Code graph analysis (2)
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImpl.java (1)
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendCore.java (1)
AppendCore(14-59)
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendFunctionImpl.java (1)
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendCore.java (1)
AppendCore(14-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: security-it-linux (21)
- GitHub Check: security-it-linux (25)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (21, unit)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (25, doc)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (25, integration)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: test-sql-cli-integration (21)
🔇 Additional comments (4)
core/src/main/java/org/opensearch/sql/expression/function/FunctionResolver.java (1)
10-13: LGTM! Correct Javadoc tag fix.The change from
{@ref}to{@link}is appropriate since{@ref}is not a valid Javadoc tag. This fix ensures proper hyperlink generation in the generated documentation.integ-test/src/test/resources/expectedOutput/calcite/explain_spath_without_path.yaml (1)
1-8: LGTM! Expected output correctly captures SPATH without explicit path scenario.The logical and physical plans are consistent and representative:
- Logical plan correctly shows field extraction via
JSON_EXTRACT_ALLandITEMwith cast toVARCHAR NOT NULL(aligning with PR objective that all extracted fields are returned as STRING).- Physical plan demonstrates proper projection pushdown (
_sourceincludes onlymessage), matching theinput=messageparameter.- The plan structure follows established Calcite explain output patterns.
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendFunctionImpl.java (1)
99-110: LGTM! Clean delegation pattern with appropriate behavioral distinction.The refactoring correctly maintains the
mvappendcontract (always returning an array or null) by usingcollectElementsToListrather thancollectElements. This differs fromAppendCore.collectElementswhich unwraps single-element results—an important distinction sincemvappendmust guarantee array-type returns.The JavaDoc appropriately references the test class for detailed behavior documentation.
core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImpl.java (1)
89-106: LGTM! Correctly usingAppendCore.collectElementsfor value merging.The switch to
AppendCore.collectElementsis appropriate here—map values should remain scalar when there's a single value and become arrays only when merging multiple values for the same key. This aligns with the behavioral difference betweenAppendCore.collectElements(unwraps single elements) andMVAppendFunctionImpl.collectElements(always returns list).
...rc/main/java/org/opensearch/sql/expression/function/CollectionUDF/MapAppendFunctionImpl.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Tomoyuki Morita <[email protected]>
Signed-off-by: Tomoyuki Morita <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
common/src/main/java/org/opensearch/sql/common/utils/DebugUtils.java (4)
23-33: Missing JavaDoc on public methods.As per coding guidelines, all public methods must have proper JavaDoc. Both
debugmethods are missing documentation explaining their purpose, parameters, and return values.📝 Suggested JavaDoc addition
+ /** + * Logs the given object with a custom message and returns it unchanged. + * Only works when IS_DEBUG is true; throws RuntimeException otherwise. + * + * @param obj the object to debug + * @param message a descriptive message to include in the log + * @param <T> the type of the object + * @return the original object unchanged + * @throws RuntimeException if IS_DEBUG is false + */ public static <T> T debug(T obj, String message) { verifyDebug(); print("### %s: %s (at %s)", message, stringify(obj), getCalledFrom(1)); return obj; } + /** + * Logs the given object and returns it unchanged. + * Only works when IS_DEBUG is true; throws RuntimeException otherwise. + * + * @param obj the object to debug + * @param <T> the type of the object + * @return the original object unchanged + * @throws RuntimeException if IS_DEBUG is false + */ public static <T> T debug(T obj) {
51-79: Inconsistent null representation between Collection and Map.
stringify(Collection<?>)returns"null"for null input (line 53), whilestringify(Map<?, ?>)returns"[[null]]"(line 67). This inconsistency could be confusing when debugging mixed data structures.♻️ Suggested fix for consistency
private static String stringify(Map<?, ?> map) { if (map == null) { - return "[[null]]"; + return "null"; } if (map.isEmpty()) { - return "[[EMPTY]]"; + return "{}"; }
81-88: Raw type usage generates compiler warnings.The
instanceofchecks and casts use raw typesCollectionandMap. While this works, it generates unchecked cast warnings.♻️ Suggested fix to suppress warnings
+ @SuppressWarnings("unchecked") private static String stringify(Object obj) { if (obj instanceof Collection) { - return stringify((Collection) obj); + return stringify((Collection<?>) obj); } else if (obj instanceof Map) { - return stringify((Map) obj); + return stringify((Map<?, ?>) obj); } return String.valueOf(obj); }
45-49: Consider usingThread.currentThread().getStackTrace()instead of creating an exception.Creating a
RuntimeExceptionsolely to obtain the stack trace works but is unconventional.Thread.currentThread().getStackTrace()is the idiomatic approach and avoids the misleading exception creation.♻️ Suggested alternative
private static String getCalledFrom(int pos) { - RuntimeException e = new RuntimeException(); - StackTraceElement item = e.getStackTrace()[pos + 1]; + StackTraceElement[] stackTrace = Thread.currentThread().getStackTrace(); + // +2 accounts for getStackTrace() and getCalledFrom() calls + StackTraceElement item = stackTrace[pos + 3]; return item.getClassName() + "." + item.getMethodName() + ":" + item.getLineNumber(); }common/src/test/java/org/opensearch/sql/common/utils/DebugUtilsTest.java (2)
14-22: Tests adequately cover the safeguard behavior.The tests verify that both
debugmethod overloads throwRuntimeExceptionwhenIS_DEBUGis false, which is the primary invariant to protect. Consider verifying the exception message to ensure the correct exception is thrown.♻️ Optional: Verify exception message
@Test public void testDebugThrowsRuntimeException() { - assertThrows(RuntimeException.class, () -> DebugUtils.debug("test", "test message")); + RuntimeException ex = assertThrows(RuntimeException.class, + () -> DebugUtils.debug("test", "test message")); + assertEquals("DebugUtils can be used only for local debugging.", ex.getMessage()); } @Test public void testDebugWithoutMessageThrowsRuntimeException() { - assertThrows(RuntimeException.class, () -> DebugUtils.debug("test")); + RuntimeException ex = assertThrows(RuntimeException.class, + () -> DebugUtils.debug("test")); + assertEquals("DebugUtils can be used only for local debugging.", ex.getMessage()); }Note: Add
import static org.junit.Assert.assertEquals;at the top.
12-12: Minor: Consider adding class-level JavaDoc.Per coding guidelines, test classes benefit from brief documentation explaining what is being tested.
📝 Suggested addition
+/** Tests for {@link DebugUtils} safeguard behavior when IS_DEBUG is false. */ public class DebugUtilsTest {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
common/src/main/java/org/opensearch/sql/common/utils/DebugUtils.javacommon/src/test/java/org/opensearch/sql/common/utils/DebugUtilsTest.java
🧰 Additional context used
📓 Path-based instructions (3)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
common/src/test/java/org/opensearch/sql/common/utils/DebugUtilsTest.javacommon/src/main/java/org/opensearch/sql/common/utils/DebugUtils.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
common/src/test/java/org/opensearch/sql/common/utils/DebugUtilsTest.javacommon/src/main/java/org/opensearch/sql/common/utils/DebugUtils.java
**/*Test.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*Test.java: All new business logic requires unit tests
Name unit tests with*Test.javasuffix in OpenSearch SQL
Files:
common/src/test/java/org/opensearch/sql/common/utils/DebugUtilsTest.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
common/src/test/java/org/opensearch/sql/common/utils/DebugUtilsTest.java
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*Test.java : Name unit tests with `*Test.java` suffix in OpenSearch SQL
Applied to files:
common/src/test/java/org/opensearch/sql/common/utils/DebugUtilsTest.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL
Applied to files:
common/src/test/java/org/opensearch/sql/common/utils/DebugUtilsTest.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*Test.java : All new business logic requires unit tests
Applied to files:
common/src/test/java/org/opensearch/sql/common/utils/DebugUtilsTest.java
🧬 Code graph analysis (1)
common/src/test/java/org/opensearch/sql/common/utils/DebugUtilsTest.java (1)
common/src/main/java/org/opensearch/sql/common/utils/DebugUtils.java (1)
DebugUtils(18-89)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (25, doc)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (25, integration)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (21, unit)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: security-it-linux (21)
- GitHub Check: security-it-linux (25)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: test-sql-cli-integration (21)
- GitHub Check: CodeQL-Scan (java)
🔇 Additional comments (1)
common/src/main/java/org/opensearch/sql/common/utils/DebugUtils.java (1)
14-21: Good safeguard design for debugging utility.The
IS_DEBUG = falsesafeguard with runtime exception is a solid approach to prevent accidental usage in production. The class design allows inline debugging during development while ensuring it won't slip into production code.
| * result is used to support `spath` command (Field Resolution-based Extraction) and schema-on-read | ||
| * support in the future. | ||
| */ | ||
| public class FieldResolutionVisitor extends AbstractNodeVisitor<Node, FieldResolutionContext> { |
There was a problem hiding this comment.
Not sure if any better way to extract the input/output fields for each operator. But we can enforce by adding input/output to AST interface? I'm just concerned about the complexity of adding new PPL feature: #4960
There was a problem hiding this comment.
I think you mean adding a method to Node or UnresolvedPlan. That is an option, but has following downside:
- AST nodes will have implementation specific methods, though AST is abstraction of PPL/SQL query structure.
- We have several nodes which is not used by PPL, we will need to implement them. I suppose we would put dummy implementation as they won't be used for now, and we could forget to update them when we start using that node.
3 participants
Description
fieldscommand) or wildcard is used.mvappendfunction, but added new internalappendfunction to avoid returning single element array. (mvappendwill always make array regardless of array size, this is consistent with other array functions in PPL.appendwill return scalar value when the result become single element, this is similar to Splunk behavior where single element array is automatically converted to scalar value.)spathcommand becoming an array.Related Issues
#4984
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.