You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "rtpsw (via GitHub)" <gi...@apache.org> on 2023/05/16 07:48:23 UTC
[GitHub] [arrow] rtpsw opened a new pull request, #35608: GH-35607: [C++] Support simple Substrait aggregate extensions
rtpsw opened a new pull request, #35608:
URL: https://github.com/apache/arrow/pull/35608
### Rationale for this change
See #35607.
### What changes are included in this PR?
A simple `SubstraitAggregateToArrow` converter for `urn:arrow:substrait_simple_extension_function` is added.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
No.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #35608: GH-35607: [C++] Support simple Substrait aggregate extensions
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #35608:
URL: https://github.com/apache/arrow/pull/35608#issuecomment-1555368831
Benchmark runs are scheduled for baseline = 99ef3aada1d1589604da8905f3c9100ff1c997e9 and contender = 6f0b34897b8645643493ce8907e4e441b274be3e. 6f0b34897b8645643493ce8907e4e441b274be3e is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/3335dcebf6874b0f89710de236573bcb...7595592c5c244e2a8efcdd043365b0c7/)
[Finished :arrow_down:0.12% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/91812a7e21f74a21b82574f71575f646...28424ac135ad4b62bdc402c322cd1baa/)
[Finished :arrow_down:0.33% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/185e87dc29354fd58c7e7002e49c0784...f0e0fdb854c247c3a557439fad57744e/)
[Finished :arrow_down:0.45% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/4f0b7cfe6e5447ddb084dbadc9a72e36...23cc5ca9c22942a9adf87b98e886003a/)
Buildkite builds:
[Finished] [`6f0b3489` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2888)
[Finished] [`6f0b3489` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2924)
[Finished] [`6f0b3489` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2889)
[Finished] [`6f0b3489` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2914)
[Finished] [`99ef3aad` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2887)
[Finished] [`99ef3aad` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2923)
[Finished] [`99ef3aad` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2888)
[Finished] [`99ef3aad` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2913)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtpsw commented on a diff in pull request #35608: GH-35607: [C++] Support simple Substrait aggregate extensions
Posted by "rtpsw (via GitHub)" <gi...@apache.org>.
rtpsw commented on code in PR #35608:
URL: https://github.com/apache/arrow/pull/35608#discussion_r1195396061
##########
cpp/src/arrow/engine/substrait/ext_test.cc:
##########
@@ -285,5 +340,55 @@ TEST(ExtensionIdRegistryTest, RegisterNestedFunctions) {
}
}
+TEST(ExtensionIdRegistryTest, RegisterNestedAggregates) {
Review Comment:
All but the last test I added (this one included) are for aggregate registration that were just missing. They are not specifically related to the main-code change here, but IMHO they are needed for basic coverage. They follow the existing tests for function registration.
The last test I added is the one that specifically covers the main-code change here.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #35608: GH-35607: [C++] Support simple Substrait aggregate extensions
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35608:
URL: https://github.com/apache/arrow/pull/35608#issuecomment-1549168838
* Closes: #35607
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] icexelloss commented on pull request #35608: GH-35607: [C++] Support simple Substrait aggregate extensions
Posted by "icexelloss (via GitHub)" <gi...@apache.org>.
icexelloss commented on PR #35608:
URL: https://github.com/apache/arrow/pull/35608#issuecomment-1549823915
@rtpsw Looks good to me. Left a question.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] icexelloss commented on a diff in pull request #35608: GH-35607: [C++] Support simple Substrait aggregate extensions
Posted by "icexelloss (via GitHub)" <gi...@apache.org>.
icexelloss commented on code in PR #35608:
URL: https://github.com/apache/arrow/pull/35608#discussion_r1195403770
##########
cpp/src/arrow/engine/substrait/ext_test.cc:
##########
@@ -285,5 +340,55 @@ TEST(ExtensionIdRegistryTest, RegisterNestedFunctions) {
}
}
+TEST(ExtensionIdRegistryTest, RegisterNestedAggregates) {
Review Comment:
Right. But I wonder what specially are you trying to test in this one? It is not clear what "NestedAggregates" means here
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] icexelloss commented on pull request #35608: GH-35607: [C++] Support simple Substrait aggregate extensions
Posted by "icexelloss (via GitHub)" <gi...@apache.org>.
icexelloss commented on PR #35608:
URL: https://github.com/apache/arrow/pull/35608#issuecomment-1549826385
@westonpace Can you please take a look also.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #35608: GH-35607: [C++] Support simple Substrait aggregate extensions
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #35608:
URL: https://github.com/apache/arrow/pull/35608#issuecomment-1555370058
['Python', 'R'] benchmarks have high level of regressions.
[ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/185e87dc29354fd58c7e7002e49c0784...f0e0fdb854c247c3a557439fad57744e/)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] icexelloss commented on pull request #35608: GH-35607: [C++] Support simple Substrait aggregate extensions
Posted by "icexelloss (via GitHub)" <gi...@apache.org>.
icexelloss commented on PR #35608:
URL: https://github.com/apache/arrow/pull/35608#issuecomment-1551419199
The CI failures seem unrelated since I saw the same error in other PR.
Based on the discussion with @westonpace on https://github.com/apache/arrow/pull/35513#issuecomment-1544485845 I am going to go head and merge this one.
@westonpace Let me know if there is anything we want to change here to do as follow up.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] icexelloss commented on a diff in pull request #35608: GH-35607: [C++] Support simple Substrait aggregate extensions
Posted by "icexelloss (via GitHub)" <gi...@apache.org>.
icexelloss commented on code in PR #35608:
URL: https://github.com/apache/arrow/pull/35608#discussion_r1196532629
##########
cpp/src/arrow/engine/substrait/ext_test.cc:
##########
@@ -285,5 +340,55 @@ TEST(ExtensionIdRegistryTest, RegisterNestedFunctions) {
}
}
+TEST(ExtensionIdRegistryTest, RegisterNestedAggregates) {
Review Comment:
I see.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] icexelloss commented on a diff in pull request #35608: GH-35607: [C++] Support simple Substrait aggregate extensions
Posted by "icexelloss (via GitHub)" <gi...@apache.org>.
icexelloss commented on code in PR #35608:
URL: https://github.com/apache/arrow/pull/35608#discussion_r1195274619
##########
cpp/src/arrow/engine/substrait/ext_test.cc:
##########
@@ -285,5 +340,55 @@ TEST(ExtensionIdRegistryTest, RegisterNestedFunctions) {
}
}
+TEST(ExtensionIdRegistryTest, RegisterNestedAggregates) {
Review Comment:
What is this testing?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] icexelloss commented on a diff in pull request #35608: GH-35607: [C++] Support simple Substrait aggregate extensions
Posted by "icexelloss (via GitHub)" <gi...@apache.org>.
icexelloss commented on code in PR #35608:
URL: https://github.com/apache/arrow/pull/35608#discussion_r1196485899
##########
cpp/src/arrow/engine/substrait/ext_test.cc:
##########
@@ -285,5 +340,55 @@ TEST(ExtensionIdRegistryTest, RegisterNestedFunctions) {
}
}
+TEST(ExtensionIdRegistryTest, RegisterNestedAggregates) {
Review Comment:
What do you mean "nested registry"? looks like registry1 and registry2 are orthgonal (we don't see them being nested together maybe I missed sth)?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] icexelloss merged pull request #35608: GH-35607: [C++] Support simple Substrait aggregate extensions
Posted by "icexelloss (via GitHub)" <gi...@apache.org>.
icexelloss merged PR #35608:
URL: https://github.com/apache/arrow/pull/35608
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtpsw commented on a diff in pull request #35608: GH-35607: [C++] Support simple Substrait aggregate extensions
Posted by "rtpsw (via GitHub)" <gi...@apache.org>.
rtpsw commented on code in PR #35608:
URL: https://github.com/apache/arrow/pull/35608#discussion_r1196519969
##########
cpp/src/arrow/engine/substrait/ext_test.cc:
##########
@@ -285,5 +340,55 @@ TEST(ExtensionIdRegistryTest, RegisterNestedFunctions) {
}
}
+TEST(ExtensionIdRegistryTest, RegisterNestedAggregates) {
Review Comment:
The invocation `MakeExtensionIdRegistry()`, which is used in `Nested` test-cases, [makes a nested registry on top of the default one](https://github.com/apache/arrow/blob/99ef3aada1d1589604da8905f3c9100ff1c997e9/cpp/src/arrow/engine/substrait/util.cc#L64).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #35608: GH-35607: [C++] Support simple Substrait aggregate extensions
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35608:
URL: https://github.com/apache/arrow/pull/35608#issuecomment-1549168886
:warning: GitHub issue #35607 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtpsw commented on pull request #35608: GH-35607: [C++] Support simple Substrait aggregate extensions
Posted by "rtpsw (via GitHub)" <gi...@apache.org>.
rtpsw commented on PR #35608:
URL: https://github.com/apache/arrow/pull/35608#issuecomment-1549171403
cc @icexelloss
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtpsw commented on a diff in pull request #35608: GH-35607: [C++] Support simple Substrait aggregate extensions
Posted by "rtpsw (via GitHub)" <gi...@apache.org>.
rtpsw commented on code in PR #35608:
URL: https://github.com/apache/arrow/pull/35608#discussion_r1195978244
##########
cpp/src/arrow/engine/substrait/ext_test.cc:
##########
@@ -285,5 +340,55 @@ TEST(ExtensionIdRegistryTest, RegisterNestedFunctions) {
}
}
+TEST(ExtensionIdRegistryTest, RegisterNestedAggregates) {
Review Comment:
"Nested" refers to a nested registry, as opposed to the default one, in which the items are registered. This is also the case in the existing "NestedFunctions" and "NestedTypes" tests.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org