Merge remote-tracking branch 'origin/main' into issues/4636

Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>

Author: yuancu

.coderabbit.yaml

@@ -23,47 +23,138 @@ reviews:
       - "WIP"
       - "DO NOT MERGE"
       - "DRAFT"
+
+  # General review instructions (applied to all reviews)
+  instructions:
+    # Architectural Decision Prompts
+    - "For new features: Check if similar functionality exists that could be enhanced instead"
+    - "Question whether new code is needed vs reusing/extending existing code"
+    - "Identify opportunities for code reuse across the codebase"
 
   # Path-specific review instructions
   path_instructions:
+    # Grammar Files - Architectural Decision Prompts
+    - path: "**/*.g4"
+      instructions: |
+        - Check if modifying unrelated grammar files (scope creep)
+        - Verify grammar rule placement follows project patterns
+        - Question if new rule needed vs reusing existing rules
+        - Ensure changes are relevant to the PR's stated purpose
+    
+    - path: "ppl/src/main/antlr/OpenSearchPPLParser.g4"
+      instructions: |
+        - Identify code reuse opportunities with existing commands
+        - Validate command follows PPL naming and structure patterns
+        - Check if command should be alias vs separate implementation
+
+    # Java Files - Code Organization and Quality
     - path: "**/*.java"
       instructions: |
+        - 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 methods are under 20 lines with single responsibility
-        - Verify proper error handling with specific exception types
+        - Ensure proper error handling with specific exception types
         - Check for Optional<T> usage instead of null returns
         - Validate proper use of try-with-resources for resource management
     
+    # Core Java - Project-Specific Patterns
+    - path: "core/src/main/java/**/*.java"
+      instructions: |
+        - New functions MUST have unit tests in the same commit
+        - Public methods MUST have JavaDoc with @param, @return, and @throws
+        - Follow existing function implementation patterns in the same package
+        - New expression functions should follow ExpressionFunction interface patterns
+        - Validate function naming follows project conventions (camelCase)
+    
+    - path: "core/src/main/java/org/opensearch/sql/expression/**/*.java"
+      instructions: |
+        - 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
+    
+    - path: "core/src/main/java/org/opensearch/sql/ast/**/*.java"
+      instructions: |
+        - AST nodes must be immutable where possible
+        - Follow visitor pattern for AST traversal
+        - Ensure proper toString() implementation for debugging
+    
+    # Test Files - Enhanced Coverage Validation
     - path: "**/test/**/*.java"
       instructions: |
+        - 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<Functionality><Condition>
+        - Verify test data is realistic and covers edge cases
         - Verify test coverage for new business logic
-        - Check test naming follows conventions (*Test.java for unit, *IT.java for integration)
         - 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
     
+    # Integration Tests
     - path: "integ-test/**/*IT.java"
       instructions: |
+        - 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
     
+    - path: "integ-test/src/test/resources/**/*"
+      instructions: |
+        - Verify test data is realistic and representative
+        - Check data format matches expected schema
+        - Ensure test data covers edge cases and boundary conditions
+    
+    # PPL-specific
     - path: "**/ppl/**/*.java"
       instructions: |
         - 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
     
+    # Calcite Integration
     - path: "**/calcite/**/*.java"
       instructions: |
+        - 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
-        - Verify SQL generation and optimization paths
         - Document any Calcite-specific workarounds
         - Test compatibility with Calcite version constraints
+    
+    - path: "core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java"
+      instructions: |
+        - 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
+    
+    # Documentation
+    - path: "docs/**/*.rst"
+      instructions: |
+        - Verify examples use valid test data and indices
+        - Check documentation follows existing patterns and structure
+        - Validate syntax examples are complete and correct
+        - Ensure code examples are tested and working
+        - Check for consistent formatting and style
 
 chat:
   auto_reply: false # require explicit tagging 

api/README.md

@@ -17,19 +17,34 @@ Together, these components enable a complete workflow: parse PPL queries into lo
 
 ## Usage
 
-### UnifiedQueryPlanner
+### UnifiedQueryContext
+
+`UnifiedQueryContext` is a reusable abstraction shared across unified query components (planner, compiler, etc.). It bundles `CalcitePlanContext` and `Settings` into a single object, centralizing configuration for all unified query operations.
 
-Use the declarative, fluent builder API to initialize the `UnifiedQueryPlanner`.
+Create a context with catalog configuration, query type, and optional settings:
 
 ```java
-UnifiedQueryPlanner planner = UnifiedQueryPlanner.builder()
+UnifiedQueryContext context = UnifiedQueryContext.builder()
     .language(QueryType.PPL)
-    .catalog("opensearch", schema)
+    .catalog("opensearch", opensearchSchema)
+    .catalog("spark_catalog", sparkSchema)
     .defaultNamespace("opensearch")
     .cacheMetadata(true)
+    .setting("plugins.query.size_limit", 200)
     .build();
+```
+
+### UnifiedQueryPlanner
 
-RelNode plan = planner.plan("source = opensearch.test");
+Use `UnifiedQueryPlanner` to parse and analyze PPL queries into Calcite logical plans. The planner accepts a `UnifiedQueryContext` and can be reused for multiple queries.
+
+```java
+// Create planner with context
+UnifiedQueryPlanner planner = new UnifiedQueryPlanner(context);
+
+// Plan multiple queries (context is reused)
+RelNode plan1 = planner.plan("source = logs | where status = 200");
+RelNode plan2 = planner.plan("source = metrics | stats avg(cpu)");
 ```
 
 ### UnifiedQueryTranspiler
@@ -46,25 +61,28 @@ String sql = transpiler.toSql(plan);
 
 ### Complete Workflow Example
 
-Combining both components to transpile PPL queries into target database SQL:
+Combining all components to transpile PPL queries into target database SQL:
 
 ```java
-// Step 1: Initialize planner
-UnifiedQueryPlanner planner = UnifiedQueryPlanner.builder()
+// Step 1: Create reusable context (shared across components)
+UnifiedQueryContext context = UnifiedQueryContext.builder()
     .language(QueryType.PPL)
     .catalog("catalog", schema)
     .defaultNamespace("catalog")
     .build();
 
-// Step 2: Parse PPL query into logical plan
+// Step 2: Create planner with context
+UnifiedQueryPlanner planner = new UnifiedQueryPlanner(context);
+
+// Step 3: Plan PPL query into logical plan
 RelNode plan = planner.plan("source = employees | where age > 30");
 
-// Step 3: Initialize transpiler with target dialect
+// Step 4: Create transpiler with target dialect
 UnifiedQueryTranspiler transpiler = UnifiedQueryTranspiler.builder()
     .dialect(SparkSqlDialect.DEFAULT)
     .build();
 
-// Step 4: Transpile to target SQL
+// Step 5: Transpile to target SQL
 String sparkSql = transpiler.toSql(plan);
 // Result: SELECT * FROM `catalog`.`employees` WHERE `age` > 30
 ```