Refactor alias type field by adding another project with alias #4881
Conversation
Signed-off-by: Heng Qian <[email protected]>
qianheng-aws
requested review from
GumpacG,
LantaoJin,
MaxKsyunz,
RyanL1997,
Swiddis,
YANG-DB,
Yury-Fridlyand,
acarbonetto,
anirudha,
dai-chen,
derek-ho,
forestmvey,
joshuali925,
kavithacm,
mengweieric,
noCharger,
penghuo,
ps48,
seankao-az,
vamsimanohar,
ykmr1224 and
yuancu
as code owners
WalkthroughRefactors alias type field handling by introducing an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
qianheng-aws
added
enhancement
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @qianheng-aws. * #4881 (comment) The following files were modified: * `core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java` * `core/src/main/java/org/opensearch/sql/calcite/plan/AliasFieldsWrappable.java` * `core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactory.java` * `integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java` * `opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/AbstractCalciteIndexScan.java` * `opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteEnumerableIndexScan.java` * `opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
integ-test/src/test/resources/expectedOutput/calcite/explain_alias_type_field.yaml (1)
8-9: Consider improving readability of the physical plan line using YAML multiline syntax.Line 9 is extremely long (exceeds 1000 characters) and difficult to review. While YAML snapshot tests often have dense content, consider using YAML's multiline scalar syntax for improved readability during code reviews and maintenance.
As an optional improvement for future readability, the physical plan could be reformatted using YAML's
|-multiline scalar indicator:physical: |- CalciteEnumerableIndexScan( table=[[OpenSearch, opensearch-sql_test_index_alias]], PushDownContext=[[ FILTER->>($0, 10), AGGREGATION->rel#:LogicalAggregate.NONE.[](input=..., avg(alias_col)=AVG($0)), LIMIT->10000 ], OpenSearchRequestBuilder(...) ]Note: Only apply if your snapshot testing framework supports multiline comparison without requiring exact whitespace matching.
core/src/main/java/org/opensearch/sql/calcite/plan/AliasFieldsWrappable.java (1)
16-35: Alias projection helper is sound; consider minor robustness tweaksCentralizing alias-field projection in
wrapProjectForAliasFields(RelBuilder)is a good fit for the new model and keeps scan/pushdown logic clean.Two optional refinements to make this more robust and avoid unnecessary nodes:
- Short‑circuit when there are no alias mappings to avoid constructing an empty
projectPlus:Map<String, String> mapping = getAliasMapping(); if (mapping.isEmpty()) { return relBuilder.peek(); }- Drive the mapping off the node on top of the builder stack rather than
this, so future implementers aren’t required to call the method only on the same instance asrelBuilder.peek():AliasFieldsWrappable top = (AliasFieldsWrappable) relBuilder.peek(); Set<Entry<String, String>> aliasFieldsSet = top.getAliasMapping().entrySet();Current usage is safe, so these are nice-to-have hardening steps rather than required fixes.
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java (1)
146-205: Relation visitation correctly hooks in alias-field projectionThe updated
visitRelation:
- Scans the table as before.
- If the resulting
RelNodeimplementsAliasFieldsWrappable(e.g.,AbstractCalciteIndexScan), immediately wraps it with the alias‑projection viawrapProjectForAliasFields.This is exactly where the alias projection belongs in the Calcite pipeline and ensures later phases see alias columns as ordinary projected fields rather than physical schema columns.
As a small optional optimization, you could avoid building an extra
Projectwhen there are no alias mappings:RelNode scan = context.relBuilder.peek(); if (scan instanceof AliasFieldsWrappable aliasScan && !aliasScan.getAliasMapping().isEmpty()) { return aliasScan.wrapProjectForAliasFields(context.relBuilder); } return scan;Functionally, the current implementation is fine.
opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/AbstractCalciteIndexScan.java (1)
11-15: Base scan’s AliasFieldsWrappable implementation is minimal and well-placedHaving
AbstractCalciteIndexScanimplementAliasFieldsWrappableand delegate@Override public Map<String, String> getAliasMapping() { return osIndex.getAliasMapping(); }to
OpenSearchIndexcleanly centralizes alias metadata at the storage layer while letting the planning layer (viaAliasFieldsWrappable.wrapProjectForAliasFields) build the corresponding projections. For indices without alias-type fields an empty map will result in at most an identity project, so this is safe across the board.If you later add more consumers of alias metadata, consider adding brief Javadoc on
getAliasMapping()’s contract (e.g., “key = alias name, value = original field path”) to prevent accidental inversion of the mapping.Also applies to: 47-52, 64-91, 257-260
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java(2 hunks)core/src/main/java/org/opensearch/sql/calcite/plan/AliasFieldsWrappable.java(1 hunks)core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactory.java(1 hunks)integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java(3 hunks)integ-test/src/test/resources/expectedOutput/calcite/explain_alias_type_field.yaml(1 hunks)integ-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_alias_type_field.yaml(1 hunks)opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/AbstractCalciteIndexScan.java(4 hunks)opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteEnumerableIndexScan.java(1 hunks)opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java (1)
integ-test/src/test/java/org/opensearch/sql/legacy/TestsConstants.java (1)
TestsConstants(9-101)
⏰ 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 (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 (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 (25)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (macos-14, 21, 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 (macos-14, 25, unit)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- 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: security-it-windows-macos (windows-latest, 25)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: WhiteSource Security Check
🔇 Additional comments (7)
integ-test/src/test/resources/expectedOutput/calcite/explain_alias_type_field.yaml (2)
2-7: Logical plan correctly demonstrates alias field introduction via Project operator.The LogicalProject operator at line 6 (
LogicalProject(alias_col=[$0])) is properly positioned before filters and aggregations, aligning with the refactoring objective to move alias resolution logic from Scan to a dedicated Project operator. The field reference$0flows through the filter and aggregation correctly.
8-9: Manual verification required for test integration points.The physical plan correctly maps the alias field to the original field in pushdown operations (
range query on "original_col"andavgaggregation on"original_col"). However, the following integration points require verification that could not be completed:
- Confirm that the test index
opensearch-sql_test_index_aliasexists in test setup and has the correct alias field mappings.- Verify that the
testAliasTypeField()method inCalciteExplainITgenerates this exact output.- Ensure the test file is loaded and assertions are properly configured.
core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactory.java (1)
310-321: Skipping alias-type fields in schema aligns with new alias projection modelFiltering out fields whose
ExprTypeexposes anoriginalPathbefore buildingfieldNameList/typeListis consistent with moving alias handling into a separate projection and avoids alias columns leaking into the base scan schema. GivenconvertExprTypeToRelDataTypealready delegates alias types togetOriginalExprType(), the two behaviors are coherent and should keep downstream code working as expected.integ-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_alias_type_field.yaml (1)
1-13: Alias-field explain fixture matches the intended logical/physical shapeThe expected plan correctly shows
LogicalProject(alias_col=[$0])overCalciteLogicalIndexScanand corresponding Enumerable operators, which is consistent with the refactored alias-field handling. No issues from a test-fixture standpoint.opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteEnumerableIndexScan.java (1)
111-123: Using row type field names for the enumerator is consistent with centralized alias handlingPassing
getRowType().getFieldNames()intoOpenSearchIndexEnumeratormatches the new model where alias columns are no longer part of the scan schema and are added by a separate projection. This avoids duplicating alias‑resolution logic in the enumerator and keeps its view aligned with the actual scan row type.integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java (1)
8-10: Alias-type explain IT wiring looks correct and completeBringing in
TEST_INDEX_ALIAS, loadingIndex.DATA_TYPE_ALIASduringinit(), and addingtestAliasTypeField()that explains a query overalias_colagainstTEST_INDEX_ALIAStogether give good end‑to‑end coverage of the new alias-field behavior. The test’s expected plan filename matches the newly added YAML resource, so this should reliably guard regressions around alias projection and pushdown.Also applies to: 36-47, 1959-1968
opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java (1)
240-281: Project pushdown now correctly uses the scan schema’s field namesIn
pushDownProject, usingnewSchema.getFieldNames().stream()in theOSRequestBuilderActionis appropriate now that alias handling has moved out of the scan:requestBuilder -> requestBuilder.pushDownProjectStream(newSchema.getFieldNames().stream());
newSchemais built as a subset of the scan’s row type (which no longer includes alias-type fields), so this pushes down only real OpenSearch fields and leaves alias columns to be handled by the separate projection layer. The aggregate‑pushed branch remains a no‑op on the request builder, so existing agg pushdown semantics are preserved.
|
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-4881-to-2.19-dev
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 6f7eae0f04f8d05b087a8b1014799faae3a44479
# Push it to GitHub
git push --set-upstream origin backport/backport-4881-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 |
…earch-project#4881) Signed-off-by: Heng Qian <[email protected]> (cherry picked from commit 6f7eae0) Signed-off-by: Heng Qian <[email protected]>
…ct with alias (#4881) (#4883) * Refactor alias type field by adding another project with alias (#4881) Signed-off-by: Heng Qian <[email protected]> (cherry picked from commit 6f7eae0) Signed-off-by: Heng Qian <[email protected]> * Fix compiling Signed-off-by: Heng Qian <[email protected]> --------- Signed-off-by: Heng Qian <[email protected]>
…earch-project#4881) Signed-off-by: Heng Qian <[email protected]>
3 participants
Description
Refactor alias type field by adding another project with alias
Related Issues
Resolves #4876
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.
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.