Skip to content

Conversation

@aparajita31pandey
Copy link
Contributor

@aparajita31pandey aparajita31pandey commented Dec 22, 2025

Description

This diff is removes a redundant alias-resolution call via the GET /_alias/<alias> API that requires the caller to have extra indices:admin/aliases/get privileges, which can cause permission issues when executing read queries.

It instead leverages existing GetFieldMapping call that works for both index and alias.

Tested Functionality

With Index Name

curl -X POST "localhost:9200/_plugins/_sql"  -H 'Content-Type: application/json' \
  -d '{
    "query": "SELECT * FROM my-index",
    "fetch_size": 1,
    "filter": {
      "term": {
        "another_field": "hello"
      }
    }
  }'
Output - 
{
  "schema": [
    {
      "name": "new_field",
      "type": "text"
    },
    {
      "name": "another_field",
      "type": "keyword"
    }
  ],
  "total": 1,
  "datarows": [[
    "Some text",
    "hello"
  ]],
  "size": 1,
  "status": 200
}

With Alias Name - Same Output

curl -X POST "localhost:9200/_plugins/_sql"   -H 'Content-Type: application/json' \
  -d '{
    "query": "SELECT * FROM my-alias",
    "fetch_size": 1,
    "filter": {
      "term": {
        "another_field": "hello"
      }
    }
  }'
Output - 
{
  "schema": [
    {
      "name": "new_field",
      "type": "text"
    },
    {
      "name": "another_field",
      "type": "keyword"
    }
  ],
  "total": 1,
  "datarows": [[
    "Some text",
    "hello"
  ]],
  "size": 1,
  "status": 200
}

Related Issues

Resolves
#2960, opensearch-project/security#5871

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • New functionality has javadoc added.
  • New functionality has a user manual doc added.
  • New PPL command checklist all confirmed.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff or -s.
  • Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 22, 2025

Warning

Rate limit exceeded

@aparajita31pandey has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 3 minutes and 27 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between ae7e692 and daff240.

📒 Files selected for processing (2)
  • integ-test/src/test/java/org/opensearch/sql/sql/PaginationIT.java
  • legacy/src/main/java/org/opensearch/sql/legacy/executor/format/SelectResultSet.java
📝 Walkthrough

Walkthrough

Removed runtime alias-resolution and alias-based indexName override in SelectResultSet; mappings validation relaxed to require only non-empty mappings and the first mapping is used; added an integration test that verifies query consistency between direct indices and a multi-index alias.

Changes

Cohort / File(s) Change Summary
Alias resolution & mapping selection
legacy/src/main/java/org/opensearch/sql/legacy/executor/format/SelectResultSet.java
Removed imports and usage of GetAliasesRequest/GetAliasesResponse; stopped overriding indexName from resolved aliases in loadFromEsState; relaxed mappings check to only ensure non-empty mappings and select type mappings via mappings.values().iterator().next(); adjusted related error handling and imports.
Integration test for alias behavior
integ-test/src/test/java/org/opensearch/sql/sql/PaginationIT.java
Added loading of Index.DOG and Index.DOGS2; new test testAliasResultConsistency() creates a multi-index alias, runs equivalent queries against an index and the alias, asserts equal result sizes/schemas, and removes the alias.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

enhancement

Suggested reviewers

  • LantaoJin
  • Swiddis
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Remove GetAlias Call' is concise and directly reflects the main change—removing the redundant GetAlias API call that required extra privileges.
Description check ✅ Passed The description clearly explains the change: removing a redundant alias-resolution call to avoid permission issues, with concrete test examples showing functionality with both index and alias names.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cbcdbd6 and 8b7cb6a.

📒 Files selected for processing (1)
  • legacy/src/main/java/org/opensearch/sql/legacy/executor/format/SelectResultSet.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java

📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)

**/*.java: Use PascalCase for class names (e.g., QueryExecutor)
Use camelCase for method and variable names (e.g., executeQuery)
Use UPPER_SNAKE_CASE for constants (e.g., MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
Prefer Optional<T> for nullable returns in Java
Avoid unnecessary object creation in loops
Use StringBuilder for string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code

Files:

  • legacy/src/main/java/org/opensearch/sql/legacy/executor/format/SelectResultSet.java

⚙️ CodeRabbit configuration file

**/*.java: - 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 usage instead of null returns
  • Validate proper use of try-with-resources for resource management

