Skip to content

GH-50634: [C++][Engine] Fix Substrait consumer silently dropping modern FetchRel/AggregateRel fields#50635

Open
fangchenli wants to merge 1 commit into
apache:mainfrom
fangchenli:gh-substrait-fetchrel-limit
Open

GH-50634: [C++][Engine] Fix Substrait consumer silently dropping modern FetchRel/AggregateRel fields#50635
fangchenli wants to merge 1 commit into
apache:mainfrom
fangchenli:gh-substrait-fetchrel-limit

Conversation

@fangchenli

@fangchenli fangchenli commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Rationale for this change

Arrow's Substrait consumer read several fields through deprecated accessors from a vendored substrait proto pinned to v0.44.0, so values sent by current producers (which use the newer forms introduced by ~v0.63.0) were silently dropped and defaulted, producing wrong results with no error.

FetchRel (LIMIT/OFFSET): count/offset became oneofs (count_mode/offset_mode), and current producers (e.g. DataFusion, substrait-rs 0.63.0) emit the count_expr/offset_expr expression arms, leaving the scalar fields unset. The consumer reads the default 0, turning every LIMIT n into LIMIT 0 (empty result) and dropping OFFSET (skipping no rows).

AggregateRel (GROUP BY): grouping expressions moved from the deprecated inline Grouping.grouping_expressions to Grouping.expression_references, indexing the AggregateRel-level grouping_expressions. Reading only the deprecated field dropped the group keys, collapsing every row into a single group.

I'm not sure about the backward compatibility policy for Substrait, so I kept the old path and only bumped the version to 0.63.0 to match DataFusion.

