Skip to content

[fix](flinksql): fix elemet_at may coredump on arm#520

Open
yangzhg wants to merge 1 commit intobytedance:mainfrom
yangzhg:fix_core_on_arm
Open

[fix](flinksql): fix elemet_at may coredump on arm#520
yangzhg wants to merge 1 commit intobytedance:mainfrom
yangzhg:fix_core_on_arm

Conversation

@yangzhg
Copy link
Copy Markdown
Collaborator

@yangzhg yangzhg commented Apr 20, 2026

What problem does this PR solve?

fix elemet_at may coredump on arm

Type of Change

  • 🐛 Bug fix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • 🚀 Performance improvement (optimization)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
  • 🔨 Refactoring (no logic changes)
  • 🔧 Build/CI or Infrastructure changes
  • 📝 Documentation only

Description

This pull request focuses on improving the test coverage and error handling for the element_at function in Flink SQL, as well as making a minor fix in the SubscriptUtil utility. The main changes include adding a new test case to verify exception handling when a zero index is used across multiple rows, and ensuring proper creation of null constants in the utility code.

Performance Impact

  • No Impact: This change does not affect the critical path (e.g., build system, doc, error handling).

  • Positive Impact: I have run benchmarks.

    Click to view Benchmark Results
    Paste your google-benchmark or TPC-H results here.
    Before: 10.5s
    After:   8.2s  (+20%)
    
  • Negative Impact: Explained below (e.g., trade-off for correctness).

Release Note

Please describe the changes in this PR

Release Note:

Release Note:
- Fixed a crash in `elemet_at `.

Checklist (For Author)

  • I have added/updated unit tests (ctest).
  • I have verified the code with local build (Release/Debug).
  • I have run clang-format / linters.
  • (Optional) I have run Sanitizers (ASAN/TSAN) locally for complex C++ changes.
  • No need to test or manual test.

Breaking Changes

  • No

  • Yes (Description: ...)

    Click to view Breaking Changes
    Breaking Changes:
    - Description of the breaking change.
    - Possible solutions or workarounds.
    - Any other relevant information.
    

Copy link
Copy Markdown
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 addresses a crash in Flink SQL element_at on ARM by ensuring a valid (null constant) result vector is produced when a constant invalid index triggers an error, and it adds a regression test for the multi-row constant-zero-index case.

Changes:

  • Return a null-constant vector early when constant index validation fails for arrays in SubscriptUtil.
  • Add a new Flink SQL element_at unit test covering constant index 0 across multiple rows.

Reviewed changes

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

File Description
bolt/functions/lib/SubscriptUtil.h Early-return a null constant vector for constant invalid index error cases to avoid undefined behavior/crash.
bolt/functions/flinksql/tests/ElementAtTest.cpp Adds a multi-row regression test for constant zero index throwing.

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

Comment thread bolt/functions/flinksql/tests/ElementAtTest.cpp Outdated
Copy link
Copy Markdown
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

Fixes a crash in FlinkSQL element_at (notably on ARM) when a constant invalid index (e.g., 0) is applied across rows, by ensuring the array subscript path returns a correctly typed null constant vector when all rows are marked failed, and adds a regression test for the multi-row constant-zero-index case.

Changes:

  • Return a typed null constant vector from SubscriptImpl::applyArrayTyped when a constant index triggers a zero-subscript error for all selected rows.
  • Add a new FlinkSQL element_at unit test covering constant index 0 across multiple input rows.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
bolt/functions/lib/SubscriptUtil.h Prevents using uninitialized dictionary indices when constant index validation fails for all rows by returning a null constant vector early.
bolt/functions/flinksql/tests/ElementAtTest.cpp Adds a regression test to ensure constant 0 index throws consistently for multi-row inputs.

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

@yangzhg yangzhg force-pushed the fix_core_on_arm branch 2 times, most recently from e08efbd to 08f3f33 Compare April 20, 2026 07:20
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