Files:

  • legacy/src/main/java/org/opensearch/sql/legacy/executor/format/SelectResultSet.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
  • GitHub Check: security-it-linux (25)
  • GitHub Check: security-it-linux (21)
  • GitHub Check: bwc-tests-full-restart (21)
  • GitHub Check: build-linux (21, doc)
  • GitHub Check: build-linux (25, integration)
  • GitHub Check: build-linux (21, integration)
  • GitHub Check: bwc-tests-rolling-upgrade (25)
  • GitHub Check: build-linux (25, unit)
  • GitHub Check: bwc-tests-rolling-upgrade (21)
  • GitHub Check: build-linux (21, unit)
  • GitHub Check: build-linux (25, doc)
  • GitHub Check: bwc-tests-full-restart (25)
  • GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
  • GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
  • GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
  • GitHub Check: build-windows-macos (macos-14, 21, integration)
  • GitHub Check: build-windows-macos (macos-14, 21, unit)
  • GitHub Check: build-windows-macos (macos-14, 25, integration)
  • GitHub Check: build-windows-macos (macos-14, 25, doc)
  • GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
  • GitHub Check: build-windows-macos (macos-14, 25, unit)
  • GitHub Check: build-windows-macos (macos-14, 21, doc)
  • GitHub Check: security-it-windows-macos (macos-14, 21)
  • GitHub Check: security-it-windows-macos (macos-14, 25)
  • GitHub Check: security-it-windows-macos (windows-latest, 21)
  • GitHub Check: security-it-windows-macos (windows-latest, 25)
  • GitHub Check: test-sql-cli-integration (21)
  • GitHub Check: CodeQL-Scan (java)
  • GitHub Check: WhiteSource Security Check
🔇 Additional comments (1)
legacy/src/main/java/org/opensearch/sql/legacy/executor/format/SelectResultSet.java (1)

175-181: Verify that the mapping validation correctly handles index aliases and patterns that resolve to multiple underlying indices in JOIN queries.

The change from checking index name presence to requiring mappings.size() == 1 is a significant functional change. While the review correctly notes that loadFromEsState() is called per table in JOIN queries (which should isolate each table), the concern about index patterns and aliases resolving to multiple indices remains valid. Since aliases can point to one or more indices, you need to verify:

  1. How response.mappings() behaves when a user specifies a table name that is an alias pointing to multiple indices (does ES return separate entries for each underlying index or one consolidated entry?)
  2. Whether integration tests cover scenarios where table names resolve to multi-index aliases
  3. How the previous mapping resolution logic handled this case and why it was changed

If the mappings response consolidates multiple underlying indices into a single entry when queried by alias name, the change is safe. If it returns separate entries per underlying index, the validation will incorrectly reject valid multi-index alias queries.

@aparajita31pandey
Copy link
Contributor Author

@LantaoJin Can I please get a review ?

@Swiddis Swiddis added the bugFix label Dec 30, 2025
Swiddis
Swiddis previously approved these changes Dec 30, 2025
Copy link
Collaborator

@Swiddis Swiddis left a comment

Choose a reason for hiding this comment

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

Ideally this should have an integ test, but nothing wrong with the impl

Copy link
Contributor

@aalva500-prog aalva500-prog left a comment

Choose a reason for hiding this comment

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

LGTM, but agree with @Swiddis that some test would be ideal.

@Swiddis
Copy link
Collaborator

Swiddis commented Dec 31, 2025

Re: integ tests, I have #5008 which lays a lot more groundwork for adding more permissions-related tests to our codebase.

Since this PR was opened before that existed, I won't block on it -- after both PRs merge I'll write a small task for myself to add a test for this.

@aparajita31pandey
Copy link
Contributor Author

aparajita31pandey commented Jan 2, 2026

