Skip to content

Commit

Permalink
feat: support new aggregate logic introduced in Substrait 0.9.0 (#39)
Browse files Browse the repository at this point in the history
BREAKING CHANGE: the grouping set index column for aggregates is now
no longer generated if there are less than two grouping sets, as per
the changes made in Substrait 0.9.0
  • Loading branch information
jvanstraten authored Aug 16, 2022
1 parent 5ae3f8b commit ddc1fd9
Show file tree
Hide file tree
Showing 11 changed files with 36 additions and 47 deletions.
13 changes: 7 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,13 @@ Substrait versions. Whenever Substrait makes a breaking change that affects
validation, the validator will be updated accordingly and drop support for the
older version. Refer to the table below for the version compatibility matrix.

| Substrait... | ... is supported by validator ... |
| ------------- | ------------------------------------ |
| 0.7.x - 0.8.x | current version |
| 0.5.x - 0.6.x | 0.0.6 |
| 0.3.x - 0.4.x | 0.0.4 - 0.0.5 |
| older | try 0.0.1, but your mileage may vary |
| Substrait... | ... is supported by validator ... |
| -------------- | ------------------------------------ |
| 0.9.x - 0.10.x | current version |
| 0.7.x - 0.8.x | 0.0.7 |
| 0.5.x - 0.6.x | 0.0.6 |
| 0.3.x - 0.4.x | 0.0.4 - 0.0.5 |
| older | try 0.0.1, but your mileage may vary |

As Substrait and the validator stabilize and breaking changes become less
frequent, the intention is to support more versions within a single validator
Expand Down
40 changes: 14 additions & 26 deletions rs/src/parse/relations/aggregate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,15 +181,15 @@ pub fn parse_aggregate_rel(
);
}

// Add the column for the grouping set index.
// FIXME: this field makes no sense for aggregate relations that only have
// measures. It's also disputable whether it should exist when there is
// only one grouping set.
fields.push(Field {
expression: expressions::Expression::Function(String::from("group_index"), vec![]),
data_type: data_type::DataType::new_integer(false),
field_type: FieldType::GroupingSetIndex,
});
// Add the column for the grouping set index if there is more than one
// grouping set.
if sets.len() > 1 {
fields.push(Field {
expression: expressions::Expression::Function(String::from("group_index"), vec![]),
data_type: data_type::DataType::new_integer(false),
field_type: FieldType::GroupingSetIndex,
});
}
let fields = fields;

// Derive schema.
Expand Down Expand Up @@ -260,23 +260,11 @@ pub fn parse_aggregate_rel(
}
}
FieldType::GroupingSetIndex => {
if x.groupings.is_empty() {
format!(
"Field {index}: undefined value, reserved for grouping \
set index."
)
} else if x.groupings.len() == 1 {
format!(
"Field {index}: always zero, representing the index of the \
matched grouping set (of which there is only one here)."
)
} else {
format!(
"Field {index}: integer between 0 and {} inclusive, \
representing the index of the matched grouping set.",
x.groupings.len() - 1
)
}
format!(
"Field {index}: integer between 0 and {} inclusive, \
representing the index of the matched grouping set.",
x.groupings.len() - 1
)
}
});
}
Expand Down
2 changes: 1 addition & 1 deletion rs/src/resources/substrait-version
Original file line number Diff line number Diff line change
@@ -1 +1 @@
0.8.0
0.10.0
2 changes: 1 addition & 1 deletion tests/tests/relations/aggregate/measure-and-group.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,4 @@ plan:
- measure:
functionReference: 1
output_type: { i64: { nullability: NULLABILITY_REQUIRED } }
__test: [ type: "STRUCT<string, i64, i32>" ]
__test: [ type: "STRUCT<string, i64>" ]
2 changes: 1 addition & 1 deletion tests/tests/relations/aggregate/measure.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -37,4 +37,4 @@ plan:
- measure:
functionReference: 1
output_type: { i64: { nullability: NULLABILITY_REQUIRED } }
__test: [ type: "STRUCT<i64, i32>" ]
__test: [ type: "STRUCT<i64>" ]
2 changes: 1 addition & 1 deletion tests/tests/relations/aggregate/single-set-one-expr.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,4 @@ plan:
- selection:
rootReference: {}
directReference: { structField: { field: 0 } }
__test: [ type: "STRUCT<string, i32>" ]
__test: [ type: "STRUCT<string>" ]
2 changes: 1 addition & 1 deletion tests/tests/relations/aggregate/single-set-two-expr.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,4 @@ plan:
- selection:
rootReference: {}
directReference: { structField: { field: 0 } }
__test: [ type: "STRUCT<fp32, string, i32>" ]
__test: [ type: "STRUCT<fp32, string>" ]
2 changes: 1 addition & 1 deletion tests/tests/tpc-h/tpc-h03.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ plan:
relations:
- root:
__test:
- type: "NSTRUCT<L_ORDERKEY: i32, REVENUE: i64, O_ORDERDATE: DECIMAL?<19, 0>, O_SHIPPRIORITY: date?>"
- type: "NSTRUCT<L_ORDERKEY: i64, REVENUE: DECIMAL?<19, 0>, O_ORDERDATE: date?, O_SHIPPRIORITY: i32?>"
input:
fetch:
common:
Expand Down
14 changes: 7 additions & 7 deletions tests/tests/tpc-h/tpc-h10.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,14 @@ plan:
__test:
- type: "\
NSTRUCT<\
C_CUSTKEY: i32, \
C_NAME: i64, \
REVENUE: VARCHAR?<25>, \
C_CUSTKEY: i64, \
C_NAME: VARCHAR?<25>, \
REVENUE: DECIMAL?<19, 0>, \
C_ACCTBAL: DECIMAL?<19, 0>, \
N_NAME: DECIMAL?<19, 0>, \
C_ADDRESS: FIXEDCHAR?<25>, \
C_PHONE: VARCHAR?<40>, \
C_COMMENT: FIXEDCHAR?<15>\
N_NAME: FIXEDCHAR?<25>, \
C_ADDRESS: VARCHAR?<40>, \
C_PHONE: FIXEDCHAR?<15>, \
C_COMMENT: VARCHAR?<117>\
>"
input:
fetch:
Expand Down
2 changes: 1 addition & 1 deletion tests/tests/tpc-h/tpc-h14.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ plan:
relations:
- root:
__test:
- type: "NSTRUCT<PROMO_REVENUE: i32>"
- type: "NSTRUCT<PROMO_REVENUE: DECIMAL?<19, 2>>"
input:
project:
common:
Expand Down

0 comments on commit ddc1fd9

Please sign in to comment.