@@ -23,47 +23,138 @@ reviews:
2323 - " WIP"
2424 - " DO NOT MERGE"
2525 - " DRAFT"
26+
27+ # General review instructions (applied to all reviews)
28+ instructions :
29+ # Architectural Decision Prompts
30+ - " For new features: Check if similar functionality exists that could be enhanced instead"
31+ - " Question whether new code is needed vs reusing/extending existing code"
32+ - " Identify opportunities for code reuse across the codebase"
2633
2734 # Path-specific review instructions
2835 path_instructions :
36+ # Grammar Files - Architectural Decision Prompts
37+ - path : " **/*.g4"
38+ instructions : |
39+ - Check if modifying unrelated grammar files (scope creep)
40+ - Verify grammar rule placement follows project patterns
41+ - Question if new rule needed vs reusing existing rules
42+ - Ensure changes are relevant to the PR's stated purpose
43+
44+ - path : " ppl/src/main/antlr/OpenSearchPPLParser.g4"
45+ instructions : |
46+ - Identify code reuse opportunities with existing commands
47+ - Validate command follows PPL naming and structure patterns
48+ - Check if command should be alias vs separate implementation
49+
50+ # Java Files - Code Organization and Quality
2951 - path : " **/*.java"
3052 instructions : |
53+ - Flag methods >50 lines as potentially too complex - suggest refactoring
54+ - Flag classes >500 lines as needing organization review
55+ - Check for dead code, unused imports, and unused variables
56+ - Identify code reuse opportunities across similar implementations
57+ - Assess holistic maintainability - is code easy to understand and modify?
58+ - Flag code that appears AI-generated without sufficient human review
3159 - Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
3260 - Check for proper JavaDoc on public classes and methods
3361 - Flag redundant comments that restate obvious code
34- - Ensure methods are under 20 lines with single responsibility
35- - Verify proper error handling with specific exception types
62+ - Ensure proper error handling with specific exception types
3663 - Check for Optional<T> usage instead of null returns
3764 - Validate proper use of try-with-resources for resource management
3865
66+ # Core Java - Project-Specific Patterns
67+ - path : " core/src/main/java/**/*.java"
68+ instructions : |
69+ - New functions MUST have unit tests in the same commit
70+ - Public methods MUST have JavaDoc with @param, @return, and @throws
71+ - Follow existing function implementation patterns in the same package
72+ - New expression functions should follow ExpressionFunction interface patterns
73+ - Validate function naming follows project conventions (camelCase)
74+
75+ - path : " core/src/main/java/org/opensearch/sql/expression/**/*.java"
76+ instructions : |
77+ - New expression implementations must follow existing patterns
78+ - Type handling must be consistent with project type system
79+ - Error handling must use appropriate exception types
80+ - Null handling must be explicit and documented
81+
82+ - path : " core/src/main/java/org/opensearch/sql/ast/**/*.java"
83+ instructions : |
84+ - AST nodes must be immutable where possible
85+ - Follow visitor pattern for AST traversal
86+ - Ensure proper toString() implementation for debugging
87+
88+ # Test Files - Enhanced Coverage Validation
3989 - path : " **/test/**/*.java"
4090 instructions : |
91+ - Verify NULL input tests for all new functions
92+ - Check boundary condition tests (min/max values, empty inputs)
93+ - Validate error condition tests (invalid inputs, exceptions)
94+ - Ensure multi-document tests for per-document operations
95+ - Flag smoke tests without meaningful assertions
96+ - Check test naming follows pattern: test<Functionality><Condition>
97+ - Verify test data is realistic and covers edge cases
4198 - Verify test coverage for new business logic
42- - Check test naming follows conventions (*Test.java for unit, *IT.java for integration)
4399 - Ensure tests are independent and don't rely on execution order
44100 - Validate meaningful test data that reflects real-world scenarios
45101 - Check for proper cleanup of test resources
46102
103+ # Integration Tests
47104 - path : " integ-test/**/*IT.java"
48105 instructions : |
106+ - Integration tests MUST use valid test data from resources
107+ - Verify test data files exist in integ-test/src/test/resources/
108+ - Check test assertions are meaningful and specific
109+ - Validate tests clean up resources after execution
110+ - Ensure tests are independent and can run in any order
111+ - Flag tests that reference non-existent indices (e.g., EMP)
49112 - Verify integration tests are in correct module (integ-test/)
50113 - Check tests can be run with ./gradlew :integ-test:integTest
51114 - Ensure proper test data setup and teardown
52115 - Validate end-to-end scenario coverage
53116
117+ - path : " integ-test/src/test/resources/**/*"
118+ instructions : |
119+ - Verify test data is realistic and representative
120+ - Check data format matches expected schema
121+ - Ensure test data covers edge cases and boundary conditions
122+
123+ # PPL-specific
54124 - path : " **/ppl/**/*.java"
55125 instructions : |
56126 - For PPL parser changes, verify grammar tests with positive/negative cases
57127 - Check AST generation for new syntax
58128 - Ensure corresponding AST builder classes are updated
59129 - Validate edge cases and boundary conditions
60130
131+ # Calcite Integration
61132 - path : " **/calcite/**/*.java"
62133 instructions : |
134+ - Follow existing Calcite integration patterns
135+ - Verify RelNode visitor implementations are complete
136+ - Check RexNode handling follows project conventions
137+ - Validate SQL generation is correct and optimized
138+ - Ensure Calcite version compatibility
63139 - Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor
64- - Verify SQL generation and optimization paths
65140 - Document any Calcite-specific workarounds
66141 - Test compatibility with Calcite version constraints
142+
143+ - path : " core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java"
144+ instructions : |
145+ - Flag methods >50 lines - this file is known to be hard to read
146+ - Suggest extracting complex logic into helper methods
147+ - Check for code organization and logical grouping
148+ - Validate all RelNode types are handled
149+
150+ # Documentation
151+ - path : " docs/**/*.rst"
152+ instructions : |
153+ - Verify examples use valid test data and indices
154+ - Check documentation follows existing patterns and structure
155+ - Validate syntax examples are complete and correct
156+ - Ensure code examples are tested and working
157+ - Check for consistent formatting and style
67158
68159chat :
69160 auto_reply : false # require explicit tagging
0 commit comments