@Swiddis @aalva500-prog I have added a small integ test for this change. Can I get a re-review ?

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
integ-test/src/test/java/org/opensearch/sql/sql/PaginationIT.java (1)

254-280: Add cleanup for the alias to ensure test independence.

The test validates alias-index query parity effectively, but doesn't clean up the created alias. This could cause test failures on reruns or affect other tests.

🔎 Proposed fix to add alias cleanup

Add cleanup at the end of the test:

     assertEquals(directResponse.getInt("size"), aliasQueryResponse.getInt("size"));
     assertTrue(
         directResponse.getJSONArray("schema").similar(aliasQueryResponse.getJSONArray("schema")));
+
+    // Clean up alias
+    String deleteAliasQuery =
+        String.format(
+            "{ \"actions\": [ { \"remove\": { \"index\": \"%s\", \"alias\": \"%s\" } } ] }",
+            indexName, aliasName);
+    Request deleteAliasRequest = new Request("POST", "/_aliases");
+    deleteAliasRequest.setJsonEntity(deleteAliasQuery);
+    executeRequest(deleteAliasRequest);
   }

As per coding guidelines, integration tests should clean up resources after execution and be independent.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 66a475b and 5366552.

📒 Files selected for processing (2)
  • integ-test/src/test/java/org/opensearch/sql/sql/PaginationIT.java
  • legacy/src/main/java/org/opensearch/sql/legacy/executor/format/SelectResultSet.java
🧰 Additional context used
📓 Path-based instructions (4)
**/*.java

📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)

**/*.java: Use PascalCase for class names (e.g., QueryExecutor)
Use camelCase for method and variable names (e.g., executeQuery)
Use UPPER_SNAKE_CASE for constants (e.g., MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
Prefer Optional<T> for nullable returns in Java
Avoid unnecessary object creation in loops
Use StringBuilder for string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code

Files:

  • integ-test/src/test/java/org/opensearch/sql/sql/PaginationIT.java
  • legacy/src/main/java/org/opensearch/sql/legacy/executor/format/SelectResultSet.java

⚙️ CodeRabbit configuration file

**/*.java: - 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 usage instead of null returns
  • Validate proper use of try-with-resources for resource management

Files:

  • integ-test/src/test/java/org/opensearch/sql/sql/PaginationIT.java
  • legacy/src/main/java/org/opensearch/sql/legacy/executor/format/SelectResultSet.java
integ-test/**/*IT.java

📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)

End-to-end scenarios need integration tests in integ-test/ module

Files:

  • integ-test/src/test/java/org/opensearch/sql/sql/PaginationIT.java

⚙️ CodeRabbit configuration file

integ-test/**/*IT.java: - Integration tests MUST use valid test data from resources

  • Verify test data files exist in integ-test/src/test/resources/
  • 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

Files:

  • integ-test/src/test/java/org/opensearch/sql/sql/PaginationIT.java
**/*IT.java

📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)

Name integration tests with *IT.java suffix in OpenSearch SQL

Files:

  • integ-test/src/test/java/org/opensearch/sql/sql/PaginationIT.java
**/test/**/*.java

⚙️ CodeRabbit configuration file

**/test/**/*.java: - 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
  • 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

Files:

  • integ-test/src/test/java/org/opensearch/sql/sql/PaginationIT.java
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL

Applied to files:

  • integ-test/src/test/java/org/opensearch/sql/sql/PaginationIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes

