Skip to content

Commit 760b65a

Browse files
emasabemasab
committed
bugfix for elastic#42041 
1 parent 0f1b076 commit 760b65a

File tree

6 files changed

+97
-18
lines changed

6 files changed

+97
-18
lines changed

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expression.java

-3
Original file line numberDiff line numberDiff line change
@@ -126,9 +126,6 @@ public boolean resolved() {
126126

127127
public abstract DataType dataType();
128128

129-
@Override
130-
public abstract int hashCode();
131-
132129
@Override
133130
public String toString() {
134131
return nodeName() + "[" + propertiesToString(false) + "]";

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/NamedExpression.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ protected Pipe makePipe() {
6767

6868
@Override
6969
public int hashCode() {
70-
return Objects.hash(id, name, synthetic);
70+
return Objects.hash(super.hashCode(), name, synthetic);
7171
}
7272

7373
@Override
@@ -96,4 +96,4 @@ public boolean equals(Object obj) {
9696
public String toString() {
9797
return super.toString() + "#" + id();
9898
}
99-
}
99+
}

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/Count.java

+6-2
Original file line numberDiff line numberDiff line change
@@ -78,11 +78,15 @@ public AggregateFunctionAttribute toAttribute() {
7878

7979
@Override
8080
public boolean equals(Object obj) {
81-
if (false == super.equals(obj)) {
81+
if (this == obj) {
82+
return true;
83+
}
84+
if (obj == null || obj.getClass() != getClass()) {
8285
return false;
8386
}
8487
Count other = (Count) obj;
85-
return Objects.equals(other.distinct(), distinct());
88+
return Objects.equals(other.distinct(), distinct())
89+
&& Objects.equals(field(), other.field());
8690
}
8791

8892
@Override

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java

+9-10
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@
1212
import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;
1313
import org.elasticsearch.xpack.sql.expression.Attribute;
1414
import org.elasticsearch.xpack.sql.expression.Expression;
15-
import org.elasticsearch.xpack.sql.expression.ExpressionId;
1615
import org.elasticsearch.xpack.sql.expression.Expressions;
1716
import org.elasticsearch.xpack.sql.expression.FieldAttribute;
1817
import org.elasticsearch.xpack.sql.expression.Foldables;
@@ -210,14 +209,14 @@ static LeafAgg toAgg(String id, Function f) {
210209
}
211210

212211
static class GroupingContext {
213-
final Map<ExpressionId, GroupByKey> groupMap;
212+
final Map<NamedExpression, GroupByKey> groupMap;
214213
final GroupByKey tail;
215214

216-
GroupingContext(Map<ExpressionId, GroupByKey> groupMap) {
215+
GroupingContext(Map<NamedExpression, GroupByKey> groupMap) {
217216
this.groupMap = groupMap;
218217

219218
GroupByKey lastAgg = null;
220-
for (Entry<ExpressionId, GroupByKey> entry : groupMap.entrySet()) {
219+
for (Entry<NamedExpression, GroupByKey> entry : groupMap.entrySet()) {
221220
lastAgg = entry.getValue();
222221
}
223222

@@ -232,7 +231,7 @@ GroupByKey groupFor(Expression exp) {
232231
GroupByKey matchingGroup = null;
233232
// group found - finding the dedicated agg
234233
if (f.field() instanceof NamedExpression) {
235-
matchingGroup = groupMap.get(((NamedExpression) f.field()).id());
234+
matchingGroup = groupMap.get(f.field());
236235
}
237236
// return matching group or the tail (last group)
238237
return matchingGroup != null ? matchingGroup : tail;
@@ -242,7 +241,7 @@ GroupByKey groupFor(Expression exp) {
242241
}
243242
}
244243
if (exp instanceof NamedExpression) {
245-
return groupMap.get(((NamedExpression) exp).id());
244+
return groupMap.get(exp);
246245
}
247246
throw new SqlIllegalArgumentException("Don't know how to find group for expression {}", exp);
248247
}
@@ -261,18 +260,18 @@ static GroupingContext groupBy(List<? extends Expression> groupings) {
261260
return null;
262261
}
263262

264-
Map<ExpressionId, GroupByKey> aggMap = new LinkedHashMap<>();
263+
Map<NamedExpression, GroupByKey> aggMap = new LinkedHashMap<>();
265264

266265
for (Expression exp : groupings) {
267266
GroupByKey key = null;
268-
ExpressionId id;
267+
NamedExpression id;
269268
String aggId;
270269

271270
if (exp instanceof NamedExpression) {
272271
NamedExpression ne = (NamedExpression) exp;
273272

274-
id = ne.id();
275-
aggId = id.toString();
273+
id = ne;
274+
aggId = ne.id().toString();
276275

277276
// change analyzed to non non-analyzed attributes
278277
if (exp instanceof FieldAttribute) {

‎x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,7 @@ public void testDuplicateFunctions() {
236236
assertTrue(result instanceof Project);
237237
List<? extends NamedExpression> projections = ((Project) result).projections();
238238
assertEquals(2, projections.size());
239-
assertSame(projections.get(0), projections.get(1));
239+
assertEquals(projections.get(0), projections.get(1));
240240
}
241241

242242
public void testCombineProjections() {

‎x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java

+79
Original file line numberDiff line numberDiff line change
@@ -1132,6 +1132,85 @@ public void testAllCountVariantsWithHavingGenerateCorrectAggregations() {
11321132
+ "\"gap_policy\":\"skip\"}}}}}"));
11331133
}
11341134

1135+
public void testGroupSelection() {
1136+
{
1137+
PhysicalPlan p = optimizeAndPlan("SELECT EXTRACT(MINUTE FROM CONVERT(date, SQL_TIMESTAMP)) x FROM test GROUP BY x");
1138+
assertEquals(EsQueryExec.class, p.getClass());
1139+
assertEquals(1, p.output().size());
1140+
assertEquals("x", p.output().get(0).qualifiedName());
1141+
assertEquals(DataType.INTEGER, p.output().get(0).dataType());
1142+
assertThat(
1143+
((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString()
1144+
.replaceAll("\\s+", ""),
1145+
endsWith("{\"source\":\"InternalSqlScriptUtils.dateTimeChrono(" +
1146+
"InternalSqlScriptUtils.docValue(doc,params.v0),params.v1,params.v2)\",\"lang\":\"painless\"," +
1147+
"\"params\":{\"v0\":\"date\",\"v1\":\"Z\",\"v2\":\"MINUTE_OF_HOUR\"}}," +
1148+
"\"missing_bucket\":true,\"value_type\":\"long\",\"order\":\"asc\"}}}]}}}")
1149+
);
1150+
}
1151+
{
1152+
PhysicalPlan p = optimizeAndPlan("SELECT EXTRACT(HOUR FROM CONVERT(date, SQL_TIMESTAMP)) FROM test GROUP BY " +
1153+
"EXTRACT(HOUR FROM CONVERT(date, SQL_TIMESTAMP))");
1154+
assertEquals(EsQueryExec.class, p.getClass());
1155+
assertEquals(1, p.output().size());
1156+
assertEquals("EXTRACT(HOUR FROM CONVERT(date, SQL_TIMESTAMP))", p.output().get(0).qualifiedName());
1157+
assertEquals(DataType.INTEGER, p.output().get(0).dataType());
1158+
assertThat(
1159+
((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString()
1160+
.replaceAll("\\s+", ""),
1161+
endsWith("{\"source\":\"InternalSqlScriptUtils.dateTimeChrono(" +
1162+
"InternalSqlScriptUtils.docValue(doc,params.v0),params.v1,params.v2)\",\"lang\":\"painless\"," +
1163+
"\"params\":{\"v0\":\"date\",\"v1\":\"Z\",\"v2\":\"HOUR_OF_DAY\"}},\"missing_bucket\":true," +
1164+
"\"value_type\":\"long\",\"order\":\"asc\"}}}]}}}")
1165+
);
1166+
}
1167+
{
1168+
PhysicalPlan p = optimizeAndPlan("SELECT PI() * int FROM test GROUP BY 1");
1169+
assertEquals(EsQueryExec.class, p.getClass());
1170+
assertEquals(1, p.output().size());
1171+
assertEquals("PI() * int", p.output().get(0).qualifiedName());
1172+
assertEquals(DataType.DOUBLE, p.output().get(0).dataType());
1173+
assertThat(
1174+
((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString()
1175+
.replaceAll("\\s+", ""),
1176+
endsWith("{\"source\":\"InternalSqlScriptUtils.mul(params.v0,InternalSqlScriptUtils.docValue(doc,params.v1))\"," +
1177+
"\"lang\":\"painless\",\"params\":{\"v0\":3.141592653589793,\"v1\":\"int\"}}," +
1178+
"\"missing_bucket\":true,\"value_type\":\"double\",\"order\":\"asc\"}}}]}}}")
1179+
);
1180+
}
1181+
{
1182+
PhysicalPlan p = optimizeAndPlan("SELECT date + 1 * INTERVAL '1' DAY FROM test GROUP BY 1");
1183+
assertEquals(EsQueryExec.class, p.getClass());
1184+
assertEquals(1, p.output().size());
1185+
assertEquals("date + 1 * INTERVAL '1' DAY", p.output().get(0).qualifiedName());
1186+
assertEquals(DataType.DATETIME, p.output().get(0).dataType());
1187+
assertThat(
1188+
((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString()
1189+
.replaceAll("\\s+", ""),
1190+
endsWith("{\"source\":\"InternalSqlScriptUtils.add(InternalSqlScriptUtils.docValue(doc,params.v0)," +
1191+
"InternalSqlScriptUtils.intervalDayTime(params.v1,params.v2))\"," +
1192+
"\"lang\":\"painless\",\"params\":{\"v0\":\"date\",\"v1\":\"PT24H\",\"v2\":\"INTERVAL_DAY\"}}," +
1193+
"\"missing_bucket\":true,\"value_type\":\"long\",\"order\":\"asc\"}}}]}}}")
1194+
);
1195+
}
1196+
{
1197+
PhysicalPlan p = optimizeAndPlan("select (3 < int) as multi_language, count(*) from test group by multi_language");
1198+
assertEquals(EsQueryExec.class, p.getClass());
1199+
assertEquals(2, p.output().size());
1200+
assertEquals("multi_language", p.output().get(0).qualifiedName());
1201+
assertEquals(DataType.BOOLEAN, p.output().get(0).dataType());
1202+
assertEquals("count(*)", p.output().get(1).qualifiedName());
1203+
assertEquals(DataType.LONG, p.output().get(1).dataType());
1204+
assertThat(
1205+
((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString()
1206+
.replaceAll("\\s+", ""),
1207+
endsWith("{\"source\":\"InternalSqlScriptUtils.gt(InternalSqlScriptUtils.docValue(doc,params.v0),params.v1)\"," +
1208+
"\"lang\":\"painless\",\"params\":{\"v0\":\"int\",\"v1\":3}}," +
1209+
"\"missing_bucket\":true,\"value_type\":\"boolean\",\"order\":\"asc\"}}}]}}}")
1210+
);
1211+
}
1212+
}
1213+
11351214
public void testTopHitsAggregationWithOneArg() {
11361215
{
11371216
PhysicalPlan p = optimizeAndPlan("SELECT FIRST(keyword) FROM test");

0 commit comments

Comments
 (0)