You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "JerAguilon (via GitHub)" <gi...@apache.org> on 2024/01/25 20:59:15 UTC
[PR] GH-39803: [C++][Acero] Fix key hashers for tables with differently ordered schemas than the output [arrow]
JerAguilon opened a new pull request, #39804:
URL: https://github.com/apache/arrow/pull/39804
### Rationale for this change
Issue is described visually in https://github.com/apache/arrow/issues/39803.
The key hasher works by inspecting the [column metadata](https://github.com/apache/arrow/blob/main/cpp/src/arrow/acero/asof_join_node.cc#L412) for the asof-join key fields. This returns whether columns are fixed width, among other things.
The issue is we are passing the `output_schema`, rather than the input's schema.
If an input looks like
```
key_string_type,ts_int32_type,val
```
But our expected output schema looks like:
```
ts_int32,key_string_type,...
```
Then the hasher will think that the `key_string_type`'s type is an int32. This completely throws of hashes. Tests currently get away with it since we just use ints across the board.
### What changes are included in this PR?
One line fix and test with string types.
### Are these changes tested?
Yes. Can see the test run before and after changes here: https://gist.github.com/JerAguilon/953d82ed288d58f9ce24d1a925def2cc
Before the change, notice that inputs 0 and 1 have mismatched hashes:
```
AsofjoinNode(0x16cf9e2d8): key hasher 1 got hashes [0, 9784892099856512926, 1050982531982388796, 10763536662319179482, 2029627098739957112, 11814237723602982167, 3080328155728858293, 12792882290360550483, 4058972722486426609, 13771526852823217039]
...
AsofjoinNode(0x16cf9dd18): key hasher 0 got hashes [17528465654998409509, 12047706865972860560, 18017664240540048750, 12358837084497432044, 8151160321586084686, 8691136767698756332, 15973065724125580046, 9654919479117127288, 618127929167745505, 3403805303373270709]
```
And after, they do match:
```
AsofjoinNode(0x16f2ea2d8): key hasher 1 got hashes [17528465654998409509, 12047706865972860560, 18017664240540048750, 12358837084497432044, 8151160321586084686, 8691136767698756332, 15973065724125580046, 9654919479117127288, 618127929167745505, 3403805303373270709]
...
AsofjoinNode(0x16f2e9d18): key hasher 0 got hashes [17528465654998409509, 12047706865972860560, 18017664240540048750, 12358837084497432044, 8151160321586084686, 8691136767698756332, 15973065724125580046, 9654919479117127288, 618127929167745505, 3403805303373270709]
```
...which is exactly what you want, since the `key` column for both tables looks like `["0", "1", ..."9"]`
### Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking.
-->
<!-- **This PR includes breaking changes to public APIs.** -->
<!--
Please uncomment the line below (and provide explanation) 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). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
-->
<!-- **This PR contains a "Critical Fix".** -->
--
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
Re: [PR] GH-39803: [C++][Acero] Fix AsOfJoin with differently ordered schemas than the output [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #39804:
URL: https://github.com/apache/arrow/pull/39804#discussion_r1486469974
##########
cpp/src/arrow/acero/asof_join_node_test.cc:
##########
@@ -16,6 +16,7 @@
// under the License.
#include <gmock/gmock-matchers.h>
+#include <iostream> // nocommit
Review Comment:
Is this required? Also, what is "nocommit" for?
##########
cpp/src/arrow/acero/asof_join_node_test.cc:
##########
@@ -1582,6 +1583,66 @@ TEST(AsofJoinTest, BatchSequencing) {
return TestSequencing(MakeIntegerBatches, /*num_batches=*/32, /*batch_size=*/1);
}
+template <typename BatchesMaker>
+void TestSchemaResolution(BatchesMaker maker, int num_batches, int batch_size) {
+ auto l_schema =
Review Comment:
Can you add a comment explaining this test, together with a reference to the GH issue?
--
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
Re: [PR] GH-39803: [C++][Acero] Fix AsOfJoin with differently ordered schemas than the output [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #39804:
URL: https://github.com/apache/arrow/pull/39804#issuecomment-1952444872
@github-actions crossbow submit -g cpp
--
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
Re: [PR] GH-39803: [C++][Acero] Fix AsOfJoin with differently ordered schemas than the output [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #39804:
URL: https://github.com/apache/arrow/pull/39804#issuecomment-1952620288
Thanks a lot for this @JerAguilon !
--
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
Re: [PR] GH-39803: [C++][Acero] Fix AsOfJoin with differently ordered schemas than the output [arrow]
Posted by "JerAguilon (via GitHub)" <gi...@apache.org>.
JerAguilon commented on code in PR #39804:
URL: https://github.com/apache/arrow/pull/39804#discussion_r1492725349
##########
cpp/src/arrow/acero/asof_join_node_test.cc:
##########
@@ -16,6 +16,7 @@
// under the License.
#include <gmock/gmock-matchers.h>
+#include <iostream> // nocommit
Review Comment:
whoops - this was for some cout debugging, will remove
--
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
Re: [PR] GH-39803: [C++][Acero] Fix AsOfJoin with differently ordered schemas than the output [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #39804:
URL: https://github.com/apache/arrow/pull/39804#issuecomment-1939118948
Also, can you please rebase from git main?
--
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
Re: [PR] GH-39803: [C++][Acero] Fix AsOfJoin with differently ordered schemas than the output [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39804:
URL: https://github.com/apache/arrow/pull/39804#issuecomment-1952449177
Revision: 11436fd2d6576fae9986be22593e68f28f5210ad
Submitted crossbow builds: [ursacomputing/crossbow @ actions-4147b43cbc](https://github.com/ursacomputing/crossbow/branches/all?query=actions-4147b43cbc)
|Task|Status|
|----|------|
|test-alpine-linux-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/7960261191/job/21728859062)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/7960261187/job/21728858909)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/7960260712/job/21728857675)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/runs/21728860449)|
|test-cuda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/7960260892/job/21728858038)|
|test-debian-11-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/7960261014/job/21728858654)|
|test-debian-11-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/7960261203/job/21728859101)|
|test-fedora-39-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/7960261389/job/21728859569)|
|test-ubuntu-20.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/7960260626/job/21728857382)|
|test-ubuntu-20.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/7960260987/job/21728858325)|
|test-ubuntu-20.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/7960260893/job/21728858289)|
|test-ubuntu-20.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/7960261064/job/21728858796)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/7960260628/job/21728857445)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/7960260985/job/21728858380)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/7960261245/job/21728859175)|
--
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
Re: [PR] GH-39803: [C++][Acero] Fix key hashers for tables with differently ordered schemas than the output [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39804:
URL: https://github.com/apache/arrow/pull/39804#issuecomment-1910986784
:warning: GitHub issue #39803 **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
Re: [PR] GH-39803: [C++][Acero] Fix AsOfJoin with differently ordered schemas than the output [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #39804:
URL: https://github.com/apache/arrow/pull/39804
--
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
Re: [PR] GH-39803: [C++][Acero] Fix AsOfJoin with differently ordered schemas than the output [arrow]
Posted by "JerAguilon (via GitHub)" <gi...@apache.org>.
JerAguilon commented on PR #39804:
URL: https://github.com/apache/arrow/pull/39804#issuecomment-1948941878
> Also, can you please rebase from git main?
Done and addressed comments. Thanks for the review
--
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
Re: [PR] GH-39803: [C++][Acero] Fix AsOfJoin with differently ordered schemas than the output [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #39804:
URL: https://github.com/apache/arrow/pull/39804#discussion_r1494538027
##########
cpp/src/arrow/acero/asof_join_node_test.cc:
##########
@@ -1582,6 +1582,70 @@ TEST(AsofJoinTest, BatchSequencing) {
return TestSequencing(MakeIntegerBatches, /*num_batches=*/32, /*batch_size=*/1);
}
+template <typename BatchesMaker>
+void TestSchemaResolution(BatchesMaker maker, int num_batches, int batch_size) {
+ // GH-39803: The key hasher needs to resolve the types of key columns. All other
+ // tests use int32 for all columns, but this test converts the key columns to
+ // strngs via a projection node to test that the column is correctly resolved
+ // to string.
Review Comment:
```suggestion
// GH-39803: The key hasher needs to resolve the types of key columns. All other
// tests use int32 for all columns, but this test converts the key columns to
// strings via a projection node to test that the column is correctly resolved
// to string.
```
--
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
Re: [PR] GH-39803: [C++][Acero] Fix AsOfJoin with differently ordered schemas than the output [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #39804:
URL: https://github.com/apache/arrow/pull/39804#issuecomment-1953129625
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 4b74b451d581e557765994a68ad87c575b4139b8.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/21743442797) has more details. It also includes information about 9 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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