Applied to files:

  • integ-test/src/test/java/org/opensearch/sql/sql/PaginationIT.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
  • GitHub Check: build-linux (25, doc)
  • GitHub Check: build-linux (25, unit)
  • GitHub Check: build-linux (25, integration)
  • GitHub Check: build-linux (21, unit)
  • GitHub Check: build-linux (21, integration)
  • GitHub Check: build-linux (21, doc)
  • GitHub Check: bwc-tests-rolling-upgrade (25)
  • GitHub Check: bwc-tests-rolling-upgrade (21)
  • GitHub Check: bwc-tests-full-restart (21)
  • GitHub Check: bwc-tests-full-restart (25)
  • GitHub Check: security-it-linux (21)
  • GitHub Check: security-it-linux (25)
  • GitHub Check: build-windows-macos (macos-14, 25, unit)
  • GitHub Check: build-windows-macos (macos-14, 21, integration)
  • GitHub Check: build-windows-macos (macos-14, 25, integration)
  • GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
  • GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
  • GitHub Check: build-windows-macos (macos-14, 25, doc)
  • GitHub Check: build-windows-macos (macos-14, 21, unit)
  • GitHub Check: build-windows-macos (macos-14, 21, doc)
  • GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
  • GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
  • GitHub Check: CodeQL-Scan (java)
  • GitHub Check: security-it-windows-macos (windows-latest, 21)
  • GitHub Check: security-it-windows-macos (macos-14, 25)
  • GitHub Check: security-it-windows-macos (macos-14, 21)
  • GitHub Check: security-it-windows-macos (windows-latest, 25)
  • GitHub Check: test-sql-cli-integration (21)
🔇 Additional comments (1)
legacy/src/main/java/org/opensearch/sql/legacy/executor/format/SelectResultSet.java (1)

176-182: LGTM! Alias resolution removal implemented correctly.

The changes properly enforce the single-mapping requirement and retrieve the mapping directly. The error message correctly uses indexName instead of query.getFrom(), resolving the issue flagged in the previous review.

This aligns with the PR objectives to remove redundant alias resolution and rely on GetFieldMapping for both indices and aliases.

Swiddis
Swiddis previously approved these changes Jan 5, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 Fix all issues with AI agents
In @integ-test/src/test/java/org/opensearch/sql/sql/PaginationIT.java:
- Around line 255-289: The test currently creates aliasName that maps to two
indices (Index.DOG and Index.DOGS2) but then compares a direct single-index
query (directQuery using TEST_INDEX_DOG) to the alias query, which is incorrect;
update the test so the alias and direct query target the same set of
indices—either change createAliasQuery to add only Index.DOG (remove
Index.DOGS2) so aliasName maps to a single index, or change directQuery to query
both indices (use the same index list as the alias) before asserting sizes;
adjust the createAliasQuery, directQuery, and/or aliasQuery variables
accordingly to ensure both queries are equivalent.

