From f51d8d8e15727419949510e5d592456c2e859a0c Mon Sep 17 00:00:00 2001 From: Tomoyuki Morita Date: Mon, 1 Dec 2025 15:13:38 -0800 Subject: [PATCH 1/2] Add review instructions for CodeRabbit Signed-off-by: Tomoyuki Morita --- .coderabbit.yaml | 97 +++++++++++++++++++++++++++++++++++++ .rules/REVIEW_GUIDELINES.md | 88 +++++++++++++++++++++++++++++++++ 2 files changed, 185 insertions(+) create mode 100644 .coderabbit.yaml create mode 100644 .rules/REVIEW_GUIDELINES.md diff --git a/.coderabbit.yaml b/.coderabbit.yaml new file mode 100644 index 00000000000..ab886deb2e7 --- /dev/null +++ b/.coderabbit.yaml @@ -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: true + auto_incremental_review: true + drafts: false # Don't review draft PRs + ignore_title_keywords: + - "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 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 + +# 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" diff --git a/.rules/REVIEW_GUIDELINES.md b/.rules/REVIEW_GUIDELINES.md new file mode 100644 index 00000000000..8ffb24c7bd5 --- /dev/null +++ b/.rules/REVIEW_GUIDELINES.md @@ -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` 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? +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 From 0eebb9ca856309e9086e5cc31f527d7a61c1ce0d Mon Sep 17 00:00:00 2001 From: Tomoyuki Morita Date: Mon, 1 Dec 2025 15:53:18 -0800 Subject: [PATCH 2/2] Disable auto review Signed-off-by: Tomoyuki Morita --- .coderabbit.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index ab886deb2e7..11653b509a3 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -16,8 +16,8 @@ reviews: collapse_walkthrough: false auto_review: - enabled: true - auto_incremental_review: true + enabled: false # Disabled auto-review until it becomes stable + auto_incremental_review: false drafts: false # Don't review draft PRs ignore_title_keywords: - "WIP"