Skip to content

Enhance and unify JSON index reader#16436

Merged
Jackie-Jiang merged 1 commit intoapache:masterfrom
Jackie-Jiang:enhance_json_index
Jul 28, 2025
Merged

Enhance and unify JSON index reader#16436
Jackie-Jiang merged 1 commit intoapache:masterfrom
Jackie-Jiang:enhance_json_index

Conversation

@Jackie-Jiang
Copy link
Contributor

Enhance and unify JSON index reader logic:

  • Enhance empty bitmap check to reduce unnecessary computation
  • Reduce unnecessary null checks
  • Introduce helper method to simplify handling
  • Make the logic the same for mutable and immutable

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances and unifies the JSON index reader logic to improve performance and maintainability by reducing unnecessary computations and simplifying the codebase.

Key changes include:

  • Enhanced empty bitmap checks to exit early and avoid unnecessary computations
  • Introduced consistent helper methods for bitmap operations that handle empty cases efficiently
  • Unified logic between mutable and immutable JSON index implementations

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
ImmutableJsonIndexReader.java Refactored bitmap operations with empty checks, unified helper methods, and extracted key-value matching logic
BitmapInvertedIndexReader.java Removed unnecessary @SuppressWarnings annotation
MutableJsonIndexImpl.java Unified LazyBitmap operations with empty checks, reorganized field declarations, and extracted key-value matching logic
Comments suppressed due to low confidence (3)

pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/json/ImmutableJsonIndexReader.java:166

  • [nitpick] The method name 'and' is ambiguous and could conflict with built-in language constructs. Consider renaming to 'intersect' or 'andBitmaps' for better clarity.
  private static ImmutableRoaringBitmap and(ImmutableRoaringBitmap target, ImmutableRoaringBitmap other) {

pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/json/ImmutableJsonIndexReader.java:181

  • [nitpick] The method name 'or' is ambiguous and could conflict with built-in language constructs. Consider renaming to 'union' or 'orBitmaps' for better clarity.
  private static ImmutableRoaringBitmap or(ImmutableRoaringBitmap target, ImmutableRoaringBitmap other) {

pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/json/ImmutableJsonIndexReader.java:199

  • [nitpick] The method name 'andNot' could be more descriptive. Consider renaming to 'subtract' or 'difference' for better clarity about the operation being performed.
  private static ImmutableRoaringBitmap andNot(ImmutableRoaringBitmap target, ImmutableRoaringBitmap other) {

@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2025

Codecov Report

❌ Patch coverage is 69.58333% with 73 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.19%. Comparing base (1a476de) to head (c621585).
⚠️ Report is 518 commits behind head on master.
⚠️ Test Analytics upload error: Unsupported file format

Files with missing lines Patch % Lines
...local/realtime/impl/json/MutableJsonIndexImpl.java 61.78% 30 Missing and 17 partials ⚠️
...t/index/readers/json/ImmutableJsonIndexReader.java 77.77% 14 Missing and 12 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #16436      +/-   ##
============================================
+ Coverage     62.90%   63.19%   +0.28%     
+ Complexity     1386     1363      -23     
============================================
  Files          2867     3003     +136     
  Lines        163354   174026   +10672     
  Branches      24952    26629    +1677     
============================================
+ Hits         102755   109970    +7215     
- Misses        52847    55677    +2830     
- Partials       7752     8379     +627     
Flag Coverage Δ
custom-integration1 100.00% <ø> (ø)
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 0.00% <ø> (ø)
java-11 63.14% <69.58%> (+0.27%) ⬆️
java-21 63.15% <69.58%> (+0.33%) ⬆️
skip-bytebuffers-false ?
skip-bytebuffers-true ?
temurin 63.19% <69.58%> (+0.28%) ⬆️
unittests 63.18% <69.58%> (+0.28%) ⬆️
unittests1 56.34% <35.41%> (+0.52%) ⬆️
unittests2 33.16% <67.91%> (-0.42%) ⬇️

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.

@Jackie-Jiang Jackie-Jiang merged commit 0bd90a1 into apache:master Jul 28, 2025
17 of 18 checks passed
@Jackie-Jiang Jackie-Jiang deleted the enhance_json_index branch July 28, 2025 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants