Skip to content

Commit 52efe34

Browse files
andiemontoyeahandiem-bq
andiemontoyeah
and
andiem-bq
authored
[AD-454] Fix for incorrect field names in DocumentDbAggregagte when there is name conflict (#183)
* WIP - untested draft with and to count nulls * [AD-196] - Initial solution - need to run performance + TDVT tests * Add/update unit tests * Update tests * Update test description * Commit Code Coverage Badge * Add additional unit test for sum zero case * Commit Code Coverage Badge * [AD-196] Improvements from code review * Commit Code Coverage Badge * [AD-454] WIP - update field names in DocumentDbAggregate * [AD-334] Initial implementation with some updated unit tests - additional tests still missing * [AD-196] Improvements from code review * Update unit tests * Update and add unit tests * [AD-334] Update unit tests + address checkstyle issues * Commit Code Coverage Badge * Update unit tests with changes from develop * [AD-454] Update unit tests * Fix test name conflict * Separate inNames and fieldNames * Commit Code Coverage Badge * Commit Code Coverage Badge Co-authored-by: andiem-bq <[email protected]>
1 parent f64ff0f commit 52efe34

File tree

4 files changed

+91
-59
lines changed

4 files changed

+91
-59
lines changed

‎.github/badges/jacoco.svg‎

Lines changed: 1 addition & 1 deletion
Loading

‎calcite-adapter/src/main/java/software/amazon/documentdb/jdbc/calcite/adapter/DocumentDbAggregate.java‎

Lines changed: 42 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
import java.util.LinkedHashMap;
3939
import java.util.List;
4040

41+
import static software.amazon.documentdb.jdbc.calcite.adapter.DocumentDbRules.getNormalizedIdentifier;
4142
import static software.amazon.documentdb.jdbc.calcite.adapter.DocumentDbRules.maybeQuote;
4243

4344
/**
@@ -123,45 +124,52 @@ public DocumentDbAggregate(final RelOptCluster cluster, final RelTraitSet traitS
123124
new DocumentDbRel.Implementor(implementor.getRexBuilder());
124125
mongoImplementor.visitChild(0, getInput());
125126
// DocumentDB: modified - start
126-
final List<String> inNames =
127+
final List<String> mongoFieldNames =
127128
DocumentDbRules.mongoFieldNames(getInput().getRowType(),
128129
mongoImplementor.getMetadataTable());
129-
final List<String> outNames =
130-
DocumentDbRules.mongoFieldNames(getRowType(),
131-
mongoImplementor.getMetadataTable());
130+
final List<String> inNames = getInput().getRowType().getFieldNames();
131+
final List<String> outNames = getRowType().getFieldNames();
132132
final LinkedHashMap<String, DocumentDbSchemaColumn> columnMap = new LinkedHashMap<>(implementor.getMetadataTable().getColumnMap());
133-
int i = 0;
133+
134+
int columnIndex = 0;
134135
if (groupSet.cardinality() == 1) {
136+
final String outName = outNames.get(columnIndex);
135137
final String inName = inNames.get(groupSet.nth(0));
136-
list.add("_id: " + maybeQuote("$" + inName));
137-
++i;
138+
final String fieldName = mongoFieldNames.get(groupSet.nth(0));
139+
final DocumentDbSchemaColumn oldColumn = implementor.getMetadataTable().getColumnMap().get(inName);
140+
list.add("_id: " + maybeQuote("$" + fieldName));
141+
columnMap.put(outName, getUpdatedColumn(oldColumn, outName));
142+
++columnIndex;
138143
} else {
139144
final List<String> keys = new ArrayList<>();
140145
for (int group : groupSet) {
146+
final String outName = outNames.get(columnIndex);
141147
final String inName = inNames.get(group);
142-
// Replace any '.'s with _ as the temporary field names in the group by output document.
143-
keys.add(maybeQuote(acceptedMongoFieldName(inName)) + ": " + DocumentDbRules.quote("$" + inName));
144-
++i;
148+
final String fieldName = mongoFieldNames.get(group);
149+
final DocumentDbSchemaColumn oldColumn = implementor.getMetadataTable().getColumnMap().get(inName);
150+
keys.add(maybeQuote(acceptedMongoFieldName(outName)) + ": " + DocumentDbRules.quote("$" + fieldName));
151+
columnMap.put(outName, getUpdatedColumn(oldColumn, outName));
152+
++columnIndex;
145153
}
146154
list.add("_id: " + Util.toString(keys, "{", ", ", "}"));
147155
}
148156

149157
for (AggregateCall aggCall : aggCalls) {
150-
final String outName = outNames.get(i++);
158+
final String outName = outNames.get(columnIndex++);
151159
list.add(
152-
maybeQuote(outName) + ": "
153-
+ toMongo(aggCall.getAggregation(), inNames, aggCall.getArgList(), aggCall.isDistinct()));
160+
maybeQuote(acceptedMongoFieldName(outName)) + ": "
161+
+ toMongo(aggCall.getAggregation(), mongoFieldNames, aggCall.getArgList(), aggCall.isDistinct()));
154162
columnMap.put(outName,
155163
DocumentDbMetadataColumn.builder()
156-
.fieldPath(outName)
157164
.isGenerated(true)
165+
.fieldPath(acceptedMongoFieldName(outName))
158166
.sqlName(outName)
159167
.build());
160168

161169
}
162170
implementor.add(null,
163171
"{$group: " + Util.toString(list, "{", ", ", "}") + "}");
164-
final List<String> fixups = getFixups(aggCalls, groupSet, inNames, outNames);
172+
final List<String> fixups = getFixups(aggCalls, groupSet, outNames);
165173

166174
if (!groupSet.isEmpty()
167175
|| aggCalls.stream().anyMatch(aggCall -> aggCall.isDistinct() || aggCall.getAggregation() == SqlStdOperatorTable.SUM)) {
@@ -187,6 +195,20 @@ public DocumentDbAggregate(final RelOptCluster cluster, final RelTraitSet traitS
187195
// DocumentDB: modified - end
188196
}
189197

198+
private static DocumentDbSchemaColumn getUpdatedColumn(final DocumentDbSchemaColumn oldColumn, final String outName) {
199+
return DocumentDbMetadataColumn.builder()
200+
.fieldPath(oldColumn.getFieldPath())
201+
.sqlName(oldColumn.getSqlName())
202+
.sqlType(oldColumn.getSqlType())
203+
.dbType(oldColumn.getDbType())
204+
.isIndex(oldColumn.isIndex())
205+
.isPrimaryKey(oldColumn.isPrimaryKey())
206+
.foreignKeyTableName(oldColumn.getForeignKeyTableName())
207+
.foreignKeyColumnName(oldColumn.getForeignKeyColumnName())
208+
.resolvedPath(acceptedMongoFieldName(outName))
209+
.build();
210+
}
211+
190212
private static String toMongo(final SqlAggFunction aggregation, final List<String> inNames,
191213
final List<Integer> args, final boolean isDistinct) {
192214

@@ -230,7 +252,7 @@ private static String toMongo(final SqlAggFunction aggregation, final List<Strin
230252
}
231253

232254
private static String acceptedMongoFieldName(final String path) {
233-
return path.replace('.', '_');
255+
return getNormalizedIdentifier(path).replace('.', '_');
234256
}
235257

236258
private static String setToAggregate(final SqlAggFunction aggFunction, final String outName) {
@@ -266,14 +288,12 @@ private static String arrayToSum(final String outName) {
266288
* Logic was pulled out of original implementation of implement method.
267289
* @param aggCalls the aggregate calls.
268290
* @param groupSet the group set.
269-
* @param inNames the names of the input row type.
270291
* @param outNames the names of the output row type.
271292
* @return list of fields that should be projected.
272293
*/
273294
private static List<String> getFixups(
274295
final List<AggregateCall> aggCalls,
275296
final ImmutableBitSet groupSet,
276-
final List<String> inNames,
277297
final List<String> outNames) {
278298
// DocumentDB: modified - start
279299
final List<String> fixups = new ArrayList<>();
@@ -282,18 +302,17 @@ private static List<String> getFixups(
282302
fixups.add(maybeQuote(outNames.get(columnIndex++)) + ": " + maybeQuote("$" + "_id"));
283303
} else {
284304
fixups.add("_id: 0");
285-
// We project the original field names (inNames) rather than any renames so the path matches the metadata.
286305
for (int group : groupSet) {
306+
final String outName = acceptedMongoFieldName(outNames.get(columnIndex++));
287307
fixups.add(
288-
maybeQuote(inNames.get(group))
308+
maybeQuote(outName)
289309
+ ": "
290-
+ maybeQuote("$_id." + acceptedMongoFieldName(inNames.get(group))));
291-
++columnIndex;
310+
+ maybeQuote("$_id." + acceptedMongoFieldName(outName)));
292311
}
293312

294313
}
295314
for (AggregateCall aggCall : aggCalls) {
296-
final String outName = outNames.get(columnIndex++);
315+
final String outName = acceptedMongoFieldName(outNames.get(columnIndex++));
297316
// Get the aggregate for any sets made in $group stage.
298317
if (aggCall.isDistinct()) {
299318
fixups.add(maybeQuote(outName) + ": " + setToAggregate(

‎calcite-adapter/src/test/java/software/amazon/documentdb/jdbc/query/DocumentDbQueryMappingServiceBasicTest.java‎

Lines changed: 20 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,7 @@ void testQueryWithDistinct() throws SQLException {
178178
BsonDocument.parse("{\"$group\": {\"_id\": \"$array.field\"}}"),
179179
result.getAggregateOperations().get(2));
180180
Assertions.assertEquals(
181-
BsonDocument.parse("{\"$project\": {\"array.field\": \"$_id\"}}"),
181+
BsonDocument.parse("{\"$project\": {\"field\": \"$_id\"}}"),
182182
result.getAggregateOperations().get(3));
183183
}
184184

@@ -475,11 +475,11 @@ void testQueryWithGroupBy() throws SQLException {
475475
result.getAggregateOperations().get(1));
476476
Assertions.assertEquals(
477477
BsonDocument.parse(
478-
"{\"$group\": {\"_id\": {\"_id\": \"$_id\", \"array_field\": \"$array.field\", \"array_field1\": \"$array.field1\"}}}"),
478+
"{\"$group\": {\"_id\": {\"testCollection__id\": \"$_id\", \"field\": \"$array.field\", \"field1\": \"$array.field1\"}}}"),
479479
result.getAggregateOperations().get(2));
480480
Assertions.assertEquals(
481481
BsonDocument.parse(
482-
"{\"$project\": {\"_id\": \"$_id._id\", \"array.field\": \"$_id.array_field\", \"array.field1\": \"$_id.array_field1\"}}"),
482+
"{\"$project\": {\"_id\": 0, \"testCollection__id\": \"$_id.testCollection__id\", \"field\": \"$_id.field\", \"field1\": \"$_id.field1\"}}"),
483483
result.getAggregateOperations().get(3));
484484
}
485485

@@ -777,7 +777,7 @@ void testQueryWithHaving() throws SQLException {
777777
Assertions.assertNotNull(result);
778778
Assertions.assertEquals(COLLECTION_NAME, result.getCollectionName());
779779
Assertions.assertEquals(3, result.getColumnMetaData().size());
780-
Assertions.assertEquals(8, result.getAggregateOperations().size());
780+
Assertions.assertEquals(6, result.getAggregateOperations().size());
781781
Assertions.assertEquals(
782782
BsonDocument.parse(
783783
"{ \"$unwind\": {"
@@ -793,33 +793,19 @@ void testQueryWithHaving() throws SQLException {
793793
result.getAggregateOperations().get(1));
794794
Assertions.assertEquals(
795795
BsonDocument.parse(
796-
"{\"$group\": {\"_id\": {\"_id\": \"$_id\", \"array_field\": \"$array.field\", \"array_field1\": \"$array.field1\"}, \"_f3\": {\"$sum\": 1}}}"),
796+
"{\"$group\": {\"_id\": {\"testCollection__id\": \"$_id\", \"field\": \"$array.field\", \"field1\": \"$array.field1\"}, \"_f3\": {\"$sum\": 1}}}"),
797797
result.getAggregateOperations().get(2));
798798
Assertions.assertEquals(
799799
BsonDocument.parse(
800-
"{\"$project\": {\"_id\": \"$_id._id\", \"array.field\": \"$_id.array_field\", \"array.field1\": \"$_id.array_field1\", \"_f3\": \"$_f3\"}}"),
800+
"{\"$project\": {\"_id\": 0, \"testCollection__id\": \"$_id.testCollection__id\", \"field\": \"$_id.field\", \"field1\": \"$_id.field1\", \"_f3\": \"$_f3\"}}"),
801801
result.getAggregateOperations().get(3));
802802
Assertions.assertEquals(
803-
BsonDocument.parse(
804-
"{\"$project\": "
805-
+ "{\"_id\": 1, "
806-
+ "\"array.field\": 1, "
807-
+ "\"array.field1\": 1, \""
808-
+ "_f3\": 1, "
809-
+ DocumentDbFilter.BOOLEAN_FLAG_FIELD
810-
+ ": {\"$cond\": [{\"$and\": ["
811-
+ "{\"$gt\": [\"$_f3\", null]}, "
812-
+ "{\"$gt\": [{\"$literal\": 1}, null]}]}, "
813-
+ "{\"$gt\": [\"$_f3\", {\"$literal\": 1}]}, null]}}}"),
803+
BsonDocument.parse("{\"$match\": {\"_f3\": {\"$gt\": 1}}}"),
814804
result.getAggregateOperations().get(4));
815805
Assertions.assertEquals(
816806
BsonDocument.parse(
817-
"{\"$match\": {" + DocumentDbFilter.BOOLEAN_FLAG_FIELD + ": {\"$eq\": true}}}"),
807+
"{\"$project\": {\"testCollection__id\": \"$testCollection__id\", \"field\": \"$field\", \"field1\": \"$field1\", \"_id\": 0}}"),
818808
result.getAggregateOperations().get(5));
819-
Assertions.assertEquals(
820-
BsonDocument.parse(
821-
"{\"$project\": {" + DocumentDbFilter.BOOLEAN_FLAG_FIELD + ": 0}}"),
822-
result.getAggregateOperations().get(6));
823809
}
824810

