Skip to content

Conversation

ankitsultana
Copy link
Contributor

@ankitsultana ankitsultana commented Sep 26, 2024

We don't really need to build the TimeSeriesBuilders until we return from the LeafTimeSeriesOperator, since there are no time-series related operations under and including the TimeSeriesCombineOperator.

This PR changes the TimeSeriesResultsBlock so that instead of keeping a TimeSeriesBlock, it keeps a new TimeSeriesBuilderBlock, which keeps the un-built series in a map keyed by the series id.

This also enables queries with complex aggregations like Percentile, Rate, etc. (until we support Exchange/Shuffles).

Additionally:

  • I have also renamed the ScanFilterAndProjectNode to the more appropriate LeafTimeSeriesPlanNode, since that node technically also corresponds to an aggregation.
  • Changed TimeSeriesLogicalPlanner#init to take in PinotConfiguration
  • Fixed a log message in PinotClientRequest

@ankitsultana ankitsultana reopened this Sep 26, 2024
@codecov-commenter
Copy link

codecov-commenter commented Sep 26, 2024

Codecov Report

Attention: Patch coverage is 36.17021% with 30 lines in your changes missing coverage. Please review.

Project coverage is 64.04%. Comparing base (59551e4) to head (9513a54).
Report is 2264 commits behind head on master.

Files with missing lines Patch % Lines
...ombine/merger/TimeSeriesAggResultsBlockMerger.java 0.00% 8 Missing ⚠️
...ore/operator/blocks/results/ResultsBlockUtils.java 0.00% 5 Missing and 1 partial ⚠️
...ery/runtime/timeseries/LeafTimeSeriesOperator.java 0.00% 3 Missing ⚠️
...pinot/tsdb/planner/TimeSeriesQueryEnvironment.java 0.00% 3 Missing ⚠️
...e/pinot/tsdb/spi/series/BaseTimeSeriesBuilder.java 0.00% 3 Missing ⚠️
...t/core/operator/blocks/TimeSeriesBuilderBlock.java 84.61% 2 Missing ⚠️
...time/timeseries/PhysicalTimeSeriesPlanVisitor.java 0.00% 2 Missing ⚠️
.../pinot/tsdb/planner/physical/TableScanVisitor.java 0.00% 2 Missing ⚠️
...pinot/broker/api/resources/PinotClientRequest.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #14092      +/-   ##
============================================
+ Coverage     61.75%   64.04%   +2.28%     
- Complexity      207     1537    +1330     
============================================
  Files          2436     2596     +160     
  Lines        133233   143293   +10060     
  Branches      20636    21948    +1312     
============================================
+ Hits          82274    91765    +9491     
+ Misses        44911    44770     -141     
- Partials       6048     6758     +710     
Flag Coverage Δ
custom-integration1 100.00% <ø> (+99.99%) ⬆️
integration 100.00% <ø> (+99.99%) ⬆️
integration1 100.00% <ø> (+99.99%) ⬆️
integration2 ?
java-11 64.01% <36.17%> (+2.30%) ⬆️
java-21 63.87% <36.17%> (+2.24%) ⬆️
skip-bytebuffers-false 64.03% <36.17%> (+2.28%) ⬆️
skip-bytebuffers-true 63.85% <36.17%> (+36.12%) ⬆️
temurin 64.04% <36.17%> (+2.28%) ⬆️
unittests 64.03% <36.17%> (+2.28%) ⬆️
unittests1 55.60% <34.78%> (+8.71%) ⬆️
unittests2 34.51% <2.12%> (+6.77%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ankitsultana ankitsultana added the timeseries-engine Tracking tag for generic time-series engine work label Sep 27, 2024
@ankitsultana ankitsultana merged commit 4709954 into apache:master Sep 27, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
timeseries-engine Tracking tag for generic time-series engine work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants