Skip to content

Add an SSE group-key generator provider - #19385

Draft
xiangfu0 wants to merge 1 commit into
apache:masterfrom
xiangfu0:xiangfu0/offheap-sse-provider-seam
Draft

Add an SSE group-key generator provider#19385
xiangfu0 wants to merge 1 commit into
apache:masterfrom
xiangfu0:xiangfu0/offheap-sse-provider-seam

Conversation

@xiangfu0

@xiangfu0 xiangfu0 commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add a narrow GroupKeyGeneratorProvider extension point for single-stage group-by planning
  • expose immutable physical group-key metadata through GroupKeyGeneratorContext
  • preserve the existing generator-selection hot path when the default provider is used
  • make custom generator ownership explicit across execution, materialization, combine, and failure paths

This is an extension seam only. The default provider is an identity sentinel, so existing queries keep the current built-in group-key generators and behavior.

Extension example

class CustomPlanMaker extends InstancePlanMakerImplV2 {
  @Override
  protected GroupKeyGeneratorProvider validateAndResolveGroupKeyGeneratorProvider(
      SegmentContext segmentContext, QueryContext queryContext) {
    return context -> {
      if (context.getGroupKeySpecs().size() != 1) {
        return Optional.empty();
      }
      return createSpecializedGenerator(context);
    };
  }
}

Providers return Optional.empty() for unsupported physical shapes. Every non-empty result must be a fresh, query-owned generator and participates in Pinot's close/ownership-transfer contract. A provider reused across plans or queries must be thread-safe.

The plan-maker hook is also called for filtered group-by queries so an override can validate policy. Filtered execution continues to use Pinot's shared built-in generator and ignores the returned provider.

Validation

  • 28 focused pinot-core group-by/provider/lifecycle tests
  • real-segment parity for RAW, RAW with a side dictionary, dictionary fallback, and null-handling fallback
  • owned-versus-borrowed StarTree initialization failure and suppressed-close coverage
  • formatting, Checkstyle, and license checks for pinot-core

Scope

The provider is currently LimitedPrivate("StarTree") and Unstable. This PR does not add an off-heap implementation or a new query/config option; those live in the dependent StarTree change.

@codecov-commenter

codecov-commenter commented Aug 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.53414% with 41 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.58%. Comparing base (bbbed25) to head (148993d).

Files with missing lines Patch % Lines
.../core/operator/combine/GroupByCombineOperator.java 57.50% 16 Missing and 1 partial ⚠️
...ry/aggregation/groupby/DefaultGroupByExecutor.java 82.97% 9 Missing and 7 partials ⚠️
...che/pinot/core/operator/query/GroupByOperator.java 80.00% 3 Missing and 1 partial ⚠️
...t/core/operator/query/FilteredGroupByOperator.java 85.71% 1 Missing and 2 partials ⚠️
...ore/startree/executor/StarTreeGroupByExecutor.java 92.85% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #19385      +/-   ##
============================================
+ Coverage     57.71%   67.58%   +9.87%     
- Complexity        7     1430    +1423     
============================================
  Files          2686     3488     +802     
  Lines        163987   224360   +60373     
  Branches      26627    35408    +8781     
============================================
+ Hits          94640   151637   +56997     
+ Misses        61352    60695     -657     
- Partials       7995    12028    +4033     
Flag Coverage Δ
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 0.00% <ø> (ø)
java-25 67.58% <83.53%> (+9.87%) ⬆️
lane-a 100.00% <ø> (ø)
lane-b 0.00% <ø> (ø)
temurin 67.58% <83.53%> (+9.87%) ⬆️
unittests 67.58% <83.53%> (+9.87%) ⬆️
unittests1 57.71% <83.53%> (+<0.01%) ⬆️
unittests2 39.29% <0.00%> (?)

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

☔ View full report in Codecov by Harness.
📢 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.

@xiangfu0
xiangfu0 force-pushed the xiangfu0/offheap-sse-provider-seam branch from 4b274fd to 98f8ec5 Compare August 29, 2026 09:07
Downstream Pinot distributions need a narrow extension point to supply specialized group-key generators without replacing the SSE group-by pipeline.

Expose immutable physical metadata at executor construction while preserving the default provider identity fast path and built-in selection. Keep filtered aggregations on the shared built-in generator while still allowing plan-level policy validation.

Make generator ownership explicit across raw-result transfer, combine, materialization, and partial-construction failures so resource-owning implementations are closed exactly once. The extension remains LimitedPrivate and Unstable, and does not add an implementation or query option.
@xiangfu0
xiangfu0 force-pushed the xiangfu0/offheap-sse-provider-seam branch from 98f8ec5 to 148993d Compare August 29, 2026 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants