Skip to content

GH-51780: [C++][IPC] Remove the mislabeled null check on schema fields in GetSchema - #51811

Merged
pitrou merged 1 commit into
apache:mainfrom
CaptainAni187:GH-51780-schema-field-null-check
Oct 7, 2026
Merged

pitrou merged 1 commit into
apache:mainfrom
CaptainAni187:GH-51780-schema-field-null-check

Conversation

@CaptainAni187

@CaptainAni187 CaptainAni187 commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Rationale for this change

In GetSchema, the loop over the schema's fields checked each Field for null with the label "DictionaryEncoding.indexType". That label belongs to a different check, so if this one ever failed, the error would point at the wrong field. As the XXX comment next to it said, the check also can't fail: for a vector of tables, fields()->Get(i) returns the element's address plus its stored offset, never null. Schema.fields itself is already checked for null just above.

What changes are included in this PR?

The check and its comment are removed, which is the first option in the issue. Child fields in FieldFromFlatbuffer are already read through children->Get(i) without a null check, so top-level fields now match.

Are these changes tested?

No new test, since the removed check couldn't fail. The arrow-ipc-* tests pass locally.

Are there any user-facing changes?

No.

Was AI used for this PR?

In accordance to the AI generation guidelines, please disclose below whether and how AI was used in this PR.

PR code and description written by:

  • Human
  • AI

Reviewed before submission by:

  • Human
  • AI
  • Not reviewed

@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown

⚠️ GitHub issue #51780 has been automatically assigned in GitHub to PR creator.

@CaptainAni187
CaptainAni187 marked this pull request as ready for review October 6, 2026 01:14
@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown

⚠️ GitHub issue #51780 has been automatically assigned in GitHub to PR creator.

@pitrou pitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is ok, let's see whether CI is green.

@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 7, 2026
@pitrou
pitrou merged commit 6c82e24 into apache:main Oct 7, 2026
63 of 64 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants