raghavyadav01 commented on code in PR #14501:
URL: https://github.com/apache/pinot/pull/14501#discussion_r1854360422
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/combine/merger/TimeSeriesAggResultsBlockMerger.java:
##########
@@ -44,6 +49,14 @@ public void mergeResultsBlocks(TimeSeriesResultsBlock
mergedBlock, TimeSeriesRes
BaseTimeSeriesBuilder newTimeSeriesToMerge = entry.getValue();
if (currentTimeSeriesBuilder == null) {
currentTimeSeriesBlock.getSeriesBuilderMap().put(seriesHash,
newTimeSeriesToMerge);
+ final long currentUniqueSeries =
currentTimeSeriesBlock.getSeriesBuilderMap().size();
+ final long numBuckets =
currentTimeSeriesBlock.getTimeBuckets().getNumBuckets();
+ Preconditions.checkState(currentUniqueSeries * numBuckets <=
_maxDataPointsLimit,
Review Comment:
Is the maxDataPointsLimit on total data points across series?
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/combine/merger/TimeSeriesAggResultsBlockMerger.java:
##########
@@ -44,6 +49,14 @@ public void mergeResultsBlocks(TimeSeriesResultsBlock
mergedBlock, TimeSeriesRes
BaseTimeSeriesBuilder newTimeSeriesToMerge = entry.getValue();
if (currentTimeSeriesBuilder == null) {
currentTimeSeriesBlock.getSeriesBuilderMap().put(seriesHash,
newTimeSeriesToMerge);
+ final long currentUniqueSeries =
currentTimeSeriesBlock.getSeriesBuilderMap().size();
+ final long numBuckets =
currentTimeSeriesBlock.getTimeBuckets().getNumBuckets();
+ Preconditions.checkState(currentUniqueSeries * numBuckets <=
_maxDataPointsLimit,
+ "Max data points limit reached in combine operator. Limit: %s.
Current count: %s",
+ _maxDataPointsLimit, currentUniqueSeries * numBuckets);
Review Comment:
Is the behavior that when maxSeries or dataPoint limit is hit query will
return failure?
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/TimeSeriesPlanNode.java:
##########
@@ -66,7 +66,8 @@ _segmentContext, _queryContext,
getProjectPlanNodeExpressions(), DocIdSetPlanNod
getGroupByColumns(),
_timeSeriesContext.getTimeBuckets(),
projectionOperator,
- _seriesBuilderFactory);
+ _seriesBuilderFactory,
+ _segmentContext.getIndexSegment().getSegmentMetadata());
Review Comment:
indexes on the column will work as it works in V1 or MSE for time series
engine as well correct?
Can you share what is the impact of this change?
##########
pinot-timeseries/pinot-timeseries-spi/src/main/java/org/apache/pinot/tsdb/spi/series/TimeSeriesBuilderFactory.java:
##########
@@ -25,12 +25,26 @@
public abstract class TimeSeriesBuilderFactory {
+ protected static final int DEFAULT_MAX_UNIQUE_SERIES_PER_SERVER_LIMIT =
100_000;
Review Comment:
Does it mean PromQL plugin will have to set these values to be effective?
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/timeseries/TimeSeriesDispatchClient.java:
##########
@@ -29,16 +29,19 @@
/**
* Dispatch client used to dispatch a runnable plan to the server.
- * TODO: This shouldn't exist and we should re-use DispatchClient. TBD as part
of multi-stage
+ * TODO(timeseries): This shouldn't exist and we should re-use DispatchClient.
TBD as part of multi-stage
* engine integration.
*/
public class TimeSeriesDispatchClient {
+ // TODO(timeseries): Note that time-series engine at present uses
QueryServer for data transfer from server to broker.
+ // This will be fixed as we integrate with MSE.
+ private static final int INBOUND_SIZE_LIMIT = 256 * 1024 * 1024;
Review Comment:
Hopefully once we move to MSE integration this issue of managing grpc size
goes away.
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
