dmitry-chirkov-dremio commented on code in PR #50635:
URL: https://github.com/apache/arrow/pull/50635#discussion_r4337613154
##########
cpp/src/arrow/engine/substrait/relation_internal.cc:
##########
@@ -855,12 +948,35 @@ Result<DeclarationInfo> FromProto(const substrait::Rel&
rel, const ExtensionSet&
std::vector<FieldRef> keys;
if (aggregate.groupings_size() > 0) {
const substrait::AggregateRel::Grouping& group =
aggregate.groupings(0);
- int grouping_expr_size = group.grouping_expressions_size();
- keys.reserve(grouping_expr_size);
- for (int exp_id = 0; exp_id < grouping_expr_size; exp_id++) {
- ARROW_ASSIGN_OR_RAISE(
- compute::Expression expr,
- FromProto(group.grouping_expressions(exp_id), ext_set,
conversion_options));
+ // Current producers reference grouping expressions by index into the
+ // AggregateRel-level `grouping_expressions`; older ones inline them
in the
+ // deprecated `Grouping.grouping_expressions`. Reading only the
deprecated
+ // field drops the group keys, collapsing every row into one group.
+ std::vector<const substrait::Expression*> grouping_exprs;
+ if (group.expression_references_size() > 0) {
+ grouping_exprs.reserve(group.expression_references_size());
+ for (uint32_t ref : group.expression_references()) {
+ if (ref >=
static_cast<uint32_t>(aggregate.grouping_expressions_size())) {
+ return Status::Invalid(
+ "substrait::AggregateRel grouping expression reference ",
ref,
+ " is out of bounds (", aggregate.grouping_expressions_size(),
+ " grouping expressions)");
+ }
+ grouping_exprs.push_back(&aggregate.grouping_expressions(ref));
Review Comment:
What if grouping_expressions is [A, B], but expression_references is [1, 0]?
I added and ran a unit test with input rows [(1, 10), (1, 10)]. It returns
columns [B, A] with values [(10, 1)], while Substrait requires declaration
order: [A, B], [(1, 10)]. The [0, 1] control passes.
Can we preserve declaration order and add this regression?
##########
cpp/src/arrow/engine/substrait/relation_internal.cc:
##########
@@ -779,8 +807,73 @@ Result<DeclarationInfo> FromProto(const substrait::Rel&
rel, const ExtensionSet&
ARROW_ASSIGN_OR_RAISE(auto input,
FromProto(fetch.input(), ext_set,
conversion_options));
- int64_t offset = fetch.offset();
- int64_t count = fetch.count();
+ // Unset offset is 0. The scalar `offset` is deprecated for `offset_expr`
+ // but still read for older producers.
+ int64_t offset = 0;
+ switch (fetch.offset_mode_case()) {
+ case substrait::FetchRel::kOffset:
+ // GH-44954: the scalar `offset` field is [[deprecated]] in the
proto.
+ ARROW_SUPPRESS_DEPRECATION_WARNING
+ offset = fetch.offset();
+ ARROW_UNSUPPRESS_DEPRECATION_WARNING
+ break;
+ case substrait::FetchRel::kOffsetExpr: {
+ ARROW_ASSIGN_OR_RAISE(auto value,
+ FetchArgFromExpr(fetch.offset_expr(),
"offset", ext_set,
+ conversion_options));
+ offset = value.value_or(0);
+ break;
+ }
+ case substrait::FetchRel::OFFSET_MODE_NOT_SET:
+ break;
+ }
+ if (offset < 0) {
+ return Status::Invalid("substrait::FetchRel offset must be
non-negative, got ",
+ offset);
+ }
+
+ // Unset count, a null count_expr, or the deprecated scalar -1 all mean
ALL.
+ // Acero has no unbounded fetch, so ALL uses int64 max (FetchCounter
tracks
+ // skipped and returned rows separately, so this cannot overflow).
+ constexpr int64_t kFetchAll = std::numeric_limits<int64_t>::max();
+ int64_t count = kFetchAll;
+ switch (fetch.count_mode_case()) {
+ case substrait::FetchRel::kCount: {
+ // The deprecated scalar field uses -1 to signal ALL.
+ ARROW_SUPPRESS_DEPRECATION_WARNING
+ int64_t scalar_count = fetch.count();
+ ARROW_UNSUPPRESS_DEPRECATION_WARNING
+ if (scalar_count >= 0) {
+ count = scalar_count;
+ } else if (scalar_count != -1) {
+ return Status::Invalid(
+ "substrait::FetchRel count must be non-negative or -1 (ALL),
got ",
+ scalar_count);
+ }
+ break;
+ }
+ case substrait::FetchRel::kCountExpr: {
+ ARROW_ASSIGN_OR_RAISE(
+ auto value,
+ FetchArgFromExpr(fetch.count_expr(), "count", ext_set,
conversion_options));
+ if (value.has_value()) {
+ if (*value < 0) {
+ return Status::Invalid(
+ "substrait::FetchRel count must be non-negative, got ",
*value);
+ }
+ count = *value;
+ }
+ break;
+ }
+ case substrait::FetchRel::COUNT_MODE_NOT_SET:
Review Comment:
Do we need to preserve LIMIT 0 for binary plans written with the older
schema?
With v0.44, protobuf omits count = 0. The new schema reads those bytes as an
unset count, which we now treat as ALL.
If older binary plans are supported, can we add a regression for this?
Otherwise, should we document the compatibility limit?
--
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]
