Codecov Report

Merging #8255 (2c4e35e) into master (d999aec) will increase coverage by 0.01%.
The diff coverage is 77.31%.

@@             Coverage Diff              @@
##             master    #8255      +/-   ##
============================================
+ Coverage     70.78%   70.79%   +0.01%     
- Complexity     4243     4317      +74     
============================================
  Files          1631     1690      +59     
  Lines         85281    88583    +3302     
  Branches      12845    13429     +584     
============================================
+ Hits          60364    62714    +2350     
- Misses        20746    21478     +732     
- Partials       4171     4391     +220     

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

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d999aec...2c4e35e. Read the comment docs.

Shall we add a queries test (search for *QueriesTest for examples) or integration test to ensure it works end-to-end?

I have added a test which prune segments based on query -
https://github.com/apache/pinot/pull/8255/files#diff-9e8214b130b1a4a54157230e84674df2328018e7e7044a90a1f2a114ad56674bR353

and Sure let me see if I can add an integration test.

Hi @Jackie-Jiang Could you please check this PR if it can be merge now?

@kmozaid This is a great feature, and I want to ensure it can work end-to-end. With the SegmentPrunerTest, it ensures the pruner itself works, but multiple partitioning could fail in other places (I remember some classes explicitly check if there is only one partition column). I'd suggest adding an integration test to ensure it works end-to-end. You may refer to NullHandlingIntegrationTest on how to add an integration test with custom data.

Thanks @Jackie-Jiang , Sure, Let me add integration test.

@kmozaid This is a great feature, and I want to ensure it can work end-to-end. With the SegmentPrunerTest, it ensures the pruner itself works, but multiple partitioning could fail in other places (I remember some classes explicitly check if there is only one partition column). I'd suggest adding an integration test to ensure it works end-to-end. You may refer to NullHandlingIntegrationTest on how to add an integration test with custom data.

Hi @Jackie-Jiang I have updated existing merge rollup minion task and realtime-to-offline minion task integration test which involves partitioning.Now these tests covered multi column partitioning flow. Please check if it is still required to write a separate integration tests.

There are three other places where multicolumn partitioning config is not allowed.

1. PinotLLCRealtimeSegmentManager.getPartitionMetadataFromTableConfig:710
2. LLRealtimeSegmentDataManager.setPartitionParameters:1425
3. PinotHadoopJobLauncher:HadoopSegmentPreprocessingJob.fetchPartitioningConfig:153

This feature is NOT support by REALTIME tables so first 2 checks are still valid. The last one is specific to Hadoop pre processing job, I think we can skip for this feature as we don't really support BoundedColumnValue partitioner for hadoop;s job (https://github.com/apache/pinot/blob/master/pinot-plugins/pinot-batch-ingestion/v0_deprecated/pinot-hadoop/src/main/java/org/apache/pinot/hadoop/job/partitioners/PartitionFunctionFactory.java#L78). It is deprecated too.

Test Cases Covered -

1. Integration tests for Realtime to Offline Task with multi column partitioning
2. Integration tests for Merge Rollup task with multi column partitioning.
3. Unit tests for direct offline segment creation from file with multi column partitioning (merge rollup task integration tests setup does this).

Hi @Jackie-Jiang , Could you please provide review feedback? please also trigger the workflow again.