Improve coderabbit config

.coderabbit.yaml

@@ -14,7 +14,11 @@ reviews:
   poem: false  # Keep reviews professional and concise
   review_status: true
   collapse_walkthrough: true
-
+  path_filters:
+    - '**/*'
+    - '!**/test/**/gen/**'
+    - '!**/.git/**'
+    - '!**/target/**'
   auto_review:
     enabled: true
     auto_incremental_review: true
@@ -23,13 +27,6 @@ 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:
@@ -47,21 +44,10 @@ reviews:
         - Validate command follows PPL naming and structure patterns
         - Check if command should be alias vs separate implementation
 
-    # Java Files - Code Organization and Quality
+    # Java Files - Apply general guidelines from REVIEW_GUIDELINES.md
     - 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 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
+        Apply all Java Standards from REVIEW_GUIDELINES.md
 
     # Core Java - Project-Specific Patterns
     - path: "core/src/main/java/**/*.java"
@@ -85,20 +71,10 @@ reviews:
         - Follow visitor pattern for AST traversal
         - Ensure proper toString() implementation for debugging
 
-    # Test Files - Enhanced Coverage Validation
+    # Test Files - Apply testing guidelines from REVIEW_GUIDELINES.md
     - 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
-        - 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
+        Apply all Testing Requirements from REVIEW_GUIDELINES.md
 
     # Integration Tests
     - path: "integ-test/**/*IT.java"
@@ -108,17 +84,19 @@ reviews:
         - 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
+        - Do NOT flag inline test data as problematic if it improves test clarity
+        - Do NOT suggest removing helper methods that are used multiple times in the same test class
 
     - 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
+        - Do NOT flag NDJSON format as invalid JSON
 
     # PPL-specific
     - path: "**/ppl/**/*.java"
@@ -147,14 +125,14 @@ reviews:
         - Check for code organization and logical grouping
         - Validate all RelNode types are handled
 
-    # Documentation
-    - path: "docs/**/*.rst"
+    - path: "docs/**/*.md"
       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
+        - Do NOT flag missing language identifiers in code blocks as critical issues
 
 chat:
   auto_reply: false # require explicit tagging 

.rules/REVIEW_GUIDELINES.md

@@ -1,6 +1,6 @@
 # Code Review Guidelines for OpenSearch SQL
 
-This document provides guidelines for code reviews in the OpenSearch SQL project. These guidelines are used by CodeRabbit AI for automated code reviews and serve as a reference for human reviewers.
+This document provides general code review principles and standards for the OpenSearch SQL project. These guidelines apply across all file types and are used by CodeRabbit AI for automated code reviews.
 
 ## Core Review Principles
 
@@ -9,19 +9,40 @@ This document provides guidelines for code reviews in the OpenSearch SQL project
 - **Self-Documenting Code**: Code should be clear through naming and structure, not comments
 - **No Redundant Comments**: Avoid comments that merely restate what the code does
 - **Concise Implementation**: Keep code, docs, and notes short and focused on essentials
+- **Code Reuse**: Identify opportunities across similar implementations
+- **Maintainability**: Assess if code is easy to understand and modify
 
 ### Java Standards
 - **Naming Conventions**: 
   - Classes: `PascalCase` (e.g., `QueryExecutor`)
   - Methods/Variables: `camelCase` (e.g., `executeQuery`)
   - Constants: `UPPER_SNAKE_CASE` (e.g., `MAX_RETRY_COUNT`)
-- **Method Size**: Keep methods under 20 lines with single responsibility
+- **Method Size**: 
+  - Target: Under 20 lines with single responsibility
+  - Suggest refactoring: Methods >100 lines
+- **Class Size**: Flag classes >500 lines for organization review
 - **JavaDoc Required**: All public classes and methods must have proper JavaDoc
+  - NOT required on test methods, test helpers, override methods, or private methods
 - **Error Handling**: Use specific exception types with meaningful messages
 - **Null Safety**: Prefer `Optional<T>` for nullable returns
+- **Resource Management**: Use try-with-resources for proper cleanup
+- **Dead Code**: Check for unused imports, variables, and methods
 
 ### Testing Requirements
