Skip to content

Commit 2f6cbfd

Browse files
ykmr1224ykmr1224
authored
Add config for CodeRabbit review (#4890)
* Add review instructions for CodeRabbit Signed-off-by: Tomoyuki Morita <moritato@amazon.com> * Disable auto review Signed-off-by: Tomoyuki Morita <moritato@amazon.com> --------- Signed-off-by: Tomoyuki Morita <moritato@amazon.com>
1 parent 96370bf commit 2f6cbfd

2 files changed

Lines changed: 185 additions & 0 deletions

File tree

‎.coderabbit.yaml‎

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json
2+
3+
# CodeRabbit Configuration for OpenSearch SQL Project
4+
# This configuration uses .rules/REVIEW_GUIDELINES.md for code review standards
5+
6+
language: "en-US"
7+
early_access: false
8+
9+
reviews:
10+
profile: "chill"
11+
request_changes_workflow: false
12+
high_level_summary: true
13+
high_level_summary_placeholder: "@coderabbitai summary"
14+
poem: false # Keep reviews professional and concise
15+
review_status: true
16+
collapse_walkthrough: false
17+
18+
auto_review:
19+
enabled: false # Disabled auto-review until it becomes stable
20+
auto_incremental_review: false
21+
drafts: false # Don't review draft PRs
22+
ignore_title_keywords:
23+
- "WIP"
24+
- "DO NOT MERGE"
25+
- "DRAFT"
26+
27+
# Path-specific review instructions
28+
path_instructions:
29+
- path: "**/*.java"
30+
instructions: |
31+
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
32+
- Check for proper JavaDoc on public classes and methods
33+
- 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
36+
- Check for Optional<T> usage instead of null returns
37+
- Validate proper use of try-with-resources for resource management
38+
39+
- path: "**/test/**/*.java"
40+
instructions: |
41+
- Verify test coverage for new business logic
42+
- Check test naming follows conventions (*Test.java for unit, *IT.java for integration)
43+
- Ensure tests are independent and don't rely on execution order
44+
- Validate meaningful test data that reflects real-world scenarios
45+
- Check for proper cleanup of test resources
46+
47+
- path: "integ-test/**/*IT.java"
48+
instructions: |
49+
- Verify integration tests are in correct module (integ-test/)
50+
- Check tests can be run with ./gradlew :integ-test:integTest
51+
- Ensure proper test data setup and teardown
52+
- Validate end-to-end scenario coverage
53+
54+
- path: "**/ppl/**/*.java"
55+
instructions: |
56+
- For PPL parser changes, verify grammar tests with positive/negative cases
57+
- Check AST generation for new syntax
58+
- Ensure corresponding AST builder classes are updated
59+
- Validate edge cases and boundary conditions
60+
61+
- path: "**/calcite/**/*.java"
62+
instructions: |
63+
- Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor
64+
- Verify SQL generation and optimization paths
65+
- Document any Calcite-specific workarounds
66+
- Test compatibility with Calcite version constraints
67+
68+
chat:
69+
auto_reply: true
70+
71+
# Knowledge base configuration
72+
knowledge_base:
73+
# Don't opt out - use knowledge base features
74+
opt_out: false
75+
76+
# Code guidelines - reference our custom review guidelines
77+
code_guidelines:
78+
enabled: true
79+
filePatterns:
80+
# Reference our custom review guidelines
81+
- ".rules/REVIEW_GUIDELINES.md"
82+
83+
# Enable web search for additional context
84+
web_search:
85+
enabled: true
86+
87+
# Use repository-specific learnings for this project
88+
learnings:
89+
scope: "local"
90+
91+
# Use repository-specific issues
92+
issues:
93+
scope: "local"
94+
95+
# Use repository-specific pull requests for context
96+
pull_requests:
97+
scope: "local"

‎.rules/REVIEW_GUIDELINES.md‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
# Code Review Guidelines for OpenSearch SQL
2+
3+
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.
4+
5+
## Core Review Principles
6+
7+
### Code Quality
8+
- **Simplicity First**: Prefer simpler solutions unless there's significant functional or performance degradation
9+
- **Self-Documenting Code**: Code should be clear through naming and structure, not comments
10+
- **No Redundant Comments**: Avoid comments that merely restate what the code does
11+
- **Concise Implementation**: Keep code, docs, and notes short and focused on essentials
12+
13+
### Java Standards
14+
- **Naming Conventions**:
15+
- Classes: `PascalCase` (e.g., `QueryExecutor`)
16+
- Methods/Variables: `camelCase` (e.g., `executeQuery`)
17+
- Constants: `UPPER_SNAKE_CASE` (e.g., `MAX_RETRY_COUNT`)
18+
- **Method Size**: Keep methods under 20 lines with single responsibility
19+
- **JavaDoc Required**: All public classes and methods must have proper JavaDoc
20+
- **Error Handling**: Use specific exception types with meaningful messages
21+
- **Null Safety**: Prefer `Optional<T>` for nullable returns
22+
23+
### Testing Requirements
24+
- **Test Coverage**: All new business logic requires unit tests
25+
- **Integration Tests**: End-to-end scenarios need integration tests in `integ-test/` module
26+
- **Test Execution**: Verify changes with `./gradlew :integ-test:integTest`
27+
- **No Failing Tests**: All tests must pass before merge; fix or ask for guidance if blocked
28+
29+
### Code Organization
30+
- **Single Responsibility**: Each class should have one clear purpose
31+
- **Package Structure**: Follow existing module organization (core, ppl, sql, opensearch)
32+
- **Separation of Concerns**: Keep parsing, execution, and storage logic separate
33+
- **Composition Over Inheritance**: Prefer composition for code reuse
34+
35+
### Performance & Security
36+
- **Efficient Loops**: Avoid unnecessary object creation in loops
37+
- **String Handling**: Use `StringBuilder` for concatenation in loops
38+
- **Input Validation**: Validate all user inputs, especially queries
39+
- **Logging Safety**: Sanitize data before logging to prevent injection
40+
- **Resource Management**: Use try-with-resources for proper cleanup
41+
42+
## Review Focus Areas
43+
44+
### What to Check
45+
1. **Code Clarity**: Is the code self-explanatory?
46+
2. **Test Coverage**: Are there adequate tests?
47+
3. **Error Handling**: Are errors handled appropriately?
48+
4. **Documentation**: Is JavaDoc complete and accurate?
49+
5. **Performance**: Are there obvious performance issues?
50+
6. **Security**: Are inputs validated and sanitized?
51+
52+
### What to Flag
53+
- Redundant or obvious comments
54+
- Methods longer than 20 lines
55+
- Missing JavaDoc on public APIs
56+
- Generic exception handling
57+
- Unused imports or dead code
58+
- Hard-coded values that should be constants
59+
- Missing or inadequate test coverage
60+
61+
### What to Encourage
62+
- Clear, descriptive naming
63+
- Proper use of Java idioms
64+
- Comprehensive test coverage
65+
- Meaningful error messages
66+
- Efficient algorithms and data structures
67+
- Security-conscious coding practices
68+
69+
## Project-Specific Guidelines
70+
71+
### OpenSearch SQL Context
72+
- **JDK 21**: Required for development
73+
- **Java 11 Compatibility**: Maintain when possible for OpenSearch 2.x
74+
- **Module Structure**: Respect existing module boundaries
75+
- **Integration Tests**: Use `./gradlew :integ-test:integTest` for testing
76+
- **Test Naming**: `*IT.java` for integration tests, `*Test.java` for unit tests
77+
78+
### PPL Parser Changes
79+
- Test new grammar rules with positive and negative cases
80+
- Verify AST generation for new syntax
81+
- Include edge cases and boundary conditions
82+
- Update corresponding AST builder classes
83+
84+
### Calcite Integration
85+
- If the PR is for PPL command, refer docs/dev/ppl-commands.md and verify the PR satisfy the checklist.
86+
- Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor`
87+
- Test SQL generation and optimization paths
88+
- Document Calcite-specific workarounds

0 commit comments

Comments
 (0)