fix: where comparing old and new bitmap versions occurred when directly comparing bytes in join and group by operations. #19082

KKould · 2025-12-09T06:28:50Z

I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/

Summary

Following the pull request (#19041), the data structure used by Bitmap was changed from RoaringBitmap to HybridBitmap. Therefore, the serialization method changed. Even though deserialization is compatible with the old data, there are still instances where comparisons are made directly using bytes during group by or join operations. This can lead to inconsistent matching results for semantically identical data when comparing old and new bitmap data.

Tests

Unit Test
Logic Test
Benchmark Test
No Test - Explain why

Type of change

Bug Fix (non-breaking change which fixes an issue)
New Feature (non-breaking change which adds functionality)
Breaking Change (fix or feature that could cause existing functionality not to work as expected)
Documentation Update
Refactoring
Performance Improvement
Other (please describe):

This change is

Copilot

Pull request overview

This pull request fixes a critical bug where bitmap comparisons in join and group by operations could produce incorrect results when comparing data serialized with the old RoaringBitmap format against data using the new HybridBitmap format. The fix ensures all bitmap data is normalized to the hybrid encoding format before comparison operations.

Key changes:

Introduces a normalize_bitmap_column utility function that detects legacy-encoded bitmaps and converts them to the current hybrid format
Updates join and group by operations to normalize bitmap columns before performing byte-level comparisons
Adds comprehensive unit tests verifying that legacy and hybrid-encoded bitmaps with identical data produce identical hashes and comparison results

Reviewed changes

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

Show a summary per file

File	Description
src/query/expression/src/utils/mod.rs	Adds new `bitmap` utility module
src/query/expression/src/utils/bitmap.rs	Implements bitmap normalization logic to convert legacy RoaringBitmap format to HybridBitmap format
src/query/expression/src/aggregate/payload_row.rs	Updates bitmap serialization and matching logic to normalize bitmaps before comparison; adds test
src/query/expression/src/aggregate/group_hash.rs	Updates bitmap hashing logic to normalize bitmaps before computing hash values; adds test
src/query/expression/Cargo.toml	Adds `roaring` dependency for test cases
Cargo.lock	Updates lock file with `roaring` dependency

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/query/expression/src/utils/bitmap.rs

chatgpt-codex-connector · 2025-12-09T11:01:17Z

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Repo admins can enable using credits for code reviews in their settings.

KKould · 2025-12-09T11:01:21Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

src/query/expression/src/values.rs

forsaken628 · 2025-12-10T13:20:11Z

I have a feeling that it's not overly complicated here? Compatibility issues are something that will naturally diminish over time. I think it only needs to be dealt with where the old data is ingested, not all places.

It is recommended to refer to those locations where DataBlock::consume_convert_to_full appears.

KKould · 2025-12-11T03:17:48Z

Before commit adc9695, it was implemented as you described. However, following @sundy-li suggestion, we attempted to pre-serialize the Bitmap in this PR to address a series of previous issues. Please refer to the updated PR description for details.

BohuTANG · 2025-12-11T04:44:23Z

Before commit adc9695, it was implemented as you described. However, following @sundy-li suggestion, we attempted to pre-serialize the Bitmap in this PR to address a series of previous issues. Please refer to the updated PR description for details.

@codex address this feedback

chatgpt-codex-connector · 2025-12-11T04:45:03Z

Codex couldn't complete this request. Try again later.

src/query/expression/src/types/bitmap.rs

KKould · 2025-12-15T02:22:51Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

src/query/expression/src/utils/mod.rs

Copilot

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/query/expression/src/utils/bitmap.rs

src/query/expression/src/values.rs

src/query/expression/src/utils/bitmap.rs

src/query/expression/src/aggregate/payload_row.rs

…ly comparing bytes in join and group by operations.

Co-authored-by: Copilot <[email protected]>

forsaken628 · 2025-12-22T00:33:49Z

https://github.com/databendlabs/databend/blob/main/src/query/expression/src/kernels/group_by_hash/method.rs#L65 for join

KKould · 2025-12-22T08:50:44Z

@codex review

chatgpt-codex-connector

💡 Codex Review

databend/src/query/expression/src/aggregate/payload_row.rs

Lines 446 to 449 in f46faf0

    
           Column::Bitmap(v) | Column::Binary(v) | Column::Variant(v) | Column::Geometry(v) => { 
        
               self.match_validity_with(counts, validity, |row, row_ptr| unsafe { 
        
                   let value = BinaryType::index_column_unchecked(v, row.to_usize()); 
        
                   row_ptr.is_bytes_eq(col_offset, value)

Row comparison mismatches normalized and legacy bitmap bytes

Bitmap columns are normalized to the hybrid encoding when rows are serialized, but comparison still reads raw bytes from the incoming column and compares them byte-for-byte to the stored normalized bytes. When the input block contains legacy bitmap serialization, the normalized row data will no longer match the unnormalized column value, causing identical keys to be treated as distinct during row matching (e.g., duplicate groups or join mismatches).

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

src/query/expression/src/values.rs

KKould requested review from b41sh, Copilot, forsaken628 and sundy-li December 9, 2025 06:28

KKould self-assigned this Dec 9, 2025

github-actions bot added the pr-bugfix this PR patches a bug in codebase label Dec 9, 2025

Copilot started reviewing on behalf of KKould December 9, 2025 06:29 View session

Copilot AI reviewed Dec 9, 2025

View reviewed changes

src/query/expression/src/utils/bitmap.rs Outdated Show resolved Hide resolved

KKould marked this pull request as ready for review December 9, 2025 11:01

chatgpt-codex-connector bot reviewed Dec 9, 2025

View reviewed changes

src/query/expression/src/values.rs Show resolved Hide resolved

KKould force-pushed the fix/bitmap_bytes_eq branch from 817126e to fef969f Compare December 10, 2025 09:47

KKould force-pushed the fix/bitmap_bytes_eq branch 2 times, most recently from b24e07a to 94a7c19 Compare December 11, 2025 03:58

sundy-li approved these changes Dec 12, 2025

View reviewed changes

sundy-li reviewed Dec 12, 2025

View reviewed changes

src/query/expression/src/types/bitmap.rs Outdated Show resolved Hide resolved

KKould force-pushed the fix/bitmap_bytes_eq branch from 4eb2698 to 426629e Compare December 12, 2025 07:32

forsaken628 reviewed Dec 12, 2025

View reviewed changes

src/query/expression/src/types/bitmap.rs Outdated Show resolved Hide resolved

KKould force-pushed the fix/bitmap_bytes_eq branch 2 times, most recently from dd789cf to e19a667 Compare December 13, 2025 12:02

KKould requested review from Copilot, forsaken628 and sundy-li December 15, 2025 02:22

Copilot started reviewing on behalf of KKould December 15, 2025 02:23 View session

chatgpt-codex-connector bot reviewed Dec 15, 2025

View reviewed changes

src/query/expression/src/utils/mod.rs Show resolved Hide resolved

Copilot AI reviewed Dec 15, 2025

View reviewed changes

src/query/expression/src/utils/bitmap.rs Show resolved Hide resolved

src/query/expression/src/values.rs Show resolved Hide resolved

src/query/expression/src/utils/bitmap.rs Show resolved Hide resolved

src/query/expression/src/utils/bitmap.rs Show resolved Hide resolved

sundy-li approved these changes Dec 16, 2025

View reviewed changes

forsaken628 reviewed Dec 17, 2025

View reviewed changes

src/query/expression/src/aggregate/payload_row.rs Outdated Show resolved Hide resolved

KKould and others added 6 commits December 22, 2025 06:09

fix: where comparing old and new bitmap versions occurred when direct…

e1a8510

…ly comparing bytes in join and group by operations.

Update src/query/expression/src/utils/bitmap.rs

440dc59

Co-authored-by: Copilot <[email protected]>

chore: codefmt

78c7715

fix: PartialOrd for Scalar::Bitmap

b7947f9

chore: codefmt

bd18689

chore: normalize one row at a time.

2d908db

KKould force-pushed the fix/bitmap_bytes_eq branch 2 times, most recently from 60ad320 to ecf9907 Compare December 21, 2025 22:25

chore: codefmt

b654f5a

KKould force-pushed the fix/bitmap_bytes_eq branch from ecf9907 to b654f5a Compare December 21, 2025 22:30

fix: normalize bitmap hash keys for join

f46faf0

KKould requested a review from forsaken628 December 22, 2025 06:56

chatgpt-codex-connector bot reviewed Dec 22, 2025

View reviewed changes

src/query/expression/src/values.rs Show resolved Hide resolved

chore: fmt

5b45eaa

forsaken628 approved these changes Dec 22, 2025

View reviewed changes

KKould merged commit fac7a9d into databendlabs:main Dec 22, 2025
86 checks passed

	Column::Bitmap(v) \| Column::Binary(v) \| Column::Variant(v) \| Column::Geometry(v) => {
	self.match_validity_with(counts, validity, \|row, row_ptr\| unsafe {
	let value = BinaryType::index_column_unchecked(v, row.to_usize());
	row_ptr.is_bytes_eq(col_offset, value)

fix: where comparing old and new bitmap versions occurred when directly comparing bytes in join and group by operations. #19082

fix: where comparing old and new bitmap versions occurred when directly comparing bytes in join and group by operations. #19082

Uh oh!

Conversation

KKould commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Tests

Type of change

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

chatgpt-codex-connector bot commented Dec 9, 2025

Uh oh!

KKould commented Dec 9, 2025

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

forsaken628 commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KKould commented Dec 11, 2025

Uh oh!

BohuTANG commented Dec 11, 2025

Uh oh!

chatgpt-codex-connector bot commented Dec 11, 2025

Uh oh!

Uh oh!

Uh oh!

KKould commented Dec 15, 2025

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

forsaken628 commented Dec 22, 2025

Uh oh!

KKould commented Dec 22, 2025

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

KKould commented Dec 9, 2025 •

edited

Loading

forsaken628 commented Dec 10, 2025 •

edited

Loading