Copilot commented on code in PR #17284:
URL: https://github.com/apache/pinot/pull/17284#discussion_r2586940783
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/MapFieldTypeTest.java:
##########
@@ -155,10 +152,12 @@ public List<File> createAvroFiles()
GenericData.Record record = new GenericData.Record(avroSchema);
record.put(STRING_MAP_FIELD_NAME, stringMap);
record.put(INT_MAP_FIELD_NAME, intMap);
- fileWriter.append(record);
+ for (DataFileWriter<GenericData.Record> writer :
avroFilesAndWriters.getWriters()) {
+ writer.append(record);
+ }
Review Comment:
Each record is written to all writers, duplicating records across all Avro
files instead of distributing them. This differs from the pattern used in other
test files and causes the total count to be multiplied by the number of files,
which is why test assertions needed modifications to account for this
duplication.
```suggestion
// Distribute records across writers instead of duplicating to all
List<DataFileWriter<GenericData.Record>> writers =
avroFilesAndWriters.getWriters();
writers.get(i % writers.size()).append(record);
```
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/ArrayTest.java:
##########
@@ -234,32 +233,40 @@ public void testListAggQueries(boolean
useMultiStageQueryEngine)
query =
String.format("SELECT "
- + "listAgg(stringCol, ' | ') WITHIN GROUP (ORDER BY stringCol) "
- + "FROM %s LIMIT %d", getTableName(), getCountStarResult());
+ + "listAgg(stringCol, ' | ') WITHIN GROUP (ORDER BY stringCol),
intCol "
+ + "FROM %s GROUP BY intCol LIMIT %d", getTableName(),
getCountStarResult());
jsonNode = postQuery(query);
rows = jsonNode.get("resultTable").get("rows");
- assertEquals(rows.size(), 1);
- row = rows.get(0);
- assertEquals(row.size(), 1);
- String[] splits = row.get(0).asText().split(" \\| ");
- assertEquals(splits.length, getCountStarResult());
- for (int i = 1; i < splits.length; i++) {
- assertTrue(splits[i].compareTo(splits[i - 1]) >= 0);
+ assertEquals(rows.size(), getCountStarResult() / 10);
+ for (int rowId = 0; rowId < getCountStarResult() / 10; rowId++) {
+ row = rows.get(rowId);
+ assertEquals(row.size(), 2);
+ String[] splits = row.get(0).asText().split(" \\| ");
+ if (splits.length > 0) {
+ assertEquals(splits.length, 10);
+ for (int i = 1; i < splits.length; i++) {
+ assertTrue(splits[i].compareTo(splits[i - 1]) >= 0);
+ }
+ }
Review Comment:
The condition `if (splits.length > 0)` is always true for non-empty strings
after split. If the intention was to check for meaningful splits, use
`splits.length > 1`. Additionally, this check is redundant because the
immediate next line asserts `splits.length == 10`, which would fail if the
condition is needed.
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/MapFieldTypeTest.java:
##########
@@ -178,9 +177,9 @@ public void testQueries(boolean useMultiStageQueryEngine)
for (int i = 0; i < getSelectionDefaultDocCount(); i++) {
JsonNode intMap = rows.get(i).get(0);
JsonNode stringMap = rows.get(i).get(1);
- for (int j = 0; j < i; j++) {
- assertEquals(intMap.get("k" + j).intValue(), i);
- assertEquals(stringMap.get("k" + j).textValue(), "v" + i);
+ for (int j = 0; j < i % NUM_DOCS_PER_SEGMENT; j++) {
+ assertEquals(intMap.get("k" + j).intValue(), i % NUM_DOCS_PER_SEGMENT);
+ assertEquals(stringMap.get("k" + j).textValue(), "v" + i %
NUM_DOCS_PER_SEGMENT);
Review Comment:
Operator precedence issue: the expression evaluates as `(\"v\" + i) %
NUM_DOCS_PER_SEGMENT` instead of the intended `\"v\" + (i %
NUM_DOCS_PER_SEGMENT)`. Wrap the modulo operation in parentheses: `\"v\" + (i %
NUM_DOCS_PER_SEGMENT)`.
```suggestion
assertEquals(stringMap.get("k" + j).textValue(), "v" + (i %
NUM_DOCS_PER_SEGMENT));
```
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/ArrayTest.java:
##########
@@ -234,32 +233,40 @@ public void testListAggQueries(boolean
useMultiStageQueryEngine)
query =
String.format("SELECT "
- + "listAgg(stringCol, ' | ') WITHIN GROUP (ORDER BY stringCol) "
- + "FROM %s LIMIT %d", getTableName(), getCountStarResult());
+ + "listAgg(stringCol, ' | ') WITHIN GROUP (ORDER BY stringCol),
intCol "
+ + "FROM %s GROUP BY intCol LIMIT %d", getTableName(),
getCountStarResult());
jsonNode = postQuery(query);
rows = jsonNode.get("resultTable").get("rows");
- assertEquals(rows.size(), 1);
- row = rows.get(0);
- assertEquals(row.size(), 1);
- String[] splits = row.get(0).asText().split(" \\| ");
- assertEquals(splits.length, getCountStarResult());
- for (int i = 1; i < splits.length; i++) {
- assertTrue(splits[i].compareTo(splits[i - 1]) >= 0);
+ assertEquals(rows.size(), getCountStarResult() / 10);
+ for (int rowId = 0; rowId < getCountStarResult() / 10; rowId++) {
+ row = rows.get(rowId);
+ assertEquals(row.size(), 2);
+ String[] splits = row.get(0).asText().split(" \\| ");
+ if (splits.length > 0) {
+ assertEquals(splits.length, 10);
+ for (int i = 1; i < splits.length; i++) {
+ assertTrue(splits[i].compareTo(splits[i - 1]) >= 0);
+ }
+ }
}
query =
String.format("SELECT "
- + "listAgg(cast(doubleCol AS VARCHAR), ' | ') WITHIN GROUP (ORDER
BY doubleCol) "
- + "FROM %s LIMIT %d", getTableName(), getCountStarResult());
+ + "listAgg(cast(doubleCol AS VARCHAR), ' | ') WITHIN GROUP (ORDER
BY doubleCol), stringCol "
+ + "FROM %s GROUP BY stringCol LIMIT %d", getTableName(),
getCountStarResult());
jsonNode = postQuery(query);
rows = jsonNode.get("resultTable").get("rows");
- assertEquals(rows.size(), 1);
- row = rows.get(0);
- assertEquals(row.size(), 1);
- splits = row.get(0).asText().split(" \\| ");
- assertEquals(splits.length, getCountStarResult());
- for (int i = 1; i < splits.length; i++) {
- assertTrue(Double.parseDouble(splits[i]) >= Double.parseDouble(splits[i
- 1]));
+ assertEquals(rows.size(), getCountStarResult() / 10);
+ for (int rowId = 0; rowId < getCountStarResult() / 10; rowId++) {
+ row = rows.get(rowId);
+ assertEquals(row.size(), 2);
+ String[] splits = row.get(0).asText().split(" \\| ");
+ if (splits.length > 0) {
+ assertEquals(splits.length, 10);
+ for (int i = 1; i < splits.length; i++) {
+ assertTrue(splits[i].compareTo(splits[i - 1]) >= 0);
+ }
+ }
Review Comment:
The condition `if (splits.length > 0)` is redundant because it's always true
for any string split result. The subsequent assertion
`assertEquals(splits.length, 10)` would fail if this guard were necessary,
indicating the check serves no purpose.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
