Support read multi-values from OpenSearch if no codegen triggered #5015
Conversation
Signed-off-by: Lantao Jin <[email protected]>
📝 WalkthroughSummary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplaces JDBC-based type conversion with accessor-based retrieval and a recursive value processor; adds early ExprValue passthrough; removes centralized converter; updates tests and integration YAML to validate array, nested, and geo-point handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Query as Query Execution
participant Engine as OpenSearchExecutionEngine
participant RS as JDBC ResultSet
participant Acc as Accessor
participant PV as processValue()
participant Expr as ExprValueUtils
Query->>Engine: buildResultSet(rs)
Engine->>RS: getAccessors(colIndex)
RS-->>Acc: Accessor for column
loop per column
Engine->>Acc: get(rs)
Acc-->>Engine: rawValue (Object)
Engine->>PV: processValue(rawValue)
alt Point
PV->>PV: convert Point → OpenSearchExprGeoPointValue
else Map
PV->>PV: recursively process map values
else List
PV->>PV: recursively process list elements
else other
PV-->>Engine: rawValue
end
PV-->>Engine: processedValue
Engine->>Expr: fromObjectValue(processedValue)
Expr-->>Engine: ExprValue
Engine->>Engine: append to row
end
Engine-->>Query: ResultSet with ExprValues
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
Comment |
Signed-off-by: Lantao Jin <[email protected]>
LantaoJin
requested review from
GumpacG,
MaxKsyunz,
RyanL1997,
Swiddis,
YANG-DB,
Yury-Fridlyand,
acarbonetto,
anirudha,
dai-chen,
derek-ho,
forestmvey,
joshuali925,
kavithacm,
mengweieric,
noCharger,
penghuo,
ps48,
qianheng-aws,
seankao-az,
vamsimanohar,
ykmr1224 and
yuancu
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In
@opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java:
- Around line 221-242: The getAccessors method uses reflection to call the
private AvaticaResultSet.getAccessor(int) because the current Calcite/Avatica
public API does not expose a way to obtain Cursor.Accessor instances; document
that this is intentionally tied to Calcite version 1.41.0 (the version validated
during development), explain that public APIs in Avatica/Calcite were
insufficient to access per-column accessors which necessitated reflection, and
add a TODO noting to remove this reflective approach and switch to the public
API once Calcite/Avatica exposes an accessor retrieval method or an upgrade to a
newer Calcite version removes the need for reflection.
🧹 Nitpick comments (2)
opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java (2)
285-297: Refactored value processing aligns with PR objectives.The shift from JDBC-based type conversion to accessor-based retrieval with recursive value processing correctly enables multi-value handling. The flow is clean:
- Get accessors via reflection (one-time per result set)
- Extract raw values via
accessor.getObject()- Process complex types (geo, maps, lists) recursively
- Wrap in
ExprValueviaExprValueUtilsThis approach properly preserves array structures as required by issue #4173.
Consider: Exception handling for accessor failures
While
buildResultSetthrowsSQLException, consider whetheraccessor.getObject()can fail in ways that deserve specific error messages for debugging.
244-274: Add recursion depth protection to prevent stack overflow on deeply nested structures.The
processValuemethod recursively processes nested maps and lists without depth limiting. While OpenSearch document size constraints provide practical limits, defensive protection against maliciously crafted deeply-nested input is warranted. Consider adding a maximum nesting depth (e.g., 100 levels) with appropriate error handling.🔎 Suggested enhancement for recursion safety
- private static Object processValue(Object value) { + private static final int MAX_RECURSION_DEPTH = 100; + + private static Object processValue(Object value) { + return processValue(value, 0); + } + + private static Object processValue(Object value, int depth) { + if (depth > MAX_RECURSION_DEPTH) { + throw new IllegalStateException("Maximum nesting depth exceeded"); + } if (value == null) { return null; } // ... Point handling ... if (value instanceof Map) { Map<String, Object> map = (Map<String, Object>) value; Map<String, Object> convertedMap = new HashMap<>(); for (Map.Entry<String, Object> entry : map.entrySet()) { - convertedMap.put(entry.getKey(), processValue(entry.getValue())); + convertedMap.put(entry.getKey(), processValue(entry.getValue(), depth + 1)); } return convertedMap; } if (value instanceof List) { List<Object> list = (List<Object>) value; List<Object> convertedList = new ArrayList<>(); for (Object item : list) { - convertedList.add(processValue(item)); + convertedList.add(processValue(item, depth + 1)); } return convertedList; } return value; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/src/main/java/org/opensearch/sql/data/model/ExprValueUtils.javacore/src/test/java/org/opensearch/sql/data/model/ExprValueUtilsTest.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLBasicIT.javainteg-test/src/yamlRestTest/resources/rest-api-spec/test/issues/4173.ymlopensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.javaopensearch/src/main/java/org/opensearch/sql/opensearch/util/JdbcOpenSearchDataTypeConvertor.java
💤 Files with no reviewable changes (1)
- opensearch/src/main/java/org/opensearch/sql/opensearch/util/JdbcOpenSearchDataTypeConvertor.java
🧰 Additional context used
📓 Path-based instructions (7)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor 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
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor 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:
opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLBasicIT.javacore/src/test/java/org/opensearch/sql/data/model/ExprValueUtilsTest.javacore/src/main/java/org/opensearch/sql/data/model/ExprValueUtils.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:
opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLBasicIT.javacore/src/test/java/org/opensearch/sql/data/model/ExprValueUtilsTest.javacore/src/main/java/org/opensearch/sql/data/model/ExprValueUtils.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/calcite/remote/CalcitePPLBasicIT.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/calcite/remote/CalcitePPLBasicIT.java
**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
Name integration tests with
*IT.javasuffix in OpenSearch SQL
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLBasicIT.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/calcite/remote/CalcitePPLBasicIT.javacore/src/test/java/org/opensearch/sql/data/model/ExprValueUtilsTest.java
**/calcite/**/*.java
⚙️ CodeRabbit configuration file
**/calcite/**/*.java: - Follow existing Calcite integration patterns
- Verify RelNode visitor implementations are complete
- Check RexNode handling follows project conventions
- Validate SQL generation is correct and optimized
- Ensure Calcite version compatibility
- Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor
- Document any Calcite-specific workarounds
- Test compatibility with Calcite version constraints
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLBasicIT.java
**/*Test.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*Test.java: All new business logic requires unit tests
Name unit tests with*Test.javasuffix in OpenSearch SQL
Files:
core/src/test/java/org/opensearch/sql/data/model/ExprValueUtilsTest.java
core/src/main/java/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/**/*.java: - New functions MUST have unit tests in the same commit
Files:
core/src/main/java/org/opensearch/sql/data/model/ExprValueUtils.java
🧠 Learnings (3)
📚 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/calcite/remote/CalcitePPLBasicIT.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 **/*.java : Document Calcite-specific workarounds in code
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLBasicIT.java
📚 Learning: 2025-12-29T05:32:03.491Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:03.491Z
Learning: For any custom Calcite RelNode class (e.g., ones that extend EnumerableLimitSort or other Calcite RelNode types), always override the copy method. If copy is not overridden, cloning/copy operations may downgrade the instance to the parent class type, losing the custom behavior. In your implementation, ensure copy returns a new instance of the concrete class with all relevant fields and traits preserved, mirroring the current instance state.
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLBasicIT.java
🧬 Code graph analysis (1)
opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java (2)
legacy/src/main/java/org/opensearch/sql/legacy/spatial/Point.java (1)
Point(9-25)opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprGeoPointValue.java (1)
OpenSearchExprGeoPointValue(22-71)
⏰ 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). (2)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
🔇 Additional comments (4)
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLBasicIT.java (1)
603-604: LGTM! Correct floating-point precision adjustment.The updated expected value accurately reflects IEEE 754 floating-point arithmetic behavior. The expression
0.06f - 0.01fproduces0.049999999999999996due to binary representation limitations, not the decimal0.05.integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/4173.yml (1)
1-70: Excellent integration test coverage for array-valued fields.This test effectively validates the PR's core objective of handling multi-valued fields under Calcite. The test covers:
- Multiple array types (text, keyword, numeric, boolean, date)
- Nested objects with array fields
- Mixed single-value and array-value documents
- Proper schema detection and data structure preservation
The test data is realistic and assertions are comprehensive.
core/src/main/java/org/opensearch/sql/data/model/ExprValueUtils.java (1)
119-121: LGTM! Defensive programming improvement.The early return for
ExprValueinstances prevents unnecessary double-wrapping and aligns with the refactored value processing inOpenSearchExecutionEngine. This is a clean, efficient guard clause.core/src/test/java/org/opensearch/sql/data/model/ExprValueUtilsTest.java (1)
221-222: LGTM! Test updated to match new behavior.The test correctly reflects the updated
fromObjectValuelogic. SinceExprValueinstances now short-circuit with an early return, the test appropriately uses a genericObjectto validate the unsupported type exception path.
opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java
Outdated
Show resolved
Hide resolved
qianheng-aws
left a comment
qianheng-aws
left a comment
There was a problem hiding this comment.
[question] What if there is codegen for the final plan?
LantaoJin
changed the title
Updated the description. This solution only resolve the failures when there is no codegen triggered. |
| ExprValue exprValue = | ||
| JdbcOpenSearchDataTypeConvertor.getExprValueFromSqlType( | ||
| resultSet, i, sqlType, fieldType, columnName); | ||
| Object value = accessorList.get(i - 1).getObject(); |
There was a problem hiding this comment.
resultSet.getObject(columnName) works. reflection is not necessary?
Signed-off-by: Lantao Jin <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/4173.yml:
- Around line 66-69: The test's query string (query: 'source=array-test ') must
include a deterministic ORDER BY clause to avoid flaky results; update the query
to add an ORDER BY (e.g., ORDER BY num or another stable column) and then adjust
the expected match for "datarows" so the row order matches the sorted results
(the current datarows assertion on the match line must be reordered to reflect
the chosen ORDER BY).
In
@opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java:
- Around line 218-248: The method processValue has unchecked casts for Map and
List that can throw ClassCastException; replace the raw casts with safe
instanceof checks and use Map<?,?> and List<?> when iterating—e.g., in
processValue, change the Map branch to if (value instanceof Map<?,?>) { for
(Map.Entry<?,?> e : ((Map<?,?>) value).entrySet()) { String key =
String.valueOf(e.getKey()); convertedMap.put(key, processValue(e.getValue())); }
} and the List branch to if (value instanceof List<?>) { for (Object item :
(List<?>) value) { convertedList.add(processValue(item)); } } while keeping the
Point -> OpenSearchExprGeoPointValue conversion and overall return type Object
to avoid unsafe casts.
🧹 Nitpick comments (1)
integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/4173.yml (1)
66-66: Remove trailing space from query string.The query string has a trailing space which appears unintentional.
🔎 Suggested cleanup
- query: 'source=array-test ' + query: 'source=array-test'
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/4173.ymlopensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor 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
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor 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:
opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.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:
opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java
🧠 Learnings (2)
📓 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: Test SQL generation and optimization paths for Calcite integration changes
📚 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 **/*.java : Document Calcite-specific workarounds in code
Applied to files:
opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java
🧬 Code graph analysis (1)
opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java (3)
legacy/src/main/java/org/opensearch/sql/legacy/spatial/Point.java (1)
Point(9-25)legacy/src/main/java/org/opensearch/sql/legacy/expression/model/ExprValueUtils.java (1)
ExprValueUtils(18-75)opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprGeoPointValue.java (1)
OpenSearchExprGeoPointValue(22-71)
⏰ 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). (56)
- GitHub Check: security-it-linux (21)
- GitHub Check: security-it-linux (25)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (25, integration)
- GitHub Check: build-linux (21, unit)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (25, doc)
- GitHub Check: build-linux (25, unit)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: build-linux (25, doc)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (25, integration)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (21, unit)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: security-it-linux (25)
- GitHub Check: security-it-linux (21)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: test-sql-cli-integration (21)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: test-sql-cli-integration (21)
🔇 Additional comments (2)
opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java (1)
268-270: Excellent simplification that addresses PR objectives.This three-line change successfully implements the core fix:
- Retrieves raw values (including Lists for multi-valued fields) via
getObject- Processes them recursively to handle geo points and nested structures
- Converts to
ExprValue(Lists → ExprCollectionValue per the PR description)This approach:
- ✓ Aligns with PR objective to restore v2-like behavior for multi-valued fields
- ✓ Applies only when no codegen is triggered (Calcite path via
buildResultSet)- ✓ Addresses penghuo's previous feedback to use
resultSet.getObject(columnName)- ✓ Eliminates the reflection-based approach from earlier iterations
integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/4173.yml (1)
61-61: Verify whether the _id fielddata warning is actually emitted by this PPL query.The test allows a deprecation warning about loading fielddata on
_id, but the querysource=array-testdoesn't explicitly sort or aggregate on_id. This warning may be triggered internally by the PPL query executor, but static analysis cannot confirm this. Consider running the test to determine if this warning is genuinely emitted or if the allowed_warnings entry can be safely removed.
integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/4173.yml
Show resolved
Hide resolved
opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java
Show resolved
Hide resolved
LantaoJin
requested review from
penghuo and
qianheng-aws
and removed request for
qianheng-aws
|
The backport to To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/sql/backport-2.19-dev 2.19-dev
# Navigate to the new working tree
pushd ../.worktrees/sql/backport-2.19-dev
# Create a new branch
git switch --create backport/backport-5015-to-2.19-dev
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 98516d6e17ad2281f173a7bb0f2d5d1d9232b1ee
# Push it to GitHub
git push --set-upstream origin backport/backport-5015-to-2.19-dev
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/sql/backport-2.19-devThen, create a pull request where the |
…ensearch-project#5015) * Support read multi-values from OpenSearch Signed-off-by: Lantao Jin <[email protected]> * Fix tests Signed-off-by: Lantao Jin <[email protected]> * remove reflection way Signed-off-by: Lantao Jin <[email protected]> --------- Signed-off-by: Lantao Jin <[email protected]> (cherry picked from commit 98516d6)
) (#5023) * Support read multi-values from OpenSearch * Fix tests * remove reflection way --------- (cherry picked from commit 98516d6) Signed-off-by: Lantao Jin <[email protected]>
4 participants
Description
PPL with Calcite enabling cannot handle the fields with multi-values. The issue was described in #4173.
#4909 (closed) provided a mitigation that handle those array columns by picking the first value instead of failure as a short-term solution.
This PR provides another solution to align with the behavior of v2 if a PPL query is fully pushdown (no codegen triggered). It could resolve the issue of PPL failures in Discover UI.
Related Issues
Resolves #4173
Check List
--signoffor-s.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.