You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "xiangfu0 (via GitHub)" <gi...@apache.org> on 2025/11/27 12:10:37 UTC
[PR] Improve CustomDataQueryClusterIntegrationTest testbility [pinot]
xiangfu0 opened a new pull request, #17284:
URL: https://github.com/apache/pinot/pull/17284
1. Increase integration tests to use 2 servers and 2 segments to ensure the behavior of merge is expected.
2. Fix aggregation intermediate results merge
3. Fix StUnion object merge issue
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Improve CustomDataQueryClusterIntegrationTest testbility [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #17284:
URL: https://github.com/apache/pinot/pull/17284#discussion_r2580770713
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountULLAggregationFunction.java:
##########
@@ -309,7 +314,12 @@ public void aggregateGroupByMV(int length, int[][] groupKeysArray, GroupByResult
public UltraLogLog extractAggregationResult(AggregationResultHolder aggregationResultHolder) {
Object result = aggregationResultHolder.getResult();
if (result == null) {
- return UltraLogLog.create(_p);
+ if (_aggregateOnRawValues) {
+ return UltraLogLog.create(_p);
+ }
+ // Here we cannot directly return a default UltraLogLog object, as if the input is ULL bytes, which may use a
+ // different _p, then the ULL object merge will fail.
+ return null;
Review Comment:
then it requires users to explicitly specify the argument p in the function call
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix multiple aggregation functions(StUnion/SumArrayLong/SumArrayDouble/DistinctCountULL) intermediate results serde issue [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17284:
URL: https://github.com/apache/pinot/pull/17284#discussion_r2584580496
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/MapTypeTest.java:
##########
@@ -99,24 +98,23 @@ public List<File> createAvroFiles()
new org.apache.avro.Schema.Field(INT_KEY_MAP_FIELD_NAME, intKeyMapAvroSchema, null, null));
avroSchema.setFields(fields);
- File avroFile = new File(_tempDir, "data.avro");
- try (DataFileWriter<GenericData.Record> fileWriter = new DataFileWriter<>(new GenericDatumWriter<>(avroSchema))) {
- fileWriter.create(avroSchema, avroFile);
- for (int i = 0; i < NUM_DOCS; i++) {
+ try (AvroFilesAndWriters avroFilesAndWriters = createAvroFilesAndWriters(avroSchema)) {
+ for (int i = 0; i < NUM_DOCS_PER_SEGMENT; i++) {
Map<String, Integer> stringKeyMap = new HashMap<>();
stringKeyMap.put("k1", i);
- stringKeyMap.put("k2", NUM_DOCS + i);
+ stringKeyMap.put("k2", NUM_DOCS_PER_SEGMENT + i);
Map<Integer, String> intKeyMap = new HashMap<>();
intKeyMap.put(95, Integer.toString(i));
- intKeyMap.put(717, Integer.toString(NUM_DOCS + i));
+ intKeyMap.put(717, Integer.toString(NUM_DOCS_PER_SEGMENT + i));
GenericData.Record record = new GenericData.Record(avroSchema);
record.put(STRING_KEY_MAP_FIELD_NAME, stringKeyMap);
record.put(INT_KEY_MAP_FIELD_NAME, intKeyMap);
- fileWriter.append(record);
+ for (DataFileWriter<GenericData.Record> writer : avroFilesAndWriters.getWriters()) {
+ writer.append(record);
+ }
Review Comment:
This loop duplicates the same record across all writers instead of distributing data across segments. Unlike other test files that use `writers.get(i % getNumAvroFiles())` to distribute records, this creates identical data in each segment. This defeats the purpose of testing multi-segment scenarios.
```suggestion
avroFilesAndWriters.getWriters().get(i % getNumAvroFiles()).append(record);
```
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/JsonPathTest.java:
##########
@@ -123,13 +120,14 @@ public List<File> createAvroFiles()
Map.of("k4-k1", "value-k4-k1-" + i, "k4-k2", "value-k4-k2-" + i, "k4-k3", "value-k4-k3-" + i,
"met", i));
record.put(COMPLEX_MAP_STR_FIELD_NAME, JsonUtils.objectToString(complexMap));
- fileWriter.append(record);
+ for (DataFileWriter<GenericData.Record> writer : avroFilesAndWriters.getWriters()) {
+ writer.append(record);
+ }
Review Comment:
This loop duplicates the same record across all writers instead of distributing data across segments. This pattern differs from other test files which use `writers.get(i % getNumAvroFiles())` to distribute records properly.
```suggestion
DataFileWriter<GenericData.Record> writer = avroFilesAndWriters.getWriters().get(i % getNumAvroFiles());
writer.append(record);
```
##########
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:
This loop duplicates the same record across all writers instead of distributing data across segments. Other test files use `writers.get(i % getNumAvroFiles())` to properly distribute records across multiple segments.
```suggestion
avroFilesAndWriters.getWriters().get(i % getNumAvroFiles()).append(record);
```
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountULLAggregationFunction.java:
##########
@@ -338,12 +341,18 @@ public UltraLogLog extractGroupByResult(GroupByResultHolder groupByResultHolder,
}
@Override
- public UltraLogLog merge(UltraLogLog intermediateResult1, UltraLogLog intermediateResult2) {
- int largerP = Math.max(intermediateResult1.getP(), intermediateResult2.getP());
- UltraLogLog merged = UltraLogLog.create(largerP);
- merged.add(intermediateResult1);
- merged.add(intermediateResult2);
- return merged;
+ public UltraLogLog merge(@Nullable UltraLogLog intermediateResult1, @Nullable UltraLogLog intermediateResult2) {
+ if (intermediateResult1 == null) {
+ return intermediateResult2;
+ }
+ if (intermediateResult2 == null) {
+ return intermediateResult1;
+ }
+ // UltraLogLog object doesn't support merge a smaller P object into a larger p object.
Review Comment:
Inconsistent capitalization: 'P' and 'p' both refer to the same parameter. Should be 'merging a smaller p object into a larger p object' for consistency.
```suggestion
// UltraLogLog object doesn't support merging a smaller p object into a larger p object.
```
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix multiple aggregation functions(StUnion/SumArrayLong/SumArrayDouble/DistinctCountULL) intermediate results serde issue [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Improve CustomDataQueryClusterIntegrationTest testbility [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17284:
URL: https://github.com/apache/pinot/pull/17284#issuecomment-3585758570
### :x: 1 Tests Failed:
| Tests completed | Failed | Passed | Skipped |
|---|---|---|---|
| 9693 | 1 | 9692 | 19 |
<details><summary>View the full list of 1 :snowflake: flaky test(s)</summary>
> <pre><code class="language-python">org.apache.pinot.common.utils.tls.RenewableTlsUtilsTest::reloadSslFactoryWhenFileStoreChanges</code></pre>
> **Flake rate in main:** 95.08% (Passed 3 times, Failed 58 times)
> <details><summary>Stack Traces | 0.655s run time</summary>
>
> > <pre><code class="language-python">did not expect [sun.security.rsa.RSAPrivateCrtKeyImpl@771701d] but found [sun.security.rsa.RSAPrivateCrtKeyImpl@771701d]</code></pre>
>
> </details>
</details>
To view more test analytics, go to the [Test Analytics Dashboard](https://app.codecov.io/gh/apache/pinot/tests/xiangfu0%3Amake-integration-test-with-2-server-instances)
<sub>📋 Got 3 mins? [Take this short survey](https://forms.gle/22i53Qa1CySZjA6c7) to help us improve Test Analytics.</sub>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Improve CustomDataQueryClusterIntegrationTest testbility [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17284:
URL: https://github.com/apache/pinot/pull/17284#issuecomment-3587589125
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17284?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `27 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 33.84%. Comparing base ([`e084b5d`](https://app.codecov.io/gh/apache/pinot/commit/e084b5defa1857b03fa1b4ddd5ee5a7db8e40ca1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`b740aac`](https://app.codecov.io/gh/apache/pinot/commit/b740aac3313add8a7f1b5fe6e74d97bf44c9db8c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 8 commits behind head on master.
:white_check_mark: All tests successful. No failed tests found.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/17284?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...gregation/function/StUnionAggregationFunction.java](https://app.codecov.io/gh/apache/pinot/pull/17284?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fquery%2Faggregation%2Ffunction%2FStUnionAggregationFunction.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9TdFVuaW9uQWdncmVnYXRpb25GdW5jdGlvbi5qYXZh) | 0.00% | [12 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17284?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...untime/operator/MultistageAggregationExecutor.java](https://app.codecov.io/gh/apache/pinot/pull/17284?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2Foperator%2FMultistageAggregationExecutor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9NdWx0aXN0YWdlQWdncmVnYXRpb25FeGVjdXRvci5qYXZh) | 0.00% | [8 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17284?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...inot/query/runtime/operator/AggregateOperator.java](https://app.codecov.io/gh/apache/pinot/pull/17284?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2Foperator%2FAggregateOperator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9BZ2dyZWdhdGVPcGVyYXRvci5qYXZh) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17284?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...erator/blocks/results/AggregationResultsBlock.java](https://app.codecov.io/gh/apache/pinot/pull/17284?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Foperator%2Fblocks%2Fresults%2FAggregationResultsBlock.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9ibG9ja3MvcmVzdWx0cy9BZ2dyZWdhdGlvblJlc3VsdHNCbG9jay5qYXZh) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17284?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../function/DistinctCountULLAggregationFunction.java](https://app.codecov.io/gh/apache/pinot/pull/17284?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fquery%2Faggregation%2Ffunction%2FDistinctCountULLAggregationFunction.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9EaXN0aW5jdENvdW50VUxMQWdncmVnYXRpb25GdW5jdGlvbi5qYXZh) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17284?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (e084b5d) and HEAD (b740aac). Click for more details.
>
> <details><summary>HEAD has 10 uploads less than BASE</summary>
>
>| Flag | BASE (e084b5d) | HEAD (b740aac) |
>|------|------|------|
>|unittests1|2|0|
>|unittests|4|1|
>|temurin|4|2|
>|java-11|2|0|
>|unittests2|2|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17284 +/- ##
=============================================
- Coverage 63.19% 33.84% -29.36%
+ Complexity 1426 724 -702
=============================================
Files 3133 3133
Lines 185904 186095 +191
Branches 28401 28412 +11
=============================================
- Hits 117488 62982 -54506
- Misses 59342 117921 +58579
+ Partials 9074 5192 -3882
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17284/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/17284/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (?)` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/17284/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (?)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/17284/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/17284/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.84% <0.00%> (-29.33%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17284/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.84% <0.00%> (-29.36%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17284/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.84% <0.00%> (-29.36%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17284/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17284/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.84% <0.00%> (-0.05%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/17284?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix multiple aggregation functions(StUnion/SumArrayLong/SumArrayDouble/DistinctCountULL) intermediate results serde issue [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #17284:
URL: https://github.com/apache/pinot/pull/17284#discussion_r2608756846
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountULLAggregationFunction.java:
##########
@@ -338,12 +341,18 @@ public UltraLogLog extractGroupByResult(GroupByResultHolder groupByResultHolder,
}
@Override
- public UltraLogLog merge(UltraLogLog intermediateResult1, UltraLogLog intermediateResult2) {
- int largerP = Math.max(intermediateResult1.getP(), intermediateResult2.getP());
- UltraLogLog merged = UltraLogLog.create(largerP);
- merged.add(intermediateResult1);
- merged.add(intermediateResult2);
- return merged;
+ public UltraLogLog merge(@Nullable UltraLogLog intermediateResult1, @Nullable UltraLogLog intermediateResult2) {
Review Comment:
(minor) Annotate return as `@Nullable`, same for other classes
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Improve CustomDataQueryClusterIntegrationTest testbility [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17284:
URL: https://github.com/apache/pinot/pull/17284#discussion_r2572631316
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/ArrayTest.java:
##########
@@ -234,32 +233,41 @@ 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);
+ System.out.println("row = " + row);
Review Comment:
Remove debug statement `System.out.println` before committing. Use SLF4J logger instead if logging is needed.
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/ArrayTest.java:
##########
@@ -341,32 +349,39 @@ public void testListDistinctAggQueries(boolean useMultiStageQueryEngine)
query =
String.format("SELECT "
- + "listAgg(DISTINCT stringCol, ' | ') WITHIN GROUP (ORDER BY stringCol) "
- + "FROM %s LIMIT %d", getTableName(), getCountStarResult());
+ + "listAgg(DISTINCT 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() / 10);
- for (int j = 1; j < splits.length; j++) {
- assertTrue(splits[j].compareTo(splits[j - 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 > 1) {
+ assertEquals(splits.length, getCountStarResult());
+ for (int i = 1; i < splits.length; i++) {
+ assertTrue(splits[i].compareTo(splits[i - 1]) >= 0);
+ }
+ }
}
query =
String.format("SELECT "
- + "listAgg(DISTINCT cast(doubleCol AS VARCHAR), ' | ') WITHIN GROUP (ORDER BY doubleCol) "
- + "FROM %s LIMIT %d", getTableName(), getCountStarResult());
+ + "listAgg(DISTINCT 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() / 10);
- for (int j = 1; j < splits.length; j++) {
- assertTrue(Double.parseDouble(splits[j]) > Double.parseDouble(splits[j - 1]));
+ assertEquals(rows.size(), getCountStarResult() / 10);
+ for (int rowId = 0; rowId < getCountStarResult() / 10; rowId++) {
+ row = rows.get(rowId);
+ System.out.println("row = " + row);
Review Comment:
Remove debug statement `System.out.println` before committing. Use SLF4J logger instead if logging is needed.
```suggestion
```
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Improve CustomDataQueryClusterIntegrationTest testbility [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #17284:
URL: https://github.com/apache/pinot/pull/17284#discussion_r2580763551
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/array/SumArrayDoubleAggregationFunction.java:
##########
@@ -138,7 +138,7 @@ public DoubleArrayList merge(DoubleArrayList intermediateResult1, DoubleArrayLis
@Override
public DataSchema.ColumnDataType getIntermediateResultColumnType() {
- return DataSchema.ColumnDataType.DOUBLE_ARRAY;
+ return DataSchema.ColumnDataType.OBJECT;
Review Comment:
aggregation intermediate serde only handle int/long/double/string see: `AggregationFunctionUtils#getIntermediateResult(...)`
and
`AggregationResultsBlock#setIntermediateResult(...)`
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix multiple aggregation functions(StUnion/SumArrayLong/SumArrayDouble/DistinctCountULL) intermediate results serde issue [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #17284:
URL: https://github.com/apache/pinot/pull/17284
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Improve CustomDataQueryClusterIntegrationTest testbility [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #17284:
URL: https://github.com/apache/pinot/pull/17284#discussion_r2570091775
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountULLAggregationFunction.java:
##########
@@ -309,7 +309,7 @@ public void aggregateGroupByMV(int length, int[][] groupKeysArray, GroupByResult
public UltraLogLog extractAggregationResult(AggregationResultHolder aggregationResultHolder) {
Object result = aggregationResultHolder.getResult();
if (result == null) {
- return UltraLogLog.create(_p);
+ return null;
Review Comment:
default ULL object will cause merge failure if p is not the same as what the data has.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Improve CustomDataQueryClusterIntegrationTest testbility [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #17284:
URL: https://github.com/apache/pinot/pull/17284#discussion_r2578526494
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/array/SumArrayDoubleAggregationFunction.java:
##########
@@ -138,7 +138,7 @@ public DoubleArrayList merge(DoubleArrayList intermediateResult1, DoubleArrayLis
@Override
public DataSchema.ColumnDataType getIntermediateResultColumnType() {
- return DataSchema.ColumnDataType.DOUBLE_ARRAY;
+ return DataSchema.ColumnDataType.OBJECT;
Review Comment:
Why do we need to change this?
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/StUnionAggregationFunction.java:
##########
@@ -136,4 +137,18 @@ public DataSchema.ColumnDataType getFinalResultColumnType() {
public ByteArray extractFinalResult(Geometry geometry) {
return new ByteArray(GeometrySerializer.serialize(geometry));
}
+
+ private static Geometry union(Geometry left, Geometry right) {
Review Comment:
Add `@Nullable` annotation when applicable
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountULLAggregationFunction.java:
##########
@@ -309,7 +314,12 @@ public void aggregateGroupByMV(int length, int[][] groupKeysArray, GroupByResult
public UltraLogLog extractAggregationResult(AggregationResultHolder aggregationResultHolder) {
Object result = aggregationResultHolder.getResult();
if (result == null) {
- return UltraLogLog.create(_p);
+ if (_aggregateOnRawValues) {
+ return UltraLogLog.create(_p);
+ }
+ // Here we cannot directly return a default UltraLogLog object, as if the input is ULL bytes, which may use a
+ // different _p, then the ULL object merge will fail.
+ return null;
Review Comment:
Can we always return `null`? Return `null` conditionally can still cause problems, e.g. when consuming segment aggregates on raw values, while sealed segments aggregate on pre-aggregated values
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org