Merge remote-tracking branch 'upstream/main' into issues/4836

Signed-off-by: Lantao Jin <ltjin@amazon.com>

Author: LantaoJin

.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: false # Disabled auto-review until it becomes stable
+    auto_incremental_review: false
+    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<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
+
+# 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"

.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<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?
+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

async-query/src/main/java/org/opensearch/sql/spark/scheduler/job/ScheduledAsyncQueryJobRunner.java

@@ -12,7 +12,6 @@
 import org.opensearch.jobscheduler.spi.ScheduledJobParameter;
 import org.opensearch.jobscheduler.spi.ScheduledJobRunner;
 import org.opensearch.plugins.Plugin;
-import org.opensearch.sql.legacy.executor.AsyncRestExecutor;
 import org.opensearch.sql.spark.asyncquery.AsyncQueryExecutorService;
 import org.opensearch.sql.spark.asyncquery.model.NullAsyncQueryRequestContext;
 import org.opensearch.sql.spark.rest.model.CreateAsyncQueryRequest;
@@ -21,6 +20,8 @@
 import org.opensearch.threadpool.ThreadPool;
 import org.opensearch.transport.client.Client;
 
+import static org.opensearch.sql.opensearch.executor.OpenSearchQueryManager.SQL_WORKER_THREAD_POOL_NAME;
+
 /**
  * The job runner class for scheduling async query.
  *
@@ -37,7 +38,7 @@
 public class ScheduledAsyncQueryJobRunner implements ScheduledJobRunner {
   // Share SQL plugin thread pool
   private static final String ASYNC_QUERY_THREAD_POOL_NAME =
-      AsyncRestExecutor.SQL_WORKER_THREAD_POOL_NAME;
+      SQL_WORKER_THREAD_POOL_NAME;
   private static final Logger LOGGER = LogManager.getLogger(ScheduledAsyncQueryJobRunner.class);
 
   private static final ScheduledAsyncQueryJobRunner INSTANCE = new ScheduledAsyncQueryJobRunner();

async-query/src/test/java/org/opensearch/sql/spark/scheduler/job/ScheduledAsyncQueryJobRunnerTest.java

@@ -15,6 +15,7 @@
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
+import static org.opensearch.sql.opensearch.executor.OpenSearchQueryManager.SQL_WORKER_THREAD_POOL_NAME;
 
 import java.time.Instant;
 import org.apache.logging.log4j.LogManager;
@@ -87,7 +88,7 @@ public void testRunJobWithCorrectParameter() {
     spyJobRunner.runJob(request, context);
 
     ArgumentCaptor<Runnable> captor = ArgumentCaptor.forClass(Runnable.class);
-    verify(threadPool.executor(AsyncRestExecutor.SQL_WORKER_THREAD_POOL_NAME))
+    verify(threadPool.executor(SQL_WORKER_THREAD_POOL_NAME))
         .submit(captor.capture());
 
     Runnable runnable = captor.getValue();
@@ -145,7 +146,7 @@ public void testDoRefreshThrowsException() {
     spyJobRunner.runJob(request, context);
 
     ArgumentCaptor<Runnable> captor = ArgumentCaptor.forClass(Runnable.class);
-    verify(threadPool.executor(AsyncRestExecutor.SQL_WORKER_THREAD_POOL_NAME))
+    verify(threadPool.executor(SQL_WORKER_THREAD_POOL_NAME))
         .submit(captor.capture());
 
     Runnable runnable = captor.getValue();

common/src/main/java/org/opensearch/sql/common/setting/Settings.java

@@ -25,6 +25,7 @@ public enum Key {
 
     /** PPL Settings. */
     PPL_ENABLED("plugins.ppl.enabled"),
+    PPL_QUERY_TIMEOUT("plugins.ppl.query.timeout"),
     PATTERN_METHOD("plugins.ppl.pattern.method"),
     PATTERN_MODE("plugins.ppl.pattern.mode"),
     PATTERN_MAX_SAMPLE_COUNT("plugins.ppl.pattern.max.sample.count"),

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java

@@ -143,6 +143,7 @@
 import org.opensearch.sql.ast.tree.UnresolvedPlan;
 import org.opensearch.sql.ast.tree.Values;
 import org.opensearch.sql.ast.tree.Window;
