fangchenli opened a new pull request, #50635:
URL: https://github.com/apache/arrow/pull/50635

   ### 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.
   
   ### 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.
   
   Closes #50634 
   
   
   **This PR contains a "Critical Fix".** (If the changes fix either (a) a 
security vulnerability, (b) a bug that caused incorrect or invalid data to be 
produced, or (c) a bug that causes a crash (even when the API contract is 
upheld), please provide explanation. If not, you can remove this.)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to