Copilot commented on code in PR #50635:
URL: https://github.com/apache/arrow/pull/50635#discussion_r3649626855
##########
cpp/thirdparty/versions.txt:
##########
@@ -106,8 +106,8 @@ ARROW_SIMDJSON_BUILD_VERSION=v4.6.4
ARROW_SIMDJSON_BUILD_SHA256_CHECKSUM=b091107844fe928158c5c2265c20360fff312889ddf7ebc4528a0f0f8f2ff9cd
ARROW_SNAPPY_BUILD_VERSION=1.2.2
ARROW_SNAPPY_BUILD_SHA256_CHECKSUM=90f74bc1fbf78a6c56b3c4a082a05103b3a56bb17bca1a27e052ea11723292dc
-ARROW_SUBSTRAIT_BUILD_VERSION=v0.44.0
-ARROW_SUBSTRAIT_BUILD_SHA256_CHECKSUM=f989a862f694e7dbb695925ddb7c4ce06aa6c51aca945105c075139aed7e55a2
+ARROW_SUBSTRAIT_BUILD_VERSION=v0.63.0
+ARROW_SUBSTRAIT_BUILD_SHA256_CHECKSUM=14aabf828d54fbae38aa90f2c10006952c0040feff8f813cabad5bc13d6fbcfe
Review Comment:
Substrait version was bumped here, but the Meson wrap file still points at
v0.44.0 (and the old hash). This will leave Meson/subproject builds using a
different Substrait proto version than the CMake toolchain, which can cause
inconsistent behavior or build failures.
##########
cpp/src/arrow/engine/substrait/serde_test.cc:
##########
@@ -2085,6 +2085,83 @@ TEST(Substrait, AggregateBasic) {
EXPECT_EQ(agg_options.aggregates[0].function, "hash_sum");
}
+// Grouping keys referenced by index (expression_references into the
+// AggregateRel-level grouping_expressions) must be resolved; otherwise the
keys
+// are dropped and the aggregate degrades from a grouped hash_sum to a global
sum.
+TEST(Substrait, AggregateGroupingExpressionReferences) {
+ ASSERT_OK_AND_ASSIGN(auto buf,
+ internal::SubstraitFromJSON("Plan", R"({
+ "version": { "major_number": 9999, "minor_number": 9999, "patch_number":
9999 },
+ "relations": [{
+ "rel": {
+ "aggregate": {
+ "input": {
+ "read": {
+ "base_schema": {
+ "names": ["A", "B", "C"],
+ "struct": {
+ "types": [{ "i32": {} }, { "i32": {} }, { "i32": {} }]
+ }
+ },
+ "local_files": {
+ "items": [
+ { "uri_file": "file:///tmp/dat.parquet", "parquet": {} }
+ ]
+ }
+ }
+ },
+ "groupings": [{
+ "expressionReferences": [0]
+ }],
+ "groupingExpressions": [{
+ "selection": {
+ "directReference": { "structField": { "field": 0 } }
+ }
+ }],
+ "measures": [{
+ "measure": {
+ "functionReference": 0,
+ "arguments": [{
+ "value": {
+ "selection": {
+ "directReference": { "structField": { "field": 1 } }
+ }
+ }
+ }],
+ "sorts": [],
+ "phase": "AGGREGATION_PHASE_INITIAL_TO_RESULT",
+ "outputType": { "i64": {} }
+ }
+ }]
+ }
+ }
+ }],
+ "extensionUris": [{
+ "extension_uri_anchor": 0,
+ "uri":
"https://github.com/substrait-io/substrait/blob/main/extensions/functions_arithmetic.yaml";
+ }],
+ "extensions": [{
+ "extension_function": {
+ "extension_uri_reference": 0,
+ "function_anchor": 0,
+ "name": "sum"
+ }
+ }],
+ })",
+
/*ignore_unknown_fields=*/false));
+
+ auto sp_ext_id_reg = MakeExtensionIdRegistry();
+ ASSERT_OK_AND_ASSIGN(auto sink_decls,
+ DeserializePlans(*buf, [] { return kNullConsumer; }));
Review Comment:
The extension registry created here is unused; this can trigger
-Wunused-variable and is confusing since DeserializePlans has an overload that
accepts a registry. Either remove the registry or pass it into DeserializePlans.
--
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]
