[Backport 2.19-dev] Support distinct_count/dc in eventstats #4122
Conversation
Signed-off-by: Kai Huang <ahkcs@amazon.com> Signed-off-by: Kai Huang <105710027+ahkcs@users.noreply.github.com> (cherry picked from commit b220be4)
Signed-off-by: Kai Huang <ahkcs@amazon.com>
ahkcs
requested review from
GumpacG,
LantaoJin,
MaxKsyunz,
Swiddis,
YANG-DB,
Yury-Fridlyand,
acarbonetto,
anirudha,
dai-chen,
derek-ho,
forestmvey,
joshuali925,
kavithacm,
mengweieric,
noCharger,
penghuo,
ps48,
qianheng-aws,
seankao-az and
ykmr1224
as code owners
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
Show resolved
Hide resolved
| executeQuery( | ||
| String.format( | ||
| "source=%s | eventstats dc(state) as dc_state", TEST_INDEX_STATE_COUNTRY)); | ||
| "source=%s | eventstats dc(state) as dc_state | fields name, country, state, month, year, age, dc_state", TEST_INDEX_STATE_COUNTRY)); |
There was a problem hiding this comment.
why fields required in these ITs.
There was a problem hiding this comment.
It will cause the CI to fail, local testing was successful without fields
There was a problem hiding this comment.
It will cause the CI to fail, local testing was successful without
fields
If the original tests were successful without this PR, sounds a potential bug is introducing in this PR.
There was a problem hiding this comment.
The CI failure is this PR was caused by the order of the schema, and this failure also is not able to be reproduced locally. I suspect that it's triggered by different testing environment in CI and local. Do you think we should also add this fields to main for consistency in this case?
There was a problem hiding this comment.
and this failure also is not able to be reproduced locally. I suspect that it's triggered by different testing environment in CI and local.
The question is how the PR introduced a reproduced CI failure. we need to figure out the reason.
LantaoJin
left a comment
LantaoJin
left a comment
There was a problem hiding this comment.
In addition to Java-version-specific fixes in backporting, do not simply modify original tests during backporting. At minimum, provide an explanation for why you're bypassing the issue rather than resolving it fundamentally.
Signed-off-by: Kai Huang <105710027+ahkcs@users.noreply.github.com>
The CI failure was reproduced locally by switching to Java 11. Since different Java version will cause the order of the schema to be different, should we add the |
Do you mean when move to Java 11, even without your PR, the CI will fail either. If yes, we can add |
The CI failure is specifically for my For testing in main, the project requires Java 21 minimum. In 2.19-dev branch, I have confirmed that CI failure can be reproduced locally with Java 11. When using Java 21, the integration test will pass locally. By |
|
@ahkcs @LantaoJin |
Signed-off-by: Kai Huang <105710027+ahkcs@users.noreply.github.com>
Signed-off-by: Kai Huang <ahkcs@amazon.com>
|
FYI. This PR is intended to resolve the issue of result ordering inconsistencies across different Java versions. #3740 |
3 participants
Description
Support distinct_count/dc in eventstats #4084