Fix path navigation on map columns for spath command#5149
Conversation
Signed-off-by: Chen Dai <daichen@amazon.com>
📝 WalkthroughWalkthroughThis PR modifies how dotted field access preserves user-visible aliases in Calcite PPL. Key changes fix alias handling in CalciteRelNodeVisitor's field expansion and QualifiedNameResolver's item access resolution, ensuring projected columns retain their original names when accessing nested paths. Documentation and tests are updated to reflect the auto-extract mode behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Comment |
Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/user/ppl/cmd/spath.md (1)
146-173:⚠️ Potential issue | 🟡 MinorVerify single‑element array rendering in Example 5.
Row 2 showsdoc.tags{}aspythonwhile the rules say arrays stringify like[a, b, c]. Please confirm the actual output and align either the rule or the row value (likely[python]).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/user/ppl/cmd/spath.md` around lines 146 - 173, The example output for spath flattening is inconsistent: the rules state arrays stringify as [a, b, c] but the second row shows doc.tags{} as python; update the example so single-element arrays are rendered as [python] (or, if behaviour is different, change the rule text to state single-element arrays render without brackets) by editing the spath example block and the bullet that describes arrays (references: the spath command, output column doc.tags{}, input doc_auto and the fields line) so the example and rule are consistent.
🧹 Nitpick comments (1)
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java (1)
433-484: Consider extracting helpers from expandProjectFields (>50 LOC).
This method now exceeds the 50‑line threshold; pulling wildcard handling and ITEM aliasing into helpers would keep the visitor easier to scan.As per coding guidelines: “core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java: Flag methods >50 lines - this file is known to be hard to read”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java` around lines 433 - 484, The expandProjectFields method is too long; extract two helpers to simplify it: 1) extract wildcard expansion logic into a helper like expandWildcardFields(String fieldName, List<String> currentFields, Set<String> addedFields, CalcitePlanContext context) that uses WildcardUtils.expandWildcardPattern, filters with isMetadataField and addedFields::add and adds context.relBuilder.field(...) to expandedFields; and 2) extract dotted-path ITEM handling into a helper like resolveFieldWithItemAlias(Field field, CalcitePlanContext context) that calls rexVisitor.analyze(field, context), checks for SqlKind.ITEM and applies context.relBuilder.alias(..., fieldName) when needed; replace the inlined blocks in expandProjectFields with calls to these helpers and keep the final validateWildcardPatterns(projectList, currentFields) behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@docs/user/ppl/cmd/spath.md`:
- Around line 146-173: The example output for spath flattening is inconsistent:
the rules state arrays stringify as [a, b, c] but the second row shows
doc.tags{} as python; update the example so single-element arrays are rendered
as [python] (or, if behaviour is different, change the rule text to state
single-element arrays render without brackets) by editing the spath example
block and the bullet that describes arrays (references: the spath command,
output column doc.tags{}, input doc_auto and the fields line) so the example and
rule are consistent.
---
Nitpick comments:
In `@core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java`:
- Around line 433-484: The expandProjectFields method is too long; extract two
helpers to simplify it: 1) extract wildcard expansion logic into a helper like
expandWildcardFields(String fieldName, List<String> currentFields, Set<String>
addedFields, CalcitePlanContext context) that uses
WildcardUtils.expandWildcardPattern, filters with isMetadataField and
addedFields::add and adds context.relBuilder.field(...) to expandedFields; and
2) extract dotted-path ITEM handling into a helper like
resolveFieldWithItemAlias(Field field, CalcitePlanContext context) that calls
rexVisitor.analyze(field, context), checks for SqlKind.ITEM and applies
context.relBuilder.alias(..., fieldName) when needed; replace the inlined blocks
in expandProjectFields with calls to these helpers and keep the final
validateWildcardPatterns(projectList, currentFields) behavior unchanged.
* Struct return array value instead of string Signed-off-by: Heng Qian <qianheng@amazon.com> * Support filter in GraphLookup Signed-off-by: Heng Qian <qianheng@amazon.com> * Fix IT Signed-off-by: Heng Qian <qianheng@amazon.com> * Add experimental tag in doc Signed-off-by: Heng Qian <qianheng@amazon.com> * Increment version to 3.6.0-SNAPSHOT (#5115) Signed-off-by: opensearch-ci-bot <opensearch-infra@amazon.com> Co-authored-by: opensearch-ci-bot <opensearch-infra@amazon.com> * Fix fallback error handling to show original Calcite error (#5133) * Fix fallback error handling to show original Calcite error When Calcite falls back to V2 and V2 also fails, now correctly returns the original Calcite error instead of V2's generic "only supported when calcite enabled" message, improving error clarity for users. Fixes #5060. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Simeon Widdis <sawiddis@amazon.com> * coderabbit: preserve exception types Signed-off-by: Simeon Widdis <sawiddis@amazon.com> * Refactoring for coderabbit: consistent handling of calcite and legacy calcite errors Signed-off-by: Simeon Widdis <sawiddis@amazon.com> * Update explain error handling to match non-explain Signed-off-by: Simeon Widdis <sawiddis@amazon.com> * Minor tweaks: commenting & eval order Signed-off-by: Simeon Widdis <sawiddis@amazon.com> * Add a yaml test Signed-off-by: Simeon Widdis <sawiddis@amazon.com> --------- Signed-off-by: Simeon Widdis <sawiddis@amazon.com> Co-authored-by: Claude <noreply@anthropic.com> * Add auto-extract mode for `spath` command (#5140) * Add auto extraction mode in spath command Signed-off-by: Chen Dai <daichen@amazon.com> * Change json_extract_all to return map<string,string> and fix null perserve issue Signed-off-by: Chen Dai <daichen@amazon.com> * Refactor all unit test and integration tests Signed-off-by: Chen Dai <daichen@amazon.com> * Refactor json_extract_all and fix stringify issue Signed-off-by: Chen Dai <daichen@amazon.com> * Fix broken IT and doctest Signed-off-by: Chen Dai <daichen@amazon.com> * Address PR comments Signed-off-by: Chen Dai <daichen@amazon.com> * Mark auto extract mode as experimental Signed-off-by: Chen Dai <daichen@amazon.com> --------- Signed-off-by: Chen Dai <daichen@amazon.com> * Add nomv command (#5130) * Add nomv command + parser/AST/calcite wiring + tests + docs Signed-off-by: Srikanth Padakanti <srikanth_padakanti@apple.com> # Conflicts: # core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java # integ-test/src/test/java/org/opensearch/sql/security/CalciteCrossClusterSearchIT.java # ppl/src/main/antlr/OpenSearchPPLParser.g4 # ppl/src/test/java/org/opensearch/sql/ppl/parser/FieldResolutionVisitorTest.java * Address coderrabbit suggestions. Signed-off-by: Srikanth Padakanti <srikanth_padakanti@apple.com> * Correct the no_push_down yaml expected result Signed-off-by: Srikanth Padakanti <srikanth_padakanti@apple.com> * Fix Windows CI: Override verifyPPLToSparkSQL to preserve ARRAY_JOIN '\n' delimiter Signed-off-by: Srikanth Padakanti <srikanth_padakanti@apple.com> * Fix Windows CI: Override verifyPPLToSparkSQL to preserve ARRAY_JOIN '\n' delimiter Signed-off-by: Srikanth Padakanti <srikanth_padakanti@apple.com> * Fix Windows CI: Override verifyPPLToSparkSQL to preserve ARRAY_JOIN '\n' delimiter Signed-off-by: Srikanth Padakanti <srikanth_padakanti@apple.com> * Address code review comments Signed-off-by: Srikanth Padakanti <srikanth_padakanti@apple.com> --------- Signed-off-by: Srikanth Padakanti <srikanth_padakanti@apple.com> Co-authored-by: Srikanth Padakanti <srikanth_padakanti@apple.com> * Fix #5114: preserve head/TopK semantics for sort-expression pushdown (#5135) * Fix bug 5114 Signed-off-by: Peng Huo <penghuo@gmail.com> * Update Signed-off-by: Peng Huo <penghuo@gmail.com> * Address comments Signed-off-by: Peng Huo <penghuo@gmail.com> * Fix spotless import ordering for EnumerableLimitSort Signed-off-by: Peng Huo <penghuo@gmail.com> --------- Signed-off-by: Peng Huo <penghuo@gmail.com> * Fix path navigation on map columns for `spath` command (#5149) * Fix path navigation bug in qualified name resolver and add spath tests Signed-off-by: Chen Dai <daichen@amazon.com> * Add spath UT and IT with other commands Signed-off-by: Chen Dai <daichen@amazon.com> * Move aliasing logic from resolver to project item expansion Signed-off-by: Chen Dai <daichen@amazon.com> * Update javadoc and readme Signed-off-by: Chen Dai <daichen@amazon.com> --------- Signed-off-by: Chen Dai <daichen@amazon.com> * Make sql plugin aware of FIPS build param (-Pcrypto.standard=FIPS-140-3) (#5155) * Make sql plugin aware of FIPS build param (-Pcrypto.standard=FIPS-140-3) Signed-off-by: Craig Perkins <cwperx@amazon.com> * Update build script to include FIPS-140-3 option Added FIPS-140-3 standard option to Gradle commands. Signed-off-by: Peter Zhu <zhujiaxi@amazon.com> --------- Signed-off-by: Craig Perkins <cwperx@amazon.com> Signed-off-by: Peter Zhu <zhujiaxi@amazon.com> Co-authored-by: Peter Zhu <zhujiaxi@amazon.com> * [Maintenance] Fix bc-fips jar hell by marking dependency as compileOnly (#5158) * [maintenance] Fix bc-fips jar hell by marking dependency as compileOnly Signed-off-by: Jialiang Liang <jiallian@amazon.com> * add fix for tests Signed-off-by: Jialiang Liang <jiallian@amazon.com> --------- Signed-off-by: Jialiang Liang <jiallian@amazon.com> * [Feature] PPL Command: MvExpand (#5144) * MvExpand new PR - After resolving the merge issues Signed-off-by: Srikanth Padakanti <srikanth_padakanti@apple.com> # Conflicts: # core/src/main/java/org/opensearch/sql/analysis/Analyzer.java # core/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.java # core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java # docs/category.json # docs/user/ppl/index.md # integ-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.java # integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java # integ-test/src/test/java/org/opensearch/sql/ppl/NewAddedCommandsIT.java # integ-test/src/test/java/org/opensearch/sql/security/CalciteCrossClusterSearchIT.java # ppl/src/main/antlr/OpenSearchPPLParser.g4 # ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java # ppl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.java # ppl/src/test/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizerTest.java * Address coderrabbit comments Signed-off-by: Srikanth Padakanti <srikanth_padakanti@apple.com> * Address coderrabbit comments Signed-off-by: Srikanth Padakanti <srikanth_padakanti@apple.com> * merge main to my branch Signed-off-by: Srikanth Padakanti <srikanth_padakanti@apple.com> * Address corerabbit suggestions Signed-off-by: Srikanth Padakanti <srikanth_padakanti@apple.com> * Address corerabbit suggestions Signed-off-by: Srikanth Padakanti <srikanth_padakanti@apple.com> * ci: trigger Signed-off-by: Srikanth Padakanti <srikanth_padakanti@apple.com> * ci: trigger Signed-off-by: Srikanth Padakanti <srikanth_padakanti@apple.com> * Address corerabbit suggestions Signed-off-by: Srikanth Padakanti <srikanth_padakanti@apple.com> * ci: trigger Signed-off-by: Srikanth Padakanti <srikanth_padakanti@apple.com> * Address comments. Signed-off-by: Srikanth Padakanti <srikanth_padakanti@apple.com> * Change the exception from illegalarg to SyntaxCheckException Signed-off-by: Srikanth Padakanti <srikanth_padakanti@apple.com> * change the exception message for consistency Signed-off-by: Srikanth Padakanti <srikanth_padakanti@apple.com> * ci: rerun Signed-off-by: Srikanth Padakanti <srikanth_padakanti@apple.com> --------- Signed-off-by: Srikanth Padakanti <srikanth_padakanti@apple.com> Co-authored-by: Srikanth Padakanti <srikanth_padakanti@apple.com> * bump antlr version to 4.13.2 (#5159) * Address comments Signed-off-by: Heng Qian <qianheng@amazon.com> --------- Signed-off-by: Heng Qian <qianheng@amazon.com> Signed-off-by: opensearch-ci-bot <opensearch-infra@amazon.com> Signed-off-by: Simeon Widdis <sawiddis@amazon.com> Signed-off-by: Chen Dai <daichen@amazon.com> Signed-off-by: Srikanth Padakanti <srikanth_padakanti@apple.com> Signed-off-by: Peng Huo <penghuo@gmail.com> Signed-off-by: Craig Perkins <cwperx@amazon.com> Signed-off-by: Peter Zhu <zhujiaxi@amazon.com> Signed-off-by: Jialiang Liang <jiallian@amazon.com> Co-authored-by: opensearch-trigger-bot[bot] <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com> Co-authored-by: opensearch-ci-bot <opensearch-infra@amazon.com> Co-authored-by: Simeon Widdis <sawiddis@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Chen Dai <daichen@amazon.com> Co-authored-by: Srikanth Padakanti <srikanth29.9@gmail.com> Co-authored-by: Srikanth Padakanti <srikanth_padakanti@apple.com> Co-authored-by: Peng Huo <penghuo@gmail.com> Co-authored-by: Craig Perkins <cwperx@amazon.com> Co-authored-by: Peter Zhu <zhujiaxi@amazon.com> Co-authored-by: Jialiang Liang <jiallian@amazon.com> Co-authored-by: Eric Wei <menwe@amazon.com>
Description
QualifiedNameResolver.resolveFieldAccesspreviously wrapped nested map access with an alias unconditionally, causing issues in non-projection contexts. As a quick fix to unblock spath, this PR removes alias wrapping fromresolveFieldAccessand adds aliasing back only inexpandProjectFields. Longer term, we can consider clarifying the path navigation semantics similar to PartiQL and address issues tracked in #5152.Example
spath input=doc | where doc.user.age = 30is translated toLogicalFilter(condition=[=(SAFE_CAST(AS(ITEM($7, 'user.age'), 'doc.user.age')), 30.0E0)]), which later triggers an execution exception in the script engine because AS() is not valid.opensearchsql> source=test_spath | spath input=doc | where doc.user.age = 30; {'error': {'reason': 'Error occurred in OpenSearch engine: all shards failed', 'details': 'Shard[0]: java.lang.RuntimeException: cannot translate call AS($t3, $t4)\n\n For more details, please send request for Json format to see the raw response from OpenSearch engine.', 'type': 'SearchPhaseExecutionException'}, 'status': 500}) opensearchsql> explain source=test_spath | spath input=doc | where doc.user.age = 30; OpenSearchException({'calcite': {'logical': " LogicalSystemLimit(fetch=[10000], type=[QUERY_SIZE_LIMIT])\n LogicalProject(case=[$0], doc=[$7])\n LogicalFilter(condition=[=(SAFE_CAST(AS(ITEM($7, 'user.age'), 'doc.user.age')), 30.0E0)])\n LogicalProject(case=[$1], _id=[$2], _index=[$3], _score=[$4], _maxscore=[$5], _sort=[$6], _routing=[$7], doc=[JSON_EXTRACT_ALL($0)])\n CalciteLogicalIndexScan(table=[[OpenSearch, test_spath]])\n", 'physical': ' EnumerableCalc(expr#0..1=[{inputs}], expr#2=[JSON_EXTRACT_ALL($t1)], case=[$t0], doc=[$t2])\n CalciteEnumerableIndexScan(table=[[OpenSearch, test_spath]], PushDownContext=[[PROJECT->[case, doc], SCRIPT->=(SAFE_CAST(AS(ITEM(JSON_EXTRACT_ALL($1), \'user.age\'), \'doc.user.age\')), 30.0E0), LIMIT->10000], OpenSearchRequestBuilder(sourceBuilder={"from":0,"size":10000,"timeout":"1m","query":{"script":{"script":{"source":"{\\"langType\\":\\"calcite\\",\\"script\\":\\"..."}","lang":"opensearch_compounded_script","params":{"utcTimestamp":1771270144416573000,"SOURCES":[0,2,2,2],"DIGESTS":["doc","user.age","doc.user.age",30.0]}},"boost":1.0}},"_source":{"includes":["case","doc"],"excludes":[]}}, requestedTotalSize=10000, pageSize=null, startFrom=0)])\n'}})Related Issues
Resolves #4307
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.