You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "rtadepalli (via GitHub)" <gi...@apache.org> on 2023/01/21 20:52:20 UTC
[GitHub] [arrow] rtadepalli opened a new pull request, #33822: ARROW-17339: [C++] Simplify IPC writer for dense unions
rtadepalli opened a new pull request, #33822:
URL: https://github.com/apache/arrow/pull/33822
Closes: https://github.com/apache/arrow/issues/32613
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/master/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
-->
### Rationale for this change
Dense union offsets are always non-strictly monotonic for any given child as mandated by the spec, The C++ implementation still assumes that the offsets may be in any order. This can be improved.
### What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
Just a change to eliminate looping over the size of a `DenseUnionArray` twice.
### Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
I am not functionally changing anything. All changes respect the spec, and behavior should be 1:1 with the existing implementation. I believe existing tests should suffice.
### Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
There are no user facing changes for this.
<!--
If there are any breaking changes to public APIs, please add the `breaking-change` label.
-->
--
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] kou commented on pull request #33822: GH-32613: [C++] Simplify IPC writer for dense unions
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #33822:
URL: https://github.com/apache/arrow/pull/33822#issuecomment-1401484089
@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
[GitHub] [arrow] kou commented on pull request #33822: GH-32613: [C++] Simplify IPC writer for dense unions
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #33822:
URL: https://github.com/apache/arrow/pull/33822#issuecomment-1401488121
Do existing tests cover this case?
--
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] rtadepalli commented on pull request #33822: GH-32613: [C++] Simplify IPC writer for dense unions
Posted by "rtadepalli (via GitHub)" <gi...@apache.org>.
rtadepalli commented on PR #33822:
URL: https://github.com/apache/arrow/pull/33822#issuecomment-1410876592
Looks like `arrow-ipc-read-write-test` is failing with my bug 👍
I will revert the bug if the test results seem satisfactory to you.
--
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] rtadepalli commented on pull request #33822: GH-32613: [C++] Simplify IPC writer for dense unions
Posted by "rtadepalli (via GitHub)" <gi...@apache.org>.
rtadepalli commented on PR #33822:
URL: https://github.com/apache/arrow/pull/33822#issuecomment-1408630249
But won't those tests run as part of the CI pipeline?
I can definitely try to run them, but would need some help 😅. Would running all tests in the ipc/ package be sufficient?
--
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 #33822: GH-32613: [C++] Simplify IPC writer for dense unions
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #33822:
URL: https://github.com/apache/arrow/pull/33822#issuecomment-1413866079
Benchmark runs are scheduled for baseline = 3ab246f374c17a216d86edcfff7ff416b3cff803 and contender = b413ac4f2b6911af5e8241803277caccc43aa3c4. b413ac4f2b6911af5e8241803277caccc43aa3c4 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/31a2f5184dd641759798c80dafbd0ebe...ccd2665f7a224aa78f8e545a30f01399/)
[Failed :arrow_down:0.55% :arrow_up:0.03%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/6492b67439424da1aaae23972fe07d4b...386b1a6062104a93a58ac539859e8362/)
[Finished :arrow_down:2.04% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/29fca76b5af5453ead383e01a26878b0...f49ffe82fec441239851e4e20cc0b580/)
[Finished :arrow_down:0.03% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/a9a213d9fc284549894989108ac8ea84...8f88bbf4942b41c9b9e53395b8da3f2f/)
Buildkite builds:
[Finished] [`b413ac4f` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2308)
[Failed] [`b413ac4f` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2334)
[Finished] [`b413ac4f` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2305)
[Finished] [`b413ac4f` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2326)
[Finished] [`3ab246f3` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2307)
[Finished] [`3ab246f3` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2333)
[Finished] [`3ab246f3` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2304)
[Finished] [`3ab246f3` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2325)
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] rtadepalli commented on pull request #33822: GH-32613: [C++] Simplify IPC writer for dense unions
Posted by "rtadepalli (via GitHub)" <gi...@apache.org>.
rtadepalli commented on PR #33822:
URL: https://github.com/apache/arrow/pull/33822#issuecomment-1410404832
Hello @kou , I have introduced a bug in the code. I removed the +1 while calculating the child offset.
--
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] rtadepalli commented on pull request #33822: GH-32613: [C++] Simplify IPC writer for dense unions
Posted by "rtadepalli (via GitHub)" <gi...@apache.org>.
rtadepalli commented on PR #33822:
URL: https://github.com/apache/arrow/pull/33822#issuecomment-1407414127
I think they should. I was looking at the implementation in Go as a reference, and it doesn't seem like the PR that introduced the change (https://github.com/apache/arrow/pull/13806) added any additional tests for finding the offset. I am of the idea right now that existing tests should break if this change is wrong. Please let me know if this is not true, and a unit test is warranted.
--
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] kou commented on pull request #33822: GH-32613: [C++] Simplify IPC writer for dense unions
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #33822:
URL: https://github.com/apache/arrow/pull/33822#issuecomment-1409870777
You can just push a commit that includes a bug to this branch.
Then CI runs all existing tests with the buggy code. If there are tests that cover this case, they will be failed.
--
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] rtadepalli commented on pull request #33822: GH-32613: [C++] Simplify IPC writer for dense unions
Posted by "rtadepalli (via GitHub)" <gi...@apache.org>.
rtadepalli commented on PR #33822:
URL: https://github.com/apache/arrow/pull/33822#issuecomment-1399510407
Hello @kou, I have fixed the broken clang-format test. Thanks.
--
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] kou commented on pull request #33822: GH-32613: [C++] Simplify IPC writer for dense unions
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #33822:
URL: https://github.com/apache/arrow/pull/33822#issuecomment-1411290277
Thanks.
It seems that we have a test for this case.
Could you revert the bug?
--
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] kou merged pull request #33822: GH-32613: [C++] Simplify IPC writer for dense unions
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #33822:
URL: https://github.com/apache/arrow/pull/33822
--
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] ramasai1 commented on pull request #33822: GH-32613: [C++] Simplify IPC writer for dense unions
Posted by "ramasai1 (via GitHub)" <gi...@apache.org>.
ramasai1 commented on PR #33822:
URL: https://github.com/apache/arrow/pull/33822#issuecomment-1400412571
Hello, can `TestS3FS.OpenOutputStreamBackgroundWrites` be restarted? I am not sure if it is related to my changes. Thanks.
--
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] rtadepalli commented on pull request #33822: GH-32613: [C++] Simplify IPC writer for dense unions
Posted by "rtadepalli (via GitHub)" <gi...@apache.org>.
rtadepalli commented on PR #33822:
URL: https://github.com/apache/arrow/pull/33822#issuecomment-1412153262
I have revert the three commits in which I pushed bugs.
--
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 #33822: GH-32613: [C++] Simplify IPC writer for dense unions
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #33822:
URL: https://github.com/apache/arrow/pull/33822#issuecomment-1413866682
['Python', 'R'] benchmarks have high level of regressions.
[ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/29fca76b5af5453ead383e01a26878b0...f49ffe82fec441239851e4e20cc0b580/)
--
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] kou commented on pull request #33822: GH-32613: [C++] Simplify IPC writer for dense unions
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #33822:
URL: https://github.com/apache/arrow/pull/33822#issuecomment-1407916880
If you add a bug in the target logic and there are tests for this case, these existing tests will be failed.
Could you try it? Or do you want me to do it?
--
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