+import org.opensearch.sql.calcite.plan.AliasFieldsWrappable;
 import org.opensearch.sql.calcite.plan.LogicalSystemLimit;
 import org.opensearch.sql.calcite.plan.LogicalSystemLimit.SystemLimitType;
 import org.opensearch.sql.calcite.plan.OpenSearchConstants;
@@ -196,7 +197,11 @@ public RelNode visitRelation(Relation node, CalcitePlanContext context) {
       throw new CalciteUnsupportedException("information_schema is unsupported in Calcite");
     }
     context.relBuilder.scan(node.getTableQualifiedName().getParts());
-    return context.relBuilder.peek();
+    RelNode scan = context.relBuilder.peek();
+    if (scan instanceof AliasFieldsWrappable) {
+      return ((AliasFieldsWrappable) scan).wrapProjectForAliasFields(context.relBuilder);
+    }
+    return scan;
   }
 
   // This is a tool method to add an existed RelOptTable to builder stack, not used for now

core/src/main/java/org/opensearch/sql/calcite/plan/AliasFieldsWrappable.java

@@ -0,0 +1,36 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package org.opensearch.sql.calcite.plan;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.Set;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.tools.RelBuilder;
+
+/**
+ * Wrapper for TableScan to add alias fields by creating another project with alias upon on it. This
+ * allows TableScan or Table to emit alias type fields in its schema, while it still supports
+ * resolving these fields used in the query.
+ */
+public interface AliasFieldsWrappable {
+
+  Map<String, String> getAliasMapping();
+
+  default RelNode wrapProjectForAliasFields(RelBuilder relBuilder) {
+    assert relBuilder.peek() instanceof AliasFieldsWrappable
+        : "The top node in RelBuilder must be AliasFieldsWrappable";
+    Set<Entry<String, String>> aliasFieldsSet = this.getAliasMapping().entrySet();
+    // Adding alias referring to the original field.
+    List<RexNode> aliasFieldsNew =
+        aliasFieldsSet.stream()
+            .map(entry -> relBuilder.alias(relBuilder.field(entry.getValue()), entry.getKey()))
+            .toList();
+    return relBuilder.projectPlus(aliasFieldsNew).peek();
+  }
+}

core/src/main/java/org/opensearch/sql/calcite/type/ExprJavaType.java

@@ -14,7 +14,9 @@
 
 /**
  * The JavaType for ExprUDT. The UDT which needs to use self-implemented java class should extend
- * this.
+ * this. Its javaClazz should override equals() and hashCode() methods. For example, {@link
+ * org.opensearch.sql.data.model.ExprIpValue} (javaClazz of {@link ExprIPType}) overrides the
+ * equals() and hashCode().
  */
 public class ExprJavaType extends AbstractExprRelDataType<JavaType> {
   public ExprJavaType(OpenSearchTypeFactory typeFactory, ExprUDT exprUDT, Class<?> javaClazz) {

core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactory.java

@@ -313,6 +313,8 @@ public static RelDataType convertSchema(Table table) {
     Map<String, ExprType> fieldTypes = new LinkedHashMap<>(table.getFieldTypes());
     fieldTypes.putAll(table.getReservedFieldTypes());
     for (Entry<String, ExprType> entry : fieldTypes.entrySet()) {
+      // skip alias type fields when constructing schema
+      if (entry.getValue().getOriginalPath().isPresent()) continue;
       fieldNameList.add(entry.getKey());
       typeList.add(OpenSearchTypeFactory.convertExprTypeToRelDataType(entry.getValue()));
     }

core/src/main/java/org/opensearch/sql/data/model/ExprIpValue.java

@@ -6,11 +6,13 @@
 package org.opensearch.sql.data.model;
 
 import inet.ipaddr.IPAddress;
+import lombok.EqualsAndHashCode;
 import org.opensearch.sql.data.type.ExprCoreType;
 import org.opensearch.sql.data.type.ExprType;
 import org.opensearch.sql.utils.IPUtils;
 
 /** Expression IP Address Value. */
+@EqualsAndHashCode(callSuper = false)
 public class ExprIpValue extends AbstractExprValue {
   private final IPAddress value;