What changes are included in this PR?

  • Bump the vendored substrait proto from v0.44.0 to v0.63.0 (matching DataFusion's pinned substrait-rs), where these newer forms were introduced while the deprecated arms remain for backward compatibility.
  • Rewrite the FetchRel consumer to read the count_mode/offset_mode oneofs: evaluate count_expr/offset_expr to a constant int64 literal, fall back to the deprecated scalar arm, treat an unset/null count as ALL and an unset/null offset as 0. A no-op fetch (ALL, offset 0) is skipped so it does not impose the fetch node's input-ordering requirement on plans that never limit output.
  • Read AggregateRel grouping keys via expression_references into the AggregateRel-level grouping_expressions, falling back to the deprecated inline field for older producers.
  • Update the JoinRel consumer for the renamed JoinType enum values (JOIN_TYPE_SEMI/ANTI -> JOIN_TYPE_LEFT_SEMI/LEFT_ANTI) surfaced by the bump.
  • Add regression tests for the count_expr/offset_expr arms, unset-count = ALL, and grouping via expression_references.

Are these changes tested?

Yes, tests added.

Are there any user-facing changes?

Yes, bug fix.

@github-actions

Copy link
Copy Markdown

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

@fangchenli
fangchenli force-pushed the gh-substrait-fetchrel-limit branch 2 times, most recently from 7c14788 to 54ff52a Compare July 25, 2026 05:34
@fangchenli
fangchenli marked this pull request as ready for review July 25, 2026 06:14
@fangchenli
fangchenli requested a review from pitrou as a code owner July 25, 2026 06:14
Copilot AI review requested due to automatic review settings July 25, 2026 06:14

Copilot AI 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.

Pull request overview

Updates Arrow C++ Substrait support to correctly consume modern Substrait v0.63-style fields (notably FetchRel count/offset oneofs and AggregateRel grouping expression references) after bumping the vendored Substrait protos, preventing silently wrong LIMIT/OFFSET and GROUP BY behavior.

Changes:

  • Bump vendored Substrait proto version to v0.63.0 and adjust protobuf tooling requirements/flags accordingly.
  • Fix Substrait relation deserialization to correctly interpret FetchRel count_expr/offset_expr and AggregateRel grouping expression_references (with backward-compatible fallbacks).
  • Add regression tests covering modern FetchRel expression arms, unset-count semantics, and AggregateRel grouping references; update JoinRel enum handling for renamed values.

Reviewed changes

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

Show a summary per file
File Description
cpp/thirdparty/versions.txt Bumps the vendored Substrait tarball version/hash to v0.63.0.
cpp/src/arrow/engine/substrait/test_plan_builder.cc Suppresses deprecation warnings when intentionally emitting deprecated grouping fields in test helpers.
cpp/src/arrow/engine/substrait/serde_test.cc Adds regression tests for FetchRel expression arms, unset-count semantics, and AggregateRel grouping via expression references.
cpp/src/arrow/engine/substrait/relation_internal.cc Updates Substrait-to-Acero conversion for modern FetchRel/AggregateRel fields and JoinRel enum rename handling.
cpp/cmake_modules/ThirdpartyToolchain.cmake Raises required protobuf version for Substrait and adds protoc flags for proto3 optional support where needed.

Comment thread cpp/thirdparty/versions.txt
Comment thread cpp/src/arrow/engine/substrait/serde_test.cc Outdated
Copilot AI review requested due to automatic review settings July 25, 2026 06:22
@fangchenli
fangchenli force-pushed the gh-substrait-fetchrel-limit branch from 54ff52a to 6ce2c2e Compare July 25, 2026 06:22

Copilot AI 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.

Pull request overview

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

@kou kou 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.

Is this really a "Critical Fix"?

Could you read https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code ?

Comment thread cpp/cmake_modules/ThirdpartyToolchain.cmake Outdated
@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Jul 25, 2026
@fangchenli

Copy link
Copy Markdown
Contributor Author

Is this really a "Critical Fix"?

Could you read https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code ?

Removed. But I can't blame Claude for this. This bug did cause an incorrect result in a DataFusion round-trip test, so I left the "Critical Fix" note in the PR body. That said, it was a fairly unconventional use case and probably didn’t need to raise the alarm.

…g modern FetchRel/AggregateRel fields

Arrow's Substrait consumer read several fields through deprecated accessors
from a vendored substrait proto pinned to v0.44.0, so values sent by current
producers (which use the newer forms introduced by ~v0.63.0) were silently
dropped and defaulted, producing wrong results with no error.

FetchRel (LIMIT/OFFSET): count/offset became oneofs (count_mode/offset_mode)
and current producers (e.g. DataFusion, substrait-rs 0.63.0) emit the
count_expr/offset_expr expression arms, leaving the scalar fields unset. The
consumer read the default 0, turning every `LIMIT n` into `LIMIT 0` (empty
result) and dropping OFFSET (skipping no rows).

AggregateRel (GROUP BY): grouping expressions moved from the deprecated inline
`Grouping.grouping_expressions` to `Grouping.expression_references` indexing the
AggregateRel-level `grouping_expressions`. Reading only the deprecated field
dropped the group keys, collapsing every row into a single group.

- Bump the vendored substrait proto from v0.44.0 to v0.63.0 (matching
  DataFusion's pinned substrait-rs), where these newer forms were introduced
  while the deprecated arms remain for backward compatibility.
- Rewrite the FetchRel consumer to read the count_mode/offset_mode oneofs:
  evaluate count_expr/offset_expr to a constant int64 literal, fall back to the
  deprecated scalar arm, treat an unset/null count as ALL and an unset/null
  offset as 0. A no-op fetch (ALL, offset 0) is skipped so it does not impose
  the fetch node's input-ordering requirement on plans that never limit output.
- Read AggregateRel grouping keys via expression_references into the
  AggregateRel-level grouping_expressions, falling back to the deprecated
  inline field for older producers.
- Update the JoinRel consumer for the renamed JoinType enum values
  (JOIN_TYPE_SEMI/ANTI -> JOIN_TYPE_LEFT_SEMI/LEFT_ANTI) surfaced by the bump.
- Add regression tests for the count_expr/offset_expr arms, unset-count = ALL,
  and grouping via expression_references.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 25, 2026 20:40
@fangchenli
fangchenli force-pushed the gh-substrait-fetchrel-limit branch from 6ce2c2e to b73c899 Compare July 25, 2026 20:40
@github-actions github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 25, 2026

Copilot AI 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.

Pull request overview

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

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.

3 participants