Repository navigation
Conversation
…ng it expand correlates the expansion with a column of its input, and the resolved field was cast to a RexInputRef unchecked. A leaf of an object or a map reaches the planner as an ITEM call, and an expression as its own call, so the cast failed as a ClassCastException and the user was shown Calcite class names instead of anything about their query. Report it the way the mvexpand check a few lines below already does: the field by name, what expand takes, and what to do instead. Closes opensearch-project#5840 Signed-off-by: Jialiang Liang <jiallian@amazon.com>
RyanL1997
requested review from
LantaoJin,
Swiddis,
acarbonetto,
ahkcs,
anirudha,
dai-chen,
joshuali925,
mengweieric,
noCharger,
penghuo,
ps48,
qianheng-aws,
songkant-aws,
vamsimanohar,
ykmr1224 and
yuancu
as code owners
October 6, 2026 18:37
Contributor
PR Reviewer Guide 🔍(Review updated until commit 55e0b03)Here are some key observations to aid the review process:
|
Contributor
PR Code Suggestions ✨Latest suggestions up to 55e0b03
Previous suggestionsSuggestions up to commit 2a495b4
|
RyanL1997
changed the title
3 tasks done
RyanL1997
added a commit
to RyanL1997/sql
that referenced
this pull request
Oct 6, 2026
A flat_object field was dropped by OpenSearchDataType.parseMapping as an unknown type, so it was absent from the schema and any reference to it failed as "field not found" (opensearch-project#1604, opensearch-project#3067). It is now a struct keyed by the dotted path of each leaf. The field declares no sub-fields, so a leaf has no type of its own: every value reads as text, and a path holding several values reads as their JSON. That is the rule spath already applies to a JSON string, so there is one rule to learn rather than two, and typeof on a leaf reports string like any other text column. Both spellings reach the same leaf -- {"a":{"b":1}} and {"a.b":1} each give the entry a.b -- and a document writing both keeps both values, in the order it wrote them. A leaf is text, so PPL does with it what it does with any text column: read it, cast it (cast(attributes.duration_ms as double) gives 12.5 for "12.5" and null for "n/a"), filter, sort, group. Nothing is restricted that is not already restricted for a spath field. The one exception is expand, which needs a column of the index to correlate the expansion with; a leaf is not one. It is reported there as a ClassCastException, which opensearch-project#5841 fixes for every field rather than only for a leaf. Signed-off-by: Jialiang Liang <jiallian@amazon.com>
RyanL1997
added a commit
to RyanL1997/sql
that referenced
this pull request
Oct 6, 2026
Both gaps a leaf filter can fall into are existing ones -- the index holds a value the engine does not read back identically, which a plain keyword or integer field reaches too -- and the paragraph said so only in its second clause. It says so first now, and adds what a reader wants next: a leaf meets fewer of them than a mapped field, since without doc values it never reaches the index for sort, dedup, stats ... by, top or rare. The expand note read as a continuation of the multi-value bullet above it. It is its own limitation and is set out as one. The expand guard itself moves to opensearch-project#5841, which fixes it for every field rather than only for a leaf, so its case goes with it. Signed-off-by: Jialiang Liang <jiallian@amazon.com>
Codecov Report❌ Patch coverage is
❌ Your project check has failed because the head coverage (63.76%) is below the target coverage (99.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #5841 +/- ##
============================================
+ Coverage 63.36% 63.76% +0.40%
- Complexity 8872 9063 +191
============================================
Files 949 962 +13
Lines 40322 40985 +663
Branches 4546 4654 +108
============================================
+ Hits 25549 26136 +587
- Misses 13929 13973 +44
- Partials 844 876 +32
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
RyanL1997
added a commit
to RyanL1997/sql
that referenced
this pull request
Oct 6, 2026
Both gaps a leaf filter can fall into belong to the engine, not to this field type: the index holds a value the engine does not read back identically, which a plain keyword or integer field reaches the same way. opensearch-project#5834 records them. Stating them here reads as though a flat_object introduced them. expand goes with them. It takes a column of the index to correlate the expansion with, which is not about a leaf either -- any expand target that is not a column of its input reaches the same place, and opensearch-project#5841 reports it for all of them. What is left is the field type: how a leaf reads, what the index answers, and what costs a record per candidate and why. Signed-off-by: Jialiang Liang <jiallian@amazon.com>
The unit test pins the planner. What the issue is about is the response: a
query that asked for something expand cannot do came back as HTTP 500 naming
Calcite classes.
spath's auto-extract mode gives a map column, and a leaf of one is reached with
ITEM, so `spath input=body | expand body.tags` is the shape a user reaches this
through. Without the fix the case fails with
500, "class org.apache.calcite.rex.RexCall cannot be cast to class
org.apache.calcite.rex.RexInputRef"
and with it the body carries the field, the code and what to do instead. A
field of the index is expanded as before, asserted alongside.
Signed-off-by: Jialiang Liang <jiallian@amazon.com>
Contributor
|
Persistent review updated to latest commit 55e0b03 |
dai-chen
reviewed
Oct 13, 2026
Comment on lines
+65
to
+67
| - match: {"$body": "/Cannot\\s+expand\\s+.body.tags./"} | ||
| - match: {"$body": "/UNSUPPORTED_OPERATION/"} | ||
| - match: {"$body": "/expand\\s+takes\\s+a\\s+field\\s+of\\s+the\\s+index/"} |
Collaborator
There was a problem hiding this comment.
can we make this assertion more readable?
| Field arrayField = expand.getField(); | ||
| RexInputRef arrayFieldRex = (RexInputRef) rexVisitor.analyze(arrayField, context); | ||
| RexNode resolved = rexVisitor.analyze(arrayField, context); | ||
| if (!(resolved instanceof RexInputRef arrayFieldRex)) { |
Collaborator
There was a problem hiding this comment.
this is a negation so the name should not be arrayFieldRex?
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
expandcorrelates the expansion with a column of its input, and the resolved field was cast to aRexInputRefunchecked:A leaf of an object or a map reaches the planner as an
ITEMcall, and an expression as its own call, so the cast failed withand the user was shown Calcite class names instead of anything about their query.
It is now reported the way the
mvexpandcheck a few lines below already reports its own unsupported target: the field by name, whatexpandtakes, and what to do instead.Testing
CalcitePPLExpandTest.testExpandOfAValueThatIsNotAColumn— a table with a map column, asserting thatexpand ATTRS.kdoes not surface as aClassCastExceptionand that the message names the field. It fails onmainand passes here.:core:test,:ppl:testandspotlessCheckpass.Issues Resolved
Closes #5840
Split out of #5830, which reached the cast with a
flat_objectleaf. The cast predates that change and is reached without it.Check List
--signoff.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.