825811
@Test
@@ -861,21 +847,20 @@ void testComplexQuery() throws SQLException {
861847
result.getAggregateOperations().get(2));
862848
Assertions.assertEquals(
863849
BsonDocument.parse(
864-
"{\"$group\": " +
865-
"{\"_id\": {\"_id\": \"$_id\", " +
866-
"\"array_field\": \"$array.field\", \"array_field1\": \"$array.field1\"}, " +
867-
"\"Total\": {\"$sum\": 1}}}"),
850+
"{\"$group\": {"
851+
+ "\"_id\": {\"testCollection__id\": \"$_id\", \"field\": \"$array.field\", \"field1\": \"$array.field1\"}, "
852+
+ "\"Total\": {\"$sum\": 1}}}"),
868853
result.getAggregateOperations().get(3));
869854
Assertions.assertEquals(
870-
BsonDocument.parse("{\"$project\": {\"_id\": \"$_id._id\", " +
871-
"\"array.field\": \"$_id.array_field\", \"array.field1\": \"$_id.array_field1\", \"Total\": \"$Total\"}}"),
855+
BsonDocument.parse(
856+
"{\"$project\": {\"_id\": 0, \"testCollection__id\": \"$_id.testCollection__id\", \"field\": \"$_id.field\", \"field1\": \"$_id.field1\", \"Total\": \"$Total\"}}"),
872857
result.getAggregateOperations().get(4));
873858
Assertions.assertEquals(
874859
BsonDocument.parse("{\"$match\": {\"Total\": {\"$gt\": 1}}}"),
875860
result.getAggregateOperations().get(5));
876861
Assertions.assertEquals(
877862
BsonDocument.parse(
878-
"{\"$project\": {\"testCollection__id\": \"$_id\", \"renamed\": \"$array.field\", \"Total\": \"$Total\", \"_id\": 0}}"),
863+
"{\"$project\": {\"testCollection__id\": \"$testCollection__id\", \"renamed\": \"$field\", \"Total\": \"$Total\", \"_id\": 0}}"),
879864
result.getAggregateOperations().get(6));
880865
Assertions.assertEquals(
881866
BsonDocument.parse("{\"$sort\": {\"renamed\": 1}}"),
@@ -1074,18 +1059,18 @@ void testComplexQueryWithSameCollectionJoin() throws SQLException {
10741059
result.getAggregateOperations().get(3));
10751060
Assertions.assertEquals(
10761061
BsonDocument.parse(
1077-
"{\"$group\": {\"_id\": {\"_id\": \"$_id\", \"array_field\": \"$array.field\", \"array_field1\": \"$array.field1\"}, \"Total\": {\"$sum\": 1}}}"),
1062+
"{\"$group\": {\"_id\": {\"testCollection__id\": \"$_id\", \"field\": \"$array.field\", \"field1\": \"$array.field1\"}, \"Total\": {\"$sum\": 1}}}"),
10781063
result.getAggregateOperations().get(4));
10791064
Assertions.assertEquals(
10801065
BsonDocument.parse(
1081-
"{\"$project\": {\"_id\": \"$_id._id\", \"array.field\": \"$_id.array_field\", \"array.field1\": \"$_id.array_field1\", \"Total\": \"$Total\"}}"),
1066+
"{\"$project\": {\"_id\": 0, \"testCollection__id\": \"$_id.testCollection__id\", \"field\": \"$_id.field\", \"field1\": \"$_id.field1\", \"Total\": \"$Total\"}}"),
10821067
result.getAggregateOperations().get(5));
10831068
Assertions.assertEquals(
10841069
BsonDocument.parse("{\"$match\": {\"Total\": {\"$gt\": 1}}}"),
10851070
result.getAggregateOperations().get(6));
10861071
Assertions.assertEquals(
10871072
BsonDocument.parse(
1088-
"{\"$project\": {\"renamed\": \"$array.field\", \"Total\": \"$Total\", \"_id\": 0}}"),
1073+
"{\"$project\": {\"renamed\": \"$field\", \"Total\": \"$Total\", \"_id\": 0}}"),
10891074
result.getAggregateOperations().get(7));
10901075
Assertions.assertEquals(
10911076
BsonDocument.parse("{\"$sort\": {\"renamed\": 1}}"),
@@ -1295,18 +1280,18 @@ void testComplexQueryWithDifferentCollectionJoin() throws SQLException {
12951280
result.getAggregateOperations().get(2));
12961281
Assertions.assertEquals(
12971282
BsonDocument.parse(
1298-
"{\"$group\": {\"_id\": {\"_id\": \"$_id\", \"testCollection_array_array_field\": \"$testCollection_array.array.field\", \"testCollection_array_array_field1\": \"$testCollection_array.array.field1\"}, \"Total\": {\"$sum\": 1}}}"),
1283+
"{\"$group\": {\"_id\": {\"otherTestCollection__id\": \"$_id\", \"field\": \"$testCollection_array.array.field\", \"field1\": \"$testCollection_array.array.field1\"}, \"Total\": {\"$sum\": 1}}}"),
12991284
result.getAggregateOperations().get(3));
13001285
Assertions.assertEquals(
13011286
BsonDocument.parse(
1302-
"{\"$project\": {\"_id\": \"$_id._id\", \"testCollection_array.array.field\": \"$_id.testCollection_array_array_field\", \"testCollection_array.array.field1\": \"$_id.testCollection_array_array_field1\", \"Total\": \"$Total\"}}"),
1287+
"{\"$project\": {\"_id\": 0, \"otherTestCollection__id\": \"$_id.otherTestCollection__id\", \"field\": \"$_id.field\", \"field1\": \"$_id.field1\", \"Total\": \"$Total\"}}"),
13031288
result.getAggregateOperations().get(4));
13041289
Assertions.assertEquals(
13051290
BsonDocument.parse("{\"$match\": {\"Total\": {\"$gt\": 1}}}"),
13061291
result.getAggregateOperations().get(5));
13071292
Assertions.assertEquals(
13081293
BsonDocument.parse(
1309-
"{\"$project\": {\"renamed\": \"$testCollection_array.array.field\", \"Total\": \"$Total\", \"_id\": 0}}"),
1294+
"{\"$project\": {\"renamed\": \"$field\", \"Total\": \"$Total\", \"_id\": 0}}"),
13101295
result.getAggregateOperations().get(6));
13111296
Assertions.assertEquals(
13121297
BsonDocument.parse("{\"$sort\": {\"renamed\": 1}}"),

0 commit comments

Comments
 (0)