Skip to content

[BugFix] Handle NOT NULL -> nullable flat-JSON columns in compaction read path (fix JsonMergeIterator CHECK crash) (backport #75680)#75749

Merged
kevincai merged 1 commit into
StarRocks:branch-4.0from
srihithg:backport-75680-branch-4.0
Jul 2, 2026
Merged

[BugFix] Handle NOT NULL -> nullable flat-JSON columns in compaction read path (fix JsonMergeIterator CHECK crash) (backport #75680)#75749
kevincai merged 1 commit into
StarRocks:branch-4.0from
srihithg:backport-75680-branch-4.0

Conversation

@srihithg

@srihithg srihithg commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Why I'm doing:

We hit a BE/CN crash loop on a shared-data cluster after a user ran
ALTER TABLE t MODIFY COLUMN c json NULL on a flat-JSON column that was originally NOT NULL.
Fast schema evolution makes that ALTER metadata-only, so the old segments on object storage are
never rewritten — they stay physically NOT NULL with no null sub-stream. The next background
compaction then reads those segments into a now-nullable output column and dies here:

CHECK((_null_iter == nullptr && null_column == nullptr) || (_null_iter != nullptr && null_column != nullptr));

The reader has _null_iter == nullptr (segment has no null stream) but the output is nullable
(null_column != nullptr), so the CHECK fires and SIGABRTs the whole process. Compaction retries,
crashes again, and the node never recovers. Reproduced on 4.0.12.

The thing is, this state is perfectly legal. A NOT NULL segment can't contain nulls, so the reader
just needs to fill the null map with "not null" instead of aborting.

What I'm doing:

Fix is read-side only, all in be/src/storage/rowset/json_column_iterator.cpp. No writer, FE,
thrift or proto changes.

All 6 read methods on JsonMergeIterator and JsonFlatColumnIterator (both next_batch
overloads and fetch_values_by_rowid on each) now handle the two mismatch cases instead of
CHECKing:

  • Segment NOT NULL, output nullable (the schema-evolution case): after the merge/read succeeds,
    append not-null flags to the output's null column for exactly the rows produced
    (json_column->size() - before), then update_has_null(). Same
    append_value_multiple_times(&NOT_NULL, n) idiom the flat-JSON writer already uses.
  • Segment nullable, output non-nullable: there's nowhere to put the null flags, so return
    Status::InternalError instead of killing the process.

While in there I fixed two adjacent problems in the same family:
JsonFlatColumnIterator::next_batch(size_t*) never had the CHECK at all and would silently
under-fill the null column in release builds, and
JsonFlatColumnIterator::fetch_values_by_rowid down_cast the destination to JsonColumn
whenever the segment had no null stream — UB if the destination was actually nullable. Also moved
check_or_die() behind the success path, since running it on a half-filled column after a failed
read would just abort in a different place.

Tests added to FlatJsonColumnCompactTest:

  • CompactJsonNotNullToNullableSchemaEvolution — NOT NULL flat segment read into a nullable
    column through all three read paths. This SIGABRTs without the fix. Asserts row count, that
    every synthesized null byte is 0, and that the JSON values round-trip.
  • CompactJsonMixedNullabilitySegments — NOT NULL + nullable + NOT NULL segments merged into one
    output, so the backfill also runs against a non-empty destination and sits next to a real NULL.
  • CompactJsonNullableSegmentToNonNullableOutputError — the inverse mismatch returns
    InternalError on all three paths instead of crashing.

Scope note: only the flat-JSON read paths (JsonMergeIterator / JsonFlatColumnIterator)
couple the segment's stored nullability to the output column like this. Non-flat JSON segments
are read through ScalarColumnIterator, and dynamic flattening (JsonDynamicFlatIterator)
reads through a clone_empty() proxy -- both maintain the output null column through the
Column append API and have no nullability-mismatch coupling, so they need no change.

The mismatch policy lives in two file-local helpers (read_json_null_stream /
finish_json_read) shared by all six read methods, so the planned ARRAY/MAP follow-up can
reuse the same shape instead of copy-pasting it.

One heads-up for reviewers: array_column_iterator.cpp (3x) and map_column_iterator.cpp (1x)
have the exact same CHECK and the same crash vector for ARRAY/MAP columns after the same kind of
ALTER. I'm deliberately not touching them here to keep this backportable and easy to review —
follow-up PR coming.

What type of PR is this:

  • BugFix
  • Feature
  • Enhancement
  • Refactor
  • UT
  • Doc
  • Tool

Does this PR entail a change in behavior?

  • Yes, this PR will result in a change in behavior.
  • No, this PR will not result in a change in behavior.

If yes, please specify the type of change:

  • Interface/UI changes: syntax, type conversion, expression evaluation, display information
  • Parameter changes: default values, similar parameters but with different default values
  • Policy changes: use new policy to replace old one, functionality automatically enabled
  • Feature removed
  • Miscellaneous: upgrade & downgrade compatibility, etc.

Checklist:

  • I have added test cases for my bug fix or my new feature
  • This pr needs user documentation (for new or modified features or behaviors)
    • I have added documentation for my new feature or new function
    • This pr needs auto generate documentation
  • This is a backport pr

Bugfix cherry-pick branch check:

  • I have checked the version labels which the pr will be auto-backported to the target branch
    • 4.1
    • 4.0
    • 3.5

(3.4 and 3.3 need manual backports — same as the earlier compaction crash fixes #63553 / #70539.)


This is a manual backport of pull request #75680 to branch-4.0. The automatic Mergify backport (#75724) could not apply cleanly because this branch predates the mutable-column accessor refactor, so the conflicts were resolved by hand (same adaptation in both: NullableColumn::data_column().get()/null_column().get() instead of the *_raw_ptr() accessors, and Columns-based test helpers). Verified locally: the full FlatJsonColumnCompactTest suite passes in an ASAN build on this branch.

…read path (fix JsonMergeIterator CHECK crash) (StarRocks#75680)

Signed-off-by: Srihith Garlapati <srihith.garlapati@gmail.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Srihith Garlapati <srihith.garlapati@gmail.com>
@kevincai
kevincai merged commit 73615b5 into StarRocks:branch-4.0 Jul 2, 2026
40 checks passed
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.

2 participants