Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 97 additions & 0 deletions .coderabbit.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

# CodeRabbit Configuration for OpenSearch SQL Project
# This configuration uses .rules/REVIEW_GUIDELINES.md for code review standards

language: "en-US"
early_access: false

reviews:
profile: "chill"
request_changes_workflow: false
high_level_summary: true
high_level_summary_placeholder: "@coderabbitai summary"
poem: false # Keep reviews professional and concise
review_status: true
collapse_walkthrough: false

auto_review:
enabled: false # Disabled auto-review until it becomes stable
auto_incremental_review: false
drafts: false # Don't review draft PRs
ignore_title_keywords:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I think we can also start exercising this in our PR template when we initially creating PR.

- "WIP"
- "DO NOT MERGE"
- "DRAFT"

# Path-specific review instructions
path_instructions:
- path: "**/*.java"
instructions: |
- 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
- Check for Optional<T> usage instead of null returns
- Validate proper use of try-with-resources for resource management

- path: "**/test/**/*.java"
instructions: |
- 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

- path: "integ-test/**/*IT.java"
instructions: |
- 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: "**/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

- path: "**/calcite/**/*.java"
instructions: |
- Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor
- Verify SQL generation and optimization paths
- Document any Calcite-specific workarounds
- Test compatibility with Calcite version constraints

chat:
auto_reply: true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I like this setting, maybe others like it though?

Usually when responding to AI comments the only audience is other reviewers (e.g. "this comment doesn't apply" or "implemented"), I'm not sure I see the value of the AI reply


# Knowledge base configuration
knowledge_base:
# Don't opt out - use knowledge base features
opt_out: false

# Code guidelines - reference our custom review guidelines
code_guidelines:
enabled: true
filePatterns:
# Reference our custom review guidelines
- ".rules/REVIEW_GUIDELINES.md"

# Enable web search for additional context
web_search:
enabled: true

# Use repository-specific learnings for this project
learnings:
scope: "local"

# Use repository-specific issues
issues:
scope: "local"

# Use repository-specific pull requests for context
pull_requests:
scope: "local"
88 changes: 88 additions & 0 deletions .rules/REVIEW_GUIDELINES.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
# 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.

## Core Review Principles

### Code Quality
- **Simplicity First**: Prefer simpler solutions unless there's significant functional or performance degradation
- **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

### 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
- **JavaDoc Required**: All public classes and methods must have proper JavaDoc
- **Error Handling**: Use specific exception types with meaningful messages
- **Null Safety**: Prefer `Optional<T>` for nullable returns

### Testing Requirements
- **Test Coverage**: All new business logic requires unit tests
- **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

### Code Organization
- **Single Responsibility**: Each class should have one clear purpose
- **Package Structure**: Follow existing module organization (core, ppl, sql, opensearch)
- **Separation of Concerns**: Keep parsing, execution, and storage logic separate
- **Composition Over Inheritance**: Prefer composition for code reuse

### Performance & Security
- **Efficient Loops**: Avoid unnecessary object creation in loops
- **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

### What to Check
1. **Code Clarity**: Is the code self-explanatory?
2. **Test Coverage**: Are there adequate tests?
3. **Error Handling**: Are errors handled appropriately?
4. **Documentation**: Is JavaDoc complete and accurate?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this rule specific for command development?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is generic for any code change in this repository.

5. **Performance**: Are there obvious performance issues?
6. **Security**: Are inputs validated and sanitized?

### What to Flag
- Redundant or obvious comments
- Methods longer than 20 lines
- Missing JavaDoc on public APIs
- Generic exception handling
- Unused imports or dead code
- Hard-coded values that should be constants
- Missing or inadequate test coverage

### What to Encourage
- Clear, descriptive naming
- Proper use of Java idioms
- Comprehensive test coverage
- Meaningful error messages
- Efficient algorithms and data structures
- Security-conscious coding practices

## Project-Specific Guidelines

### OpenSearch SQL Context
- **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
- **Test Naming**: `*IT.java` for integration tests, `*Test.java` for unit tests

### PPL Parser Changes
- 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

### 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`
- Test SQL generation and optimization paths
- Document Calcite-specific workarounds
Loading