-- **Test Coverage**: All new business logic requires unit tests
+- **Test Coverage**: All new business logic requires unit tests in the same commit
+- **Required Test Cases**:
+  - NULL input tests for all new functions
+  - Boundary condition tests (min/max values, empty inputs)
+  - Error condition tests (invalid inputs, exceptions)
+  - Multi-document tests for per-document operations
+- **Test Naming**: Follow pattern `test<Functionality><Condition>`
+- **Test Independence**: Tests must not rely on execution order
+- **Test Data**: 
+  - Use realistic data that reflects real-world scenarios
+  - Inline test data is acceptable when it improves readability
+  - Don't require loading from files if inline is clearer
+- **Test Assertions**: Flag smoke tests without meaningful assertions only if they provide no value
+- **Resource Cleanup**: Ensure proper cleanup of test resources
 - **Integration Tests**: End-to-end scenarios need integration tests in `integ-test/` module
 - **Test Execution**: Verify changes with `./gradlew :integ-test:integTest`
 - **No Failing Tests**: All tests must pass before merge; fix or ask for guidance if blocked
@@ -37,7 +58,6 @@ This document provides guidelines for code reviews in the OpenSearch SQL project
 - **String Handling**: Use `StringBuilder` for concatenation in loops
 - **Input Validation**: Validate all user inputs, especially queries
 - **Logging Safety**: Sanitize data before logging to prevent injection
-- **Resource Management**: Use try-with-resources for proper cleanup
 
 ## Review Focus Areas
 
@@ -51,7 +71,7 @@ This document provides guidelines for code reviews in the OpenSearch SQL project
 
 ### What to Flag
 - Redundant or obvious comments
-- Methods longer than 20 lines
+- Methods longer than target size (see Java Standards)
 - Missing JavaDoc on public APIs
 - Generic exception handling
 - Unused imports or dead code
@@ -66,23 +86,47 @@ This document provides guidelines for code reviews in the OpenSearch SQL project
 - Efficient algorithms and data structures
 - Security-conscious coding practices
 
-## Project-Specific Guidelines
+## Project Context
 
-### OpenSearch SQL Context
+### OpenSearch SQL
 - **JDK 21**: Required for development
 - **Java 11 Compatibility**: Maintain when possible for OpenSearch 2.x
-- **Module Structure**: Respect existing module boundaries
-- **Integration Tests**: Use `./gradlew :integ-test:integTest` for testing
+- **Module Structure**: Respect existing module boundaries (core, ppl, sql, opensearch, integ-test)
 - **Test Naming**: `*IT.java` for integration tests, `*Test.java` for unit tests
 
-### PPL Parser Changes
+### Grammar and Parser Development
 - Test new grammar rules with positive and negative cases
 - Verify AST generation for new syntax
 - Include edge cases and boundary conditions
 - Update corresponding AST builder classes
+- Check for code reuse opportunities vs creating new rules
 
 ### Calcite Integration
-- If the PR is for PPL command, refer docs/dev/ppl-commands.md and verify the PR satisfy the checklist.
-- Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor`
+- Follow existing patterns in visitor implementations
 - Test SQL generation and optimization paths
 - Document Calcite-specific workarounds
+- Ensure version compatibility
+
+## Review Tone and Focus
+
+### Review Priorities
+- **Correctness** > Performance > Maintainability > Style
+- Focus on functional issues, not cosmetic ones
+- Explain WHY when suggesting improvements, not just WHAT
+- Avoid repeating the same suggestion multiple times
+
+### What NOT to Flag
+- Minor refactorings unless code is clearly problematic (>100 lines, deeply nested, etc.)
+- Missing JavaDoc on test methods, test helpers, override methods, or private methods
+- Inline test data when it improves readability
+- NDJSON format as invalid JSON
+- Missing language identifiers in markdown code blocks (unless in documentation files)
+- Hard-coded test data when it's intentionally inline for clarity
+- Helper methods in tests unless they're truly unused
+- Extracting helper methods unless the method exceeds 100 lines
+
+### Keep Reviews Concise
+- Be direct and actionable
+- Focus on significant issues that impact functionality or maintainability
+- Avoid nitpicking on style preferences
+- Prioritize issues that could cause bugs or maintenance problems