In
@legacy/src/main/java/org/opensearch/sql/legacy/executor/format/SelectResultSet.java:
- Line 178: The code in SelectResultSet constructs an error message with a
redundant nested String.format call (String.format(String.format("Index type %s
does not exist", query.getFrom())));—remove the outer String.format so the
message is built once, e.g. use String.format("Index type %s does not exist",
query.getFrom()) or simply concatenate, and update the throw/log site that
references this message accordingly.
- Around line 176-180: In SelectResultSet, the exception message construction
uses a nested String.format which will fail at runtime; replace
String.format(String.format("Index type %s does not exist", query.getFrom()))
with a single-format invocation so the message is built as String.format("Index
type %s does not exist", query.getFrom()); locate this in the block that checks
mappings.isEmpty() (around the mappings / query.getFrom() usage) and update the
throw new IllegalArgumentException to use the single String.format call.
🧹 Nitpick comments (1)
integ-test/src/test/java/org/opensearch/sql/sql/PaginationIT.java (1)

9-9: Consider using explicit imports instead of wildcard.

Wildcard imports can reduce code clarity and may introduce namespace pollution. Consider importing only the specific constants used in this test (e.g., TEST_INDEX_CALCS, TEST_INDEX_ONLINE, TEST_INDEX_DOG).

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7978cb7 and 3558324.

📒 Files selected for processing (2)
  • integ-test/src/test/java/org/opensearch/sql/sql/PaginationIT.java
  • legacy/src/main/java/org/opensearch/sql/legacy/executor/format/SelectResultSet.java
🧰 Additional context used
📓 Path-based instructions (4)
**/*.java

📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)

**/*.java: Use PascalCase for class names (e.g., QueryExecutor)
Use camelCase for method and variable names (e.g., executeQuery)
Use UPPER_SNAKE_CASE for constants (e.g., MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
Prefer Optional<T> for nullable returns in Java
Avoid unnecessary object creation in loops
Use StringBuilder for string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code

Files:

  • legacy/src/main/java/org/opensearch/sql/legacy/executor/format/SelectResultSet.java
  • integ-test/src/test/java/org/opensearch/sql/sql/PaginationIT.java

⚙️ CodeRabbit configuration file

**/*.java: - 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 usage instead of null returns
  • Validate proper use of try-with-resources for resource management

Files:

  • legacy/src/main/java/org/opensearch/sql/legacy/executor/format/SelectResultSet.java
  • integ-test/src/test/java/org/opensearch/sql/sql/PaginationIT.java
integ-test/**/*IT.java

📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)

End-to-end scenarios need integration tests in integ-test/ module

Files:

  • integ-test/src/test/java/org/opensearch/sql/sql/PaginationIT.java

⚙️ CodeRabbit configuration file

integ-test/**/*IT.java: - Integration tests MUST use valid test data from resources

  • Verify test data files exist in integ-test/src/test/resources/
  • 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

Files:

  • integ-test/src/test/java/org/opensearch/sql/sql/PaginationIT.java
**/*IT.java

📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)

Name integration tests with *IT.java suffix in OpenSearch SQL

Files:

  • integ-test/src/test/java/org/opensearch/sql/sql/PaginationIT.java
**/test/**/*.java

⚙️ CodeRabbit configuration file

**/test/**/*.java: - 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
  • 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

Files:

  • integ-test/src/test/java/org/opensearch/sql/sql/PaginationIT.java
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL

Applied to files:

  • integ-test/src/test/java/org/opensearch/sql/sql/PaginationIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*Test.java : Name unit tests with `*Test.java` suffix in OpenSearch SQL

Applied to files:

  • integ-test/src/test/java/org/opensearch/sql/sql/PaginationIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes

Applied to files:

  • integ-test/src/test/java/org/opensearch/sql/sql/PaginationIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Use `./gradlew :integ-test:integTest` for integration testing in OpenSearch SQL

Applied to files:

  • integ-test/src/test/java/org/opensearch/sql/sql/PaginationIT.java
🧬 Code graph analysis (1)
integ-test/src/test/java/org/opensearch/sql/sql/PaginationIT.java (1)
integ-test/src/test/java/org/opensearch/sql/sql/IdentifierIT.java (1)
  • Index (218-262)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
  • GitHub Check: security-it-linux (25)
  • GitHub Check: security-it-linux (21)
  • GitHub Check: build-linux (25, integration)
  • GitHub Check: build-linux (21, unit)
  • GitHub Check: build-linux (21, integration)
  • GitHub Check: build-linux (25, doc)
  • GitHub Check: build-linux (25, unit)
  • GitHub Check: build-linux (21, doc)
  • GitHub Check: bwc-tests-full-restart (21)
  • GitHub Check: bwc-tests-full-restart (25)
  • GitHub Check: bwc-tests-rolling-upgrade (21)
  • GitHub Check: bwc-tests-rolling-upgrade (25)
  • GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
  • GitHub Check: build-windows-macos (macos-14, 21, unit)
  • GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
  • GitHub Check: build-windows-macos (macos-14, 21, doc)
  • GitHub Check: build-windows-macos (macos-14, 21, integration)
  • GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
  • GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
  • GitHub Check: build-windows-macos (macos-14, 25, unit)
  • GitHub Check: build-windows-macos (macos-14, 25, integration)
  • GitHub Check: build-windows-macos (macos-14, 25, doc)
  • GitHub Check: security-it-windows-macos (windows-latest, 25)
  • GitHub Check: security-it-windows-macos (macos-14, 25)
  • GitHub Check: security-it-windows-macos (macos-14, 21)
  • GitHub Check: security-it-windows-macos (windows-latest, 21)
  • GitHub Check: CodeQL-Scan (java)
  • GitHub Check: test-sql-cli-integration (21)

Signed-off-by: Aparajita Pandey <[email protected]>
Signed-off-by: Aparajita Pandey <[email protected]>
Copy link
Collaborator

@dai-chen dai-chen left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@Swiddis Swiddis merged commit 74b2fb3 into opensearch-project:main Jan 9, 2026
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants