Skip to content

fix bit_count on arm - #25076

Merged
mergify[bot] merged 2 commits into
matrixorigin:mainfrom
daviszhen:0622-fix-bitcount-bug1
Jun 22, 2026
Merged

fix bit_count on arm#25076
mergify[bot] merged 2 commits into
matrixorigin:mainfrom
daviszhen:0622-fix-bitcount-bug1

Conversation

@daviszhen

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #25046

What this PR does / why we need it:

原因:

BIT_COUNT(negative DOUBLE) comment 是对的
我用最小程序复现了 /mnt/fastdata/temp2/matrixone/pkg/sql/plan/function/func_unary.go:605 的逻辑:

rounded := math.RoundToEven(val)
uint64(rounded)

结果:

amd64/go1.25.x:

-1.4 -> rounded=-1 -> uint64=0xffffffffffffffff -> bits=64
arm64/go1.25.4:

-1.4 -> rounded=-1 -> uint64=0 -> bits=0
所以 comment 里说的 ARM64 上“负浮点转 uint64 变成 0”我已经实测确认了。
这说明 bitCountFromFloat() 的实现确实有跨架构问题,不应该依赖这个转换行为。

修改方案: 先转in64, 再转uint64. 已经在arm64上验证了.

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-checked the current head. The fix is narrowly scoped and correct for the actual cross-arch bug: negative FLOAT/DOUBLE values are now rounded first and then converted through int64 before the uint64 bit-count path, which avoids the ARM-specific zeroing behavior while preserving the existing MinInt64 clamp. The added regression cases hit the exact problematic boundary values, and I do not see a remaining correctness issue to block on.

@mergify

mergify Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Queued — the merge queue status continues in this comment ↓.

@mergify mergify Bot added the queued label Jun 22, 2026
@mergify

mergify Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Merge Queue Status

  • Entered queue2026-06-22 12:46 UTC · Rule: main · triggered by rule Automatic queue on approval for main
  • Checks passed · in-place
  • Merged2026-06-22 14:00 UTC · at 03cd594ec6cc53574c35f4b4ceb6ce137f00cb2a · squash

This pull request spent 1 hour 14 minutes 20 seconds in the queue, including 1 hour 13 minutes 54 seconds running CI.

Required conditions to merge
  • #approved-reviews-by >= 1 [🛡 GitHub branch protection]
  • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
  • github-review-decision = APPROVED [🛡 GitHub branch protection]
  • any of [🛡 GitHub branch protection]:
    • check-success = Matrixone Compose CI / multi cn e2e bvt test docker compose(PESSIMISTIC)
    • check-neutral = Matrixone Compose CI / multi cn e2e bvt test docker compose(PESSIMISTIC)
    • check-skipped = Matrixone Compose CI / multi cn e2e bvt test docker compose(PESSIMISTIC)
  • any of [🛡 GitHub branch protection]:
    • check-success = Matrixone Standlone CI / Multi-CN e2e BVT Test on Linux/x64(LAUNCH, PROXY)
    • check-neutral = Matrixone Standlone CI / Multi-CN e2e BVT Test on Linux/x64(LAUNCH, PROXY)
    • check-skipped = Matrixone Standlone CI / Multi-CN e2e BVT Test on Linux/x64(LAUNCH, PROXY)
  • any of [🛡 GitHub branch protection]:
    • check-success = Matrixone Standlone CI / e2e BVT Test on Linux/x64(LAUNCH, PESSIMISTIC)
    • check-neutral = Matrixone Standlone CI / e2e BVT Test on Linux/x64(LAUNCH, PESSIMISTIC)
    • check-skipped = Matrixone Standlone CI / e2e BVT Test on Linux/x64(LAUNCH, PESSIMISTIC)
  • any of [🛡 GitHub branch protection]:
    • check-success = Matrixone CI / SCA Test on Ubuntu/x86
    • check-neutral = Matrixone CI / SCA Test on Ubuntu/x86
    • check-skipped = Matrixone CI / SCA Test on Ubuntu/x86
  • any of [🛡 GitHub branch protection]:
    • check-success = Matrixone CI / UT Test on Ubuntu/x86
    • check-neutral = Matrixone CI / UT Test on Ubuntu/x86
    • check-skipped = Matrixone CI / UT Test on Ubuntu/x86
  • any of [🛡 GitHub branch protection]:
    • check-success = Matrixone Compose CI / multi cn e2e bvt test docker compose(Optimistic/PUSH)
    • check-neutral = Matrixone Compose CI / multi cn e2e bvt test docker compose(Optimistic/PUSH)
    • check-skipped = Matrixone Compose CI / multi cn e2e bvt test docker compose(Optimistic/PUSH)
  • any of [🛡 GitHub branch protection]:
    • check-success = Matrixone Standlone CI / e2e BVT Test on Linux/x64(LAUNCH,Optimistic)
    • check-neutral = Matrixone Standlone CI / e2e BVT Test on Linux/x64(LAUNCH,Optimistic)
    • check-skipped = Matrixone Standlone CI / e2e BVT Test on Linux/x64(LAUNCH,Optimistic)
  • any of [🛡 GitHub branch protection]:
    • check-success = Matrixone Upgrade CI / Compatibility Test With Target on Linux/x64(LAUNCH)
    • check-neutral = Matrixone Upgrade CI / Compatibility Test With Target on Linux/x64(LAUNCH)
    • check-skipped = Matrixone Upgrade CI / Compatibility Test With Target on Linux/x64(LAUNCH)
  • any of [🛡 GitHub branch protection]:
    • check-success = Matrixone Utils CI / Coverage
    • check-neutral = Matrixone Utils CI / Coverage
    • check-skipped = Matrixone Utils CI / Coverage

@mergify
mergify Bot merged commit 7cbeacd into matrixorigin:main Jun 22, 2026
23 of 24 checks passed
@mergify mergify Bot removed the queued label Jun 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working size/XS Denotes a PR that changes [1, 9] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants