Fix bug that Streamstats command incorrectly treats null as a valid group#4777
Conversation
Signed-off-by: Xinyu Hao <haoxinyu@amazon.com>
RyanL1997
left a comment
There was a problem hiding this comment.
Hi @ishaoxy , thanks for taking this on. I just left some comments. In addition, for the testing, I saw we have covered most of the 'happy' cases, consider adding more negative cases (e.g. “What happens when GROUP BY col1, col2 and only one column is null?”).
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java
Show resolved
Hide resolved
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java
Outdated
Show resolved
Hide resolved
| .toList(); | ||
| RexNode groupNotNull = | ||
| notNullList.isEmpty() | ||
| ? context.relBuilder.literal(true) |
There was a problem hiding this comment.
Just for my understanding: when notNullList.isEmpty(), returning literal(true) means all rows pass the null check. Is this the intended behavior when there are no grouping fields?
There was a problem hiding this comment.
This code now has been modified; sorry for this redundant logic. Because in this branch always hasGroup==true, notNullList will never be empty, and it will get result like [state IS NOT NULL, country IS NOT NULL] based on the group field.
|
nit: I noticed that there were multiple empty space line deletion in the diff - consider revert these changes. |
Thank you for the suggestion! I have added such negative test. |
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java
Show resolved
Hide resolved
Signed-off-by: Xinyu Hao <haoxinyu@amazon.com>
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteStreamstatsCommandIT.java
Outdated
Show resolved
Hide resolved
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteStreamstatsCommandIT.java
Show resolved
Hide resolved
Signed-off-by: Xinyu Hao <haoxinyu@amazon.com>
|
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-4777-to-2.19-dev
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 66662ae93e1f2521101fd8f85c732a4eda729d91
# Push it to GitHub
git push --set-upstream origin backport/backport-4777-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 |
… group (opensearch-project#4777) * use null to cover the aggregate value when group value is null Signed-off-by: Xinyu Hao <haoxinyu@amazon.com> * fix test Signed-off-by: Xinyu Hao <haoxinyu@amazon.com> * fix some problems Signed-off-by: Xinyu Hao <haoxinyu@amazon.com> * add IT case Signed-off-by: Xinyu Hao <haoxinyu@amazon.com> --------- Signed-off-by: Xinyu Hao <haoxinyu@amazon.com>
Description
Added a
groupNotNullpredicateWrapped each window expression in a conditional form:
CASE WHEN groupNotNull THEN raw_expr ELSE CAST(NULL AS expr_type) ENDRelated Issues
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.