You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "andimiller (via GitHub)" <gi...@apache.org> on 2023/06/27 17:24:21 UTC
[GitHub] [pinot] andimiller opened a new pull request, #10982: Allow extra aggregation types in RealtimeToOfflineSegmentsTask
andimiller opened a new pull request, #10982:
URL: https://github.com/apache/pinot/pull/10982
The only thing blocking these being used was the table validator in `TableConfigUtils` so I've updated it.
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10982: Allow extra aggregation types in RealtimeToOfflineSegmentsTask
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10982:
URL: https://github.com/apache/pinot/pull/10982#discussion_r1244407923
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java:
##########
@@ -1755,6 +1755,14 @@ public void testTaskConfig() {
} catch (IllegalStateException e) {
Assert.assertTrue(e.getMessage().contains("has invalid aggregate type"));
}
+
+ // valid agg
+ HashMap<String, String> validAggConfig = new HashMap<>(realtimeToOfflineTaskConfig);
Review Comment:
oh so we dont differentiate invalid agg vs valid agg with invalid configured agg?
ask in a different way: in what circumstances will distinctCountHLL not valid and can we add a test case for that?
--
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
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10982: Allow extra aggregation types in RealtimeToOfflineSegmentsTask
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10982:
URL: https://github.com/apache/pinot/pull/10982#discussion_r1248332662
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -42,6 +42,7 @@
import org.apache.pinot.common.request.context.RequestContextUtils;
import org.apache.pinot.common.tier.TierFactory;
import org.apache.pinot.common.utils.config.TagNameUtils;
+import org.apache.pinot.segment.local.aggregator.ValueAggregatorFactory;
Review Comment:
We should be able to use `ValueAggregatorFactory.getValueAggregator()`. `dataType` can be read from the schema
--
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
[GitHub] [pinot] andimiller commented on a diff in pull request #10982: Allow extra aggregation types in RealtimeToOfflineSegmentsTask
Posted by "andimiller (via GitHub)" <gi...@apache.org>.
andimiller commented on code in PR #10982:
URL: https://github.com/apache/pinot/pull/10982#discussion_r1247701457
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -42,6 +42,7 @@
import org.apache.pinot.common.request.context.RequestContextUtils;
import org.apache.pinot.common.tier.TierFactory;
import org.apache.pinot.common.utils.config.TagNameUtils;
+import org.apache.pinot.segment.local.aggregator.ValueAggregatorFactory;
Review Comment:
ah good catch, I forgot there was the second one, will rework it around that
--
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
[GitHub] [pinot] ksnijjer commented on pull request #10982: Allow extra aggregation types in RealtimeToOfflineSegmentsTask
Posted by "ksnijjer (via GitHub)" <gi...@apache.org>.
ksnijjer commented on PR #10982:
URL: https://github.com/apache/pinot/pull/10982#issuecomment-1610041218
@mayankshriv FYI
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10982: Allow extra aggregation types in RealtimeToOfflineSegmentsTask
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10982:
URL: https://github.com/apache/pinot/pull/10982#discussion_r1244327305
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java:
##########
@@ -1755,6 +1755,14 @@ public void testTaskConfig() {
} catch (IllegalStateException e) {
Assert.assertTrue(e.getMessage().contains("has invalid aggregate type"));
}
+
+ // valid agg
+ HashMap<String, String> validAggConfig = new HashMap<>(realtimeToOfflineTaskConfig);
Review Comment:
could we also add a test for invalid agg please?
--
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
[GitHub] [pinot] andimiller commented on a diff in pull request #10982: Allow extra aggregation types in RealtimeToOfflineSegmentsTask
Posted by "andimiller (via GitHub)" <gi...@apache.org>.
andimiller commented on code in PR #10982:
URL: https://github.com/apache/pinot/pull/10982#discussion_r1244444492
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java:
##########
@@ -1755,6 +1755,14 @@ public void testTaskConfig() {
} catch (IllegalStateException e) {
Assert.assertTrue(e.getMessage().contains("has invalid aggregate type"));
}
+
+ // valid agg
+ HashMap<String, String> validAggConfig = new HashMap<>(realtimeToOfflineTaskConfig);
Review Comment:
I've added a test case above for a valid `AggregationFunctionType` which has no `ValueAggregator` available in the `ValueAggregatorFactory`, I picked `histogram` for this because that doesn't have one right now, and I don't think would make sense to have one
--
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
[GitHub] [pinot] andimiller commented on a diff in pull request #10982: Allow extra aggregation types in RealtimeToOfflineSegmentsTask
Posted by "andimiller (via GitHub)" <gi...@apache.org>.
andimiller commented on code in PR #10982:
URL: https://github.com/apache/pinot/pull/10982#discussion_r1244438998
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java:
##########
@@ -1755,6 +1755,14 @@ public void testTaskConfig() {
} catch (IllegalStateException e) {
Assert.assertTrue(e.getMessage().contains("has invalid aggregate type"));
}
+
+ // valid agg
+ HashMap<String, String> validAggConfig = new HashMap<>(realtimeToOfflineTaskConfig);
Review Comment:
anything available in the `ValueAggregatorFactory` methods should be valid here, which is why I've implemented it this way
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10982: Allow extra aggregation types in RealtimeToOfflineSegmentsTask
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10982:
URL: https://github.com/apache/pinot/pull/10982#discussion_r1244407923
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java:
##########
@@ -1755,6 +1755,14 @@ public void testTaskConfig() {
} catch (IllegalStateException e) {
Assert.assertTrue(e.getMessage().contains("has invalid aggregate type"));
}
+
+ // valid agg
+ HashMap<String, String> validAggConfig = new HashMap<>(realtimeToOfflineTaskConfig);
Review Comment:
oh so we dont differentiate invalid agg vs valid agg with invalid configured agg?
ask in a different way: in what circumstances will distinctCountHLL not valid and can we add a test case for that? if nothing special, we can always simply extend the SUM/MAX/MIN enum list in the precondition check to include DISTINCTCOUNTHLL right?
--
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
[GitHub] [pinot] andimiller commented on a diff in pull request #10982: Allow extra aggregation types in RealtimeToOfflineSegmentsTask
Posted by "andimiller (via GitHub)" <gi...@apache.org>.
andimiller commented on code in PR #10982:
URL: https://github.com/apache/pinot/pull/10982#discussion_r1244444492
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java:
##########
@@ -1755,6 +1755,14 @@ public void testTaskConfig() {
} catch (IllegalStateException e) {
Assert.assertTrue(e.getMessage().contains("has invalid aggregate type"));
}
+
+ // valid agg
+ HashMap<String, String> validAggConfig = new HashMap<>(realtimeToOfflineTaskConfig);
Review Comment:
I've added a test case above for a valid `AggregationFunctionType` which has no `ValueAggregator` available in the `ValueAggregatorFactory`, I picked `histogram` for this because that doesn't have one right now, and I don't think would make sense to have one; people would probably just use the `KLL` sketch
--
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
[GitHub] [pinot] Jackie-Jiang merged pull request #10982: Allow extra aggregation types in RealtimeToOfflineSegmentsTask
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #10982:
URL: https://github.com/apache/pinot/pull/10982
--
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
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10982: Allow extra aggregation types in RealtimeToOfflineSegmentsTask
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10982:
URL: https://github.com/apache/pinot/pull/10982#discussion_r1253930820
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -103,7 +105,7 @@ private TableConfigUtils() {
// hardcode the value here to avoid pulling the entire pinot-kinesis module as dependency.
private static final String KINESIS_STREAM_TYPE = "kinesis";
private static final EnumSet<AggregationFunctionType> SUPPORTED_INGESTION_AGGREGATIONS =
- EnumSet.of(AggregationFunctionType.SUM, AggregationFunctionType.MIN, AggregationFunctionType.MAX,
+ EnumSet.of(AggregationFunctionType.SUM, MIN, AggregationFunctionType.MAX,
Review Comment:
```suggestion
EnumSet.of(SUM, MIN, MAX, COUNT);
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -483,6 +485,14 @@ static void validateDecoder(StreamConfig streamConfig) {
}
}
+ public final static Set<AggregationFunctionType> AVAILABLE_CORE_VALUE_AGGREGATORS = Set.of(
Review Comment:
Let's use `EnumSet` here, and reformat the change
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10982: Allow extra aggregation types in RealtimeToOfflineSegmentsTask
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10982:
URL: https://github.com/apache/pinot/pull/10982#discussion_r1244468924
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java:
##########
@@ -1755,6 +1755,14 @@ public void testTaskConfig() {
} catch (IllegalStateException e) {
Assert.assertTrue(e.getMessage().contains("has invalid aggregate type"));
}
+
+ // valid agg
+ HashMap<String, String> validAggConfig = new HashMap<>(realtimeToOfflineTaskConfig);
Review Comment:
Ah. I see. Ok that sounds good
--
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
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10982: Allow extra aggregation types in RealtimeToOfflineSegmentsTask
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10982:
URL: https://github.com/apache/pinot/pull/10982#discussion_r1247531397
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -42,6 +42,7 @@
import org.apache.pinot.common.request.context.RequestContextUtils;
import org.apache.pinot.common.tier.TierFactory;
import org.apache.pinot.common.utils.config.TagNameUtils;
+import org.apache.pinot.segment.local.aggregator.ValueAggregatorFactory;
Review Comment:
This is not the correct `ValueAggregatorFactory`. We should import `org.apache.pinot.core.segment.processing.aggregator.ValueAggregatorFactory` which is used for `RealtimeToOfflineTask`
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -523,8 +524,17 @@ static void validateTaskConfigs(TableConfig tableConfig, Schema schema) {
if (entry.getKey().endsWith(".aggregationType")) {
Preconditions.checkState(columnNames.contains(StringUtils.removeEnd(entry.getKey(), ".aggregationType")),
String.format("Column \"%s\" not found in schema!", entry.getKey()));
- Preconditions.checkState(ImmutableSet.of("SUM", "MAX", "MIN").contains(entry.getValue().toUpperCase()),
- String.format("Column \"%s\" has invalid aggregate type: %s", entry.getKey(), entry.getValue()));
+ try {
+ // check that it's a valid aggregation function type
+ AggregationFunctionType aft = AggregationFunctionType.valueOf(entry.getValue().toUpperCase());
Review Comment:
```suggestion
AggregationFunctionType aft = AggregationFunctionType.getAggregationFunctionType(entry.getValue());
```
--
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
[GitHub] [pinot] andimiller commented on a diff in pull request #10982: Allow extra aggregation types in RealtimeToOfflineSegmentsTask
Posted by "andimiller (via GitHub)" <gi...@apache.org>.
andimiller commented on code in PR #10982:
URL: https://github.com/apache/pinot/pull/10982#discussion_r1248857733
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -42,6 +42,7 @@
import org.apache.pinot.common.request.context.RequestContextUtils;
import org.apache.pinot.common.tier.TierFactory;
import org.apache.pinot.common.utils.config.TagNameUtils;
+import org.apache.pinot.segment.local.aggregator.ValueAggregatorFactory;
Review Comment:
> We should be able to use `ValueAggregatorFactory.getValueAggregator()`. `dataType` can be read from the schema
that one is in `pinot-core`, which isn't available in `pinot-segment-local`, I could add the dependency but that seemed like a big change
--
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
[GitHub] [pinot] andimiller commented on a diff in pull request #10982: Allow extra aggregation types in RealtimeToOfflineSegmentsTask
Posted by "andimiller (via GitHub)" <gi...@apache.org>.
andimiller commented on code in PR #10982:
URL: https://github.com/apache/pinot/pull/10982#discussion_r1244438998
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java:
##########
@@ -1755,6 +1755,14 @@ public void testTaskConfig() {
} catch (IllegalStateException e) {
Assert.assertTrue(e.getMessage().contains("has invalid aggregate type"));
}
+
+ // valid agg
+ HashMap<String, String> validAggConfig = new HashMap<>(realtimeToOfflineTaskConfig);
Review Comment:
anything available[ in the `ValueAggregatorFactory` methods](https://github.com/apache/pinot/blob/master/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/ValueAggregatorFactory.java#L40-L124) should be valid here, which is why I've implemented it this way
--
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
[GitHub] [pinot] andimiller commented on a diff in pull request #10982: Allow extra aggregation types in RealtimeToOfflineSegmentsTask
Posted by "andimiller (via GitHub)" <gi...@apache.org>.
andimiller commented on code in PR #10982:
URL: https://github.com/apache/pinot/pull/10982#discussion_r1266609847
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -483,6 +485,14 @@ static void validateDecoder(StreamConfig streamConfig) {
}
}
+ public final static Set<AggregationFunctionType> AVAILABLE_CORE_VALUE_AGGREGATORS = Set.of(
Review Comment:
done
--
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
[GitHub] [pinot] andimiller commented on a diff in pull request #10982: Allow extra aggregation types in RealtimeToOfflineSegmentsTask
Posted by "andimiller (via GitHub)" <gi...@apache.org>.
andimiller commented on code in PR #10982:
URL: https://github.com/apache/pinot/pull/10982#discussion_r1247760748
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -42,6 +42,7 @@
import org.apache.pinot.common.request.context.RequestContextUtils;
import org.apache.pinot.common.tier.TierFactory;
import org.apache.pinot.common.utils.config.TagNameUtils;
+import org.apache.pinot.segment.local.aggregator.ValueAggregatorFactory;
Review Comment:
I've gone back to using a `Set`, but used a `Set` of `AggregationFunctionType`, and added a comment in the core `ValueAggregatorFactory` to remind people to update this `Set`
--
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
[GitHub] [pinot] codecov-commenter commented on pull request #10982: Allow extra aggregation types in RealtimeToOfflineSegmentsTask
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10982:
URL: https://github.com/apache/pinot/pull/10982#issuecomment-1610021184
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10982?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#10982](https://app.codecov.io/gh/apache/pinot/pull/10982?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (bbfe20a) into [master](https://app.codecov.io/gh/apache/pinot/commit/2d30e28bedea29d264b0d7946ebb3f0e2cf1b5cc?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (2d30e28) will **decrease** coverage by `0.01%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10982 +/- ##
==========================================
- Coverage 0.11% 0.11% -0.01%
==========================================
Files 2190 2190
Lines 118056 118062 +6
Branches 17873 17873
==========================================
Hits 137 137
- Misses 117899 117905 +6
Partials 20 20
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1temurin11 | `0.00% <0.00%> (ø)` | |
| integration1temurin17 | `0.00% <0.00%> (ø)` | |
| integration1temurin20 | `0.00% <0.00%> (ø)` | |
| integration2temurin11 | `0.00% <0.00%> (ø)` | |
| integration2temurin17 | `0.00% <0.00%> (ø)` | |
| integration2temurin20 | `0.00% <0.00%> (ø)` | |
| unittests1temurin11 | `0.00% <0.00%> (ø)` | |
| unittests1temurin17 | `0.00% <0.00%> (ø)` | |
| unittests1temurin20 | `0.00% <0.00%> (ø)` | |
| unittests2temurin11 | `0.11% <0.00%> (ø)` | |
| unittests2temurin17 | `0.11% <0.00%> (ø)` | |
| unittests2temurin20 | `0.11% <0.00%> (ø)` | |
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.
| [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/10982?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...he/pinot/segment/local/utils/TableConfigUtils.java](https://app.codecov.io/gh/apache/pinot/pull/10982?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91dGlscy9UYWJsZUNvbmZpZ1V0aWxzLmphdmE=) | `0.00% <0.00%> (ø)` | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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
[GitHub] [pinot] andimiller commented on a diff in pull request #10982: Allow extra aggregation types in RealtimeToOfflineSegmentsTask
Posted by "andimiller (via GitHub)" <gi...@apache.org>.
andimiller commented on code in PR #10982:
URL: https://github.com/apache/pinot/pull/10982#discussion_r1244392095
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java:
##########
@@ -1755,6 +1755,14 @@ public void testTaskConfig() {
} catch (IllegalStateException e) {
Assert.assertTrue(e.getMessage().contains("has invalid aggregate type"));
}
+
+ // valid agg
+ HashMap<String, String> validAggConfig = new HashMap<>(realtimeToOfflineTaskConfig);
Review Comment:
it was already there, on the lines above
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10982: Allow extra aggregation types in RealtimeToOfflineSegmentsTask
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10982:
URL: https://github.com/apache/pinot/pull/10982#discussion_r1244407923
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java:
##########
@@ -1755,6 +1755,14 @@ public void testTaskConfig() {
} catch (IllegalStateException e) {
Assert.assertTrue(e.getMessage().contains("has invalid aggregate type"));
}
+
+ // valid agg
+ HashMap<String, String> validAggConfig = new HashMap<>(realtimeToOfflineTaskConfig);
Review Comment:
oh so we dont differentiate invalid agg vs valid agg with invalid configured agg?
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10982: Allow extra aggregation types in RealtimeToOfflineSegmentsTask
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10982:
URL: https://github.com/apache/pinot/pull/10982#discussion_r1244407923
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java:
##########
@@ -1755,6 +1755,14 @@ public void testTaskConfig() {
} catch (IllegalStateException e) {
Assert.assertTrue(e.getMessage().contains("has invalid aggregate type"));
}
+
+ // valid agg
+ HashMap<String, String> validAggConfig = new HashMap<>(realtimeToOfflineTaskConfig);
Review Comment:
oh so we dont differentiate invalid agg vs valid agg with invalid configured agg?
ask in a different way: in what circumstances will distinctCountHLL not valid and can we add a test case for that? --> otherwise we can always simply extend the SUM/MAX/MIN enum list in the precondition check rigth?
--
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