Skip to content

Commit 1efb6c3

Browse files
ahkcsahkcs
authored
Add tiebreaker to stats sort-on-measure IT queries (opensearch-project#5435)
When a top-K query sorts by a measure column that has ties at the cutoff, PPL spec doesn't define a tiebreaker, so different execution engines (legacy DSL terms-agg, Calcite enumerable sort, analytics-engine hash sort) return different tied rows. The tests were over-specified — they asserted whichever member happened to come from the engine used when they were authored. Address by adding the group-by column(s) as an explicit secondary sort key. The query is now deterministic regardless of engine, and the strict containsInAnyOrder assertion can stay. Six tests in StatsCommandIT touched: - testStatsSortOnMeasure (Q1, Q2) - testStatsSortOnMeasureWithScript (Q1, Q2) - testStatsSpanSortOnMeasure (Q1, Q2 — tiebreaker references the span column via its auto-generated name) - testStatsSpanSortOnMeasureMultiTerms (Q1, Q2) - testStatsSpanSortOnMeasureMultiTermsWithScript (Q1, Q2) - testStatsSortOnMeasureComplex (Q1, Q2) Verified against the default Calcite-DSL path: 63 / 63 pass. Suggested by @penghuo on review of an earlier revision that added a tie-tolerant matcher to MatcherUtils — the tiebreaker approach fixes the root cause and keeps the existing strict assertion semantics. Signed-off-by: Kai Huang <ahkcs@amazon.com>
1 parent 89c2de0 commit 1efb6c3

1 file changed

Lines changed: 61 additions & 115 deletions

File tree

‎integ-test/src/test/java/org/opensearch/sql/ppl/StatsCommandIT.java‎

Lines changed: 61 additions & 115 deletions
Original file line numberDiff line numberDiff line change
@@ -811,37 +811,26 @@ public void testStatsByDependentGroupFields() throws IOException {
811811
public void testStatsSortOnMeasure() throws IOException {
812812
try {
813813
setQueryBucketSize(5);
814+
// count=25 is tied across multiple states (AL/ME/TN/WY); add `state` as an explicit
815+
// tiebreaker so the top-K cutoff is deterministic. PPL spec doesn't define a
816+
// tiebreaker for tied measure values, so without this the result varies by engine.
814817
JSONObject response =
815818
executeQuery(
816819
String.format(
817-
"source=%s | stats bucket_nullable=false count() by state | sort - `count()` |"
818-
+ " head 5",
820+
"source=%s | stats bucket_nullable=false count() by state | sort - `count()`,"
821+
+ " state | head 5",
819822
TEST_INDEX_ACCOUNT));
820823
verifyDataRows(
821-
response, rows(30, "TX"), rows(28, "MD"), rows(27, "ID"), rows(25, "ME"), rows(25, "AL"));
824+
response, rows(30, "TX"), rows(28, "MD"), rows(27, "ID"), rows(25, "AL"), rows(25, "ME"));
825+
// Ascending: count=13 (NV/SC) and count=14 (many states); state asc as tiebreaker.
822826
response =
823827
executeQuery(
824828
String.format(
825-
"source=%s | stats bucket_nullable=false count() by state | sort `count()` | head"
826-
+ " 5",
829+
"source=%s | stats bucket_nullable=false count() by state | sort `count()`,"
830+
+ " state | head 5",
827831
TEST_INDEX_ACCOUNT));
828-
if (!isPushdownDisabled()) {
829-
verifyDataRows(
830-
response,
831-
rows(13, "NV"),
832-
rows(13, "SC"),
833-
rows(14, "CO"),
834-
rows(14, "AZ"),
835-
rows(14, "DE"));
836-
} else {
837-
verifyDataRows(
838-
response,
839-
rows(13, "NV"),
840-
rows(13, "SC"),
841-
rows(14, "DE"),
842-
rows(14, "AZ"),
843-
rows(14, "NM"));
844-
}
832+
verifyDataRows(
833+
response, rows(13, "NV"), rows(13, "SC"), rows(14, "AZ"), rows(14, "CO"), rows(14, "DE"));
845834
response =
846835
executeQuery(
847836
String.format(
@@ -877,11 +866,14 @@ public void testStatsSortOnMeasure() throws IOException {
877866
public void testStatsSpanSortOnMeasure() throws IOException {
878867
try {
879868
setQueryBucketSize(5);
869+
// Many month buckets tie at cnt=1; add the span column (referenced via its
870+
// auto-generated column name) as an explicit tiebreaker so the top-K cutoff is
871+
// deterministic.
880872
JSONObject response =
881873
executeQuery(
882874
String.format(
883875
"source=%s | stats bucket_nullable=false count() as cnt by span(birthdate,"
884-
+ " 1month) | sort - cnt | head 5",
876+
+ " 1month) | sort - cnt, `span(birthdate,1month)` | head 5",
885877
TEST_INDEX_BANK));
886878
verifyDataRows(
887879
response,
@@ -894,15 +886,15 @@ public void testStatsSpanSortOnMeasure() throws IOException {
894886
executeQuery(
895887
String.format(
896888
"source=%s | stats bucket_nullable=false count() as cnt by span(birthdate,"
897-
+ " 1month) | sort cnt | head 5",
889+
+ " 1month) | sort cnt, `span(birthdate,1month)` | head 5",
898890
TEST_INDEX_BANK));
899891
verifyDataRows(
900892
response,
901-
rows(1, "2018-11-01 00:00:00"),
902-
rows(1, "2017-11-01 00:00:00"),
903893
rows(1, "2017-10-01 00:00:00"),
904-
rows(2, "2018-08-01 00:00:00"),
905-
rows(2, "2018-06-01 00:00:00"));
894+
rows(1, "2017-11-01 00:00:00"),
895+
rows(1, "2018-11-01 00:00:00"),
896+
rows(2, "2018-06-01 00:00:00"),
897+
rows(2, "2018-08-01 00:00:00"));
906898
response =
907899
executeQuery(
908900
String.format(
@@ -938,37 +930,24 @@ public void testStatsSpanSortOnMeasure() throws IOException {
938930
public void testStatsSortOnMeasureWithScript() throws IOException {
939931
try {
940932
setQueryBucketSize(5);
933+
// Script-derived `new_state` mirrors testStatsSortOnMeasure shape. Add new_state
934+
// as the tiebreaker.
941935
JSONObject response =
942936
executeQuery(
943937
String.format(
944938
"source=%s | eval new_state = lower(state) | stats bucket_nullable=false count()"
945-
+ " by new_state | sort - `count()` | head 5",
939+
+ " by new_state | sort - `count()`, new_state | head 5",
946940
TEST_INDEX_ACCOUNT));
947941
verifyDataRows(
948-
response, rows(30, "tx"), rows(28, "md"), rows(27, "id"), rows(25, "me"), rows(25, "al"));
942+
response, rows(30, "tx"), rows(28, "md"), rows(27, "id"), rows(25, "al"), rows(25, "me"));
949943
response =
950944
executeQuery(
951945
String.format(
952946
"source=%s | eval new_state = lower(state) | stats bucket_nullable=false count()"
953-
+ " by new_state | sort `count()` | head 5",
947+
+ " by new_state | sort `count()`, new_state | head 5",
954948
TEST_INDEX_ACCOUNT));
955-
if (!isPushdownDisabled()) {
956-
verifyDataRows(
957-
response,
958-
rows(13, "nv"),
959-
rows(13, "sc"),
960-
rows(14, "co"),
961-
rows(14, "az"),
962-
rows(14, "de"));
963-
} else {
964-
verifyDataRows(
965-
response,
966-
rows(13, "nv"),
967-
rows(13, "sc"),
968-
rows(14, "de"),
969-
rows(14, "az"),
970-
rows(14, "nm"));
971-
}
949+
verifyDataRows(
950+
response, rows(13, "nv"), rows(13, "sc"), rows(14, "az"), rows(14, "co"), rows(14, "de"));
972951
} finally {
973952
resetQueryBucketSize();
974953
}
@@ -1013,52 +992,34 @@ public void testStatsSpanSortOnMeasureWithScript() throws IOException {
1013992
public void testStatsSpanSortOnMeasureMultiTerms() throws IOException {
1014993
try {
1015994
setQueryBucketSize(5);
995+
// count=17 ties between (M,ID) and (F,TX); count=5 ties across many (gender, state)
996+
// pairs. Add gender, state as tiebreakers so the top-K cutoff is deterministic.
1016997
JSONObject response =
1017998
executeQuery(
1018999
String.format(
10191000
"source=%s | stats bucket_nullable=false count() by gender, state | sort -"
1020-
+ " `count()` | head 5",
1001+
+ " `count()`, gender, state | head 5",
10211002
TEST_INDEX_ACCOUNT));
10221003
verifyDataRows(
10231004
response,
10241005
rows(18, "M", "MD"),
1025-
rows(17, "M", "ID"),
10261006
rows(17, "F", "TX"),
1007+
rows(17, "M", "ID"),
10271008
rows(16, "M", "ME"),
10281009
rows(15, "M", "OK"));
10291010
response =
10301011
executeQuery(
10311012
String.format(
10321013
"source=%s | stats bucket_nullable=false count() by gender, state | sort"
1033-
+ " `count()` | head 5",
1014+
+ " `count()`, gender, state | head 5",
10341015
TEST_INDEX_ACCOUNT));
1035-
if (isCalciteEnabled()) {
1036-
if (!isPushdownDisabled()) {
1037-
verifyDataRows(
1038-
response,
1039-
rows(3, "F", "DE"),
1040-
rows(5, "F", "CT"),
1041-
rows(5, "F", "OR"),
1042-
rows(5, "F", "WI"),
1043-
rows(5, "M", "MI"));
1044-
} else {
1045-
verifyDataRows(
1046-
response,
1047-
rows(3, "F", "DE"),
1048-
rows(5, "F", "WI"),
1049-
rows(5, "F", "OR"),
1050-
rows(5, "M", "RI"),
1051-
rows(5, "F", "CT"));
1052-
}
1053-
} else {
1054-
verifyDataRows(
1055-
response,
1056-
rows(3, "F", "DE"),
1057-
rows(5, "M", "RI"),
1058-
rows(5, "M", "MI"),
1059-
rows(5, "F", "WI"),
1060-
rows(5, "M", "NE"));
1061-
}
1016+
verifyDataRows(
1017+
response,
1018+
rows(3, "F", "DE"),
1019+
rows(5, "F", "CT"),
1020+
rows(5, "F", "OR"),
1021+
rows(5, "F", "WI"),
1022+
rows(5, "M", "MI"));
10621023
response =
10631024
executeQuery(
10641025
String.format(
@@ -1094,54 +1055,36 @@ public void testStatsSpanSortOnMeasureMultiTerms() throws IOException {
10941055
public void testStatsSpanSortOnMeasureMultiTermsWithScript() throws IOException {
10951056
try {
10961057
setQueryBucketSize(5);
1058+
// Same tie shapes as testStatsSpanSortOnMeasureMultiTerms but with script-derived
1059+
// group columns. Add new_gender, new_state as tiebreakers.
10971060
JSONObject response =
10981061
executeQuery(
10991062
String.format(
11001063
"source=%s | eval new_gender = lower(gender), new_state = lower(state) | stats"
1101-
+ " bucket_nullable=false count() by new_gender, new_state | sort - `count()`"
1102-
+ " | head 5",
1064+
+ " bucket_nullable=false count() by new_gender, new_state | sort -"
1065+
+ " `count()`, new_gender, new_state | head 5",
11031066
TEST_INDEX_ACCOUNT));
11041067
verifyDataRows(
11051068
response,
11061069
rows(18, "m", "md"),
1107-
rows(17, "m", "id"),
11081070
rows(17, "f", "tx"),
1071+
rows(17, "m", "id"),
11091072
rows(16, "m", "me"),
11101073
rows(15, "m", "ok"));
11111074
response =
11121075
executeQuery(
11131076
String.format(
11141077
"source=%s | eval new_gender = lower(gender), new_state = lower(state) | stats"
1115-
+ " bucket_nullable=false count() by new_gender, new_state | sort `count()` |"
1116-
+ " head 5",
1078+
+ " bucket_nullable=false count() by new_gender, new_state | sort `count()`,"
1079+
+ " new_gender, new_state | head 5",
11171080
TEST_INDEX_ACCOUNT));
1118-
if (isCalciteEnabled()) {
1119-
if (!isPushdownDisabled()) {
1120-
verifyDataRows(
1121-
response,
1122-
rows(3, "f", "de"),
1123-
rows(5, "f", "ct"),
1124-
rows(5, "f", "or"),
1125-
rows(5, "f", "wi"),
1126-
rows(5, "m", "mi"));
1127-
} else {
1128-
verifyDataRows(
1129-
response,
1130-
rows(3, "f", "de"),
1131-
rows(5, "m", "ri"),
1132-
rows(5, "f", "ct"),
1133-
rows(5, "m", "mi"),
1134-
rows(5, "m", "ne"));
1135-
}
1136-
} else {
1137-
verifyDataRows(
1138-
response,
1139-
rows(3, "f", "de"),
1140-
rows(5, "m", "ri"),
1141-
rows(5, "m", "mi"),
1142-
rows(5, "f", "wi"),
1143-
rows(5, "m", "ne"));
1144-
}
1081+
verifyDataRows(
1082+
response,
1083+
rows(3, "f", "de"),
1084+
rows(5, "f", "ct"),
1085+
rows(5, "f", "or"),
1086+
rows(5, "f", "wi"),
1087+
rows(5, "m", "mi"));
11451088
response =
11461089
executeQuery(
11471090
String.format(
@@ -1179,11 +1122,13 @@ public void testStatsSpanSortOnMeasureMultiTermsWithScript() throws IOException
11791122
public void testStatsSortOnMeasureComplex() throws IOException {
11801123
try {
11811124
setQueryBucketSize(5);
1125+
// c=25 ties across multiple states; add state as tiebreaker for a deterministic
1126+
// top-K cutoff.
11821127
JSONObject response =
11831128
executeQuery(
11841129
String.format(
11851130
"source=%s | stats bucket_nullable=false sum(balance), count() as c, dc(employer)"
1186-
+ " as d by state | sort - c | head 5",
1131+
+ " as d by state | sort - c, state | head 5",
11871132
TEST_INDEX_ACCOUNT));
11881133
verifySchema(
11891134
response,
@@ -1197,14 +1142,15 @@ public void testStatsSortOnMeasureComplex() throws IOException {
11971142
rows(782199, 30, 30, "TX"),
11981143
rows(732523, 28, 28, "MD"),
11991144
rows(657957, 27, 27, "ID"),
1200-
rows(541575, 25, 25, "ME"),
1201-
rows(643489, 25, 25, "AL"));
1145+
rows(643489, 25, 25, "AL"),
1146+
rows(541575, 25, 25, "ME"));
1147+
// d=17 ties between (M, id) and (F, tx); add gender, new_state as tiebreakers.
12021148
response =
12031149
executeQuery(
12041150
String.format(
12051151
"source=%s | eval new_state = lower(state) | stats bucket_nullable=false"
12061152
+ " sum(balance), count() as c, dc(employer) as d by gender, new_state | sort"
1207-
+ " - d | head 5",
1153+
+ " - d, gender, new_state | head 5",
12081154
TEST_INDEX_ACCOUNT));
12091155
verifySchema(
12101156
response,
@@ -1217,8 +1163,8 @@ public void testStatsSortOnMeasureComplex() throws IOException {
12171163
verifyDataRows(
12181164
response,
12191165
rows(484567, 18, 18, "M", "md"),
1220-
rows(376394, 17, 17, "M", "id"),
12211166
rows(505688, 17, 17, "F", "tx"),
1167+
rows(376394, 17, 17, "M", "id"),
12221168
rows(375409, 16, 16, "M", "me"),
12231169
rows(432776, 15, 15, "M", "ok"));
12241170
} finally {

0 commit comments

Comments
 (0)