You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "liujiacheng777 (via GitHub)" <gi...@apache.org> on 2023/04/06 01:53:57 UTC
[GitHub] [arrow] liujiacheng777 opened a new pull request, #34919: ARROW-34918: [C++] Update vendored double-conversion
liujiacheng777 opened a new pull request, #34919:
URL: https://github.com/apache/arrow/pull/34919
Modifications based on doubleConversion version 3.2.1
--
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] liujiacheng777 commented on pull request #34919: ARROW-34918: [C++] Update vendored double-conversion
Posted by "liujiacheng777 (via GitHub)" <gi...@apache.org>.
liujiacheng777 commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1498390760
https://github.com/apache/arrow/issues/34918#issue-1656542730
--
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] liujiacheng777 closed pull request #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "liujiacheng777 (via GitHub)" <gi...@apache.org>.
liujiacheng777 closed pull request #34919: GH-34918: [C++] Update vendored double-conversion
URL: https://github.com/apache/arrow/pull/34919
--
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 #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1553673194
@ursabot please benchmark
--
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 #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1501420706
@anthonylouisbsb @projjal @praveenbingo You changed our vendored double-conversion in #9816. Is the change still needed?
Some Gandiva tests failed by upgrading vendored double-conversion (the change is revered):
https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/46724699#L2520
```text
[ RUN ] TestGdvFnStubs.TestCastVARCHARFromFloat
C:/projects/arrow/cpp/src/gandiva/gdv_function_stubs_test.cc(400): error: Expected equality of these values:
std::string(out_str, out_len)
Which is: "1E-5"
"1.0E-5"
[ FAILED ] TestGdvFnStubs.TestCastVARCHARFromFloat (0 ms)
[ RUN ] TestGdvFnStubs.TestCastVARCHARFromDouble
C:/projects/arrow/cpp/src/gandiva/gdv_function_stubs_test.cc(435): error: Expected equality of these values:
std::string(out_str, out_len)
Which is: "1E-5"
"1.0E-5"
[ FAILED ] TestGdvFnStubs.TestCastVARCHARFromDouble (0 ms)
```
If you really need the change, could you improve double-conversion in upstream instead of changing vendored double-conversion.
--
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] liujiacheng777 commented on pull request #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "liujiacheng777 (via GitHub)" <gi...@apache.org>.
liujiacheng777 commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1541166358
`66% tests passed, 15 tests failed out of 44
Label Time Summary:
arrow-benchmarks = 2663.27 sec*proc (44 tests)
benchmark = 2663.27 sec*proc (44 tests)
Total Test time (real) = 980.08 sec
The following tests FAILED:
6 - arrow-compute-function-benchmark (Failed)
7 - arrow-compute-scalar-arithmetic-benchmark (Failed)
8 - arrow-compute-scalar-boolean-benchmark (Failed)
10 - arrow-compute-scalar-compare-benchmark (Failed)
11 - arrow-compute-scalar-if-else-benchmark (Failed)
12 - arrow-compute-scalar-random-benchmark (Failed)
13 - arrow-compute-scalar-round-benchmark (Failed)
14 - arrow-compute-scalar-set-lookup-benchmark (Failed)
15 - arrow-compute-scalar-string-benchmark (Failed)
16 - arrow-compute-scalar-temporal-benchmark (Failed)
18 - arrow-compute-vector-sort-benchmark (Failed)
19 - arrow-compute-vector-partition-benchmark (Failed)
20 - arrow-compute-vector-topk-benchmark (Failed)
21 - arrow-compute-vector-replace-benchmark (Failed)
22 - arrow-compute-vector-selection-benchmark (Failed)
Errors while running CTest`
@kou ，I tested using “cmake .. -DARROW_BUILD_BENCHMARKS=ON && ctest -j8 --output-on-failure” according to the official manual, and there were 15 errors in the test results. The reason for the error is similar to the function not being registered（Key error: No function registered with name: greater）. Do I need to turn on some test switches
--
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 #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1553855664
It seems that we can't run our benchmarks multiple times for one commit.
@liujiacheng777 Could you rebase on main to re-run our benchmarks? (Rebasing changes a commit ID.)
--
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 #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1553741620
Commit ed9907e9400363592b7c23a345d2170ffb69197e already has scheduled benchmark runs.
--
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 #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1541210531
You may need to add `-DARROW_COMPUTE=ON`.
--
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 #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1550532083
@ursabot please benchmark
--
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] ianmcook commented on pull request #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1553747046
@ursabot please benchmark lang=R
--
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 #34919: GH-34918: [C++] Update vendored double-conversion 3.2.1
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1556465218
Benchmark runs are scheduled for baseline = a4bcee3e389f73669c64637c93cd691ee26b1cd2 and contender = 00e6996d9e32379ff9286fb1da25034ed073f8ba. 00e6996d9e32379ff9286fb1da25034ed073f8ba 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/8b1984e05a954da2ac29d971422bc2c1...ab9aeede76eb41ebbde2240933be0a4f/)
[Finished :arrow_down:0.48% :arrow_up:0.09%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/8fe557596e0e4c8b9068a61dbba4963b...e0ce56953c4442a68d512255916afe80/)
[Finished :arrow_down:0.33% :arrow_up:1.63%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/2a01fa8531ae4c70ba78070cb2111ed7...fe615a67b7f44d45acbbdf147c6da829/)
[Finished :arrow_down:0.21% :arrow_up:0.15%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/2412e3b45cab49d085fa74a5bbb38715...7706bf170a2441bb8a62fdc8732e9e40/)
Buildkite builds:
[Finished] [`00e6996d` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2905)
[Finished] [`00e6996d` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2941)
[Finished] [`00e6996d` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2906)
[Finished] [`00e6996d` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2931)
[Finished] [`a4bcee3e` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2904)
[Finished] [`a4bcee3e` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2940)
[Finished] [`a4bcee3e` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2905)
[Finished] [`a4bcee3e` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2930)
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] liujiacheng777 commented on pull request #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "liujiacheng777 (via GitHub)" <gi...@apache.org>.
liujiacheng777 commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1543949980
@kou thankds，I turned on the DARROW_COMPUT and passed all the tests
`100% tests passed, 0 tests failed out of 44
Label Time Summary:
arrow-benchmarks = 4639.70 sec*proc (44 tests)
benchmark = 4639.70 sec*proc (44 tests)
Total Test time (real) = 691.67 sec`
besides，I integrated the first two commits：https://github.com/apache/arrow/pull/9816 && https://github.com/apache/arrow/pull/35135，upgrade double-conversion version from v3.1.5 to v3.2.1
--
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 #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1553747064
Commit ed9907e9400363592b7c23a345d2170ffb69197e already has scheduled benchmark runs.
--
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] ianmcook commented on pull request #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1544823150
I opened a PR to upstream the double-conversion change at https://github.com/google/double-conversion/pull/195
--
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 #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1553456664
['Python', 'R'] benchmarks have high level of regressions.
[ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/32b4b885197b47e6be7afde1ef4eae14...ad7f8a6d1056462fa4f87b3a91e6f72b/)
--
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 #34919: GH-34918: [C++] Update vendored double-conversion 3.2.1
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #34919:
URL: https://github.com/apache/arrow/pull/34919
--
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 #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1530595609
FYI: #35135 added an update script. We need to update the patches under https://github.com/apache/arrow/tree/main/cpp/src/arrow/vendored/double-conversion/patches .
FYI: https://lists.apache.org/thread/mzb2qj4vkwhtbc64d1yfloc4s3r2po7j is the mailing list discussion about upstreaming.
--
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] liujiacheng777 commented on a diff in pull request #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "liujiacheng777 (via GitHub)" <gi...@apache.org>.
liujiacheng777 commented on code in PR #34919:
URL: https://github.com/apache/arrow/pull/34919#discussion_r1161390866
##########
cpp/src/arrow/CMakeLists.txt:
##########
@@ -237,12 +237,12 @@ set(ARROW_SRCS
vendored/base64.cpp
vendored/datetime/tz.cpp
vendored/double-conversion/bignum.cc
- vendored/double-conversion/double-conversion.cc
+ vendored/double-conversion/string-to-double.cc
+ vendored/double-conversion/double-to-string.cc
vendored/double-conversion/bignum-dtoa.cc
vendored/double-conversion/fast-dtoa.cc
vendored/double-conversion/cached-powers.cc
vendored/double-conversion/fixed-dtoa.cc
- vendored/double-conversion/diy-fp.cc
vendored/double-conversion/strtod.cc)
Review Comment:
Hello, I have sorted alphabetically in the new commit
--
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] ianmcook commented on pull request #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1530497802
@liujiacheng777 As Kou says above, there were a few small changes made to the vendored double-conversion in #9816. It https://github.com/apache/arrow/pull/9816/files#diff-d1cc5b70a5e980626bb70ae604a050d3393ac25a717a5a4c8dc40e8b5caf4b05
It looks like they were very small changes: less than 10 lines of code added, plus some comments. Could you please push a commit to this PR that ports these changes to the vendored double-conversion 3.2.1?
As Kou says above, we would like to upstream these changes, but porting the changes here will help, and I think it will fix the failing CI.
--
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 #34919: ARROW-34918: [C++] Update vendored double-conversion
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1498384973
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/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}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)
--
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 #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1553673240
Commit ed9907e9400363592b7c23a345d2170ffb69197e already has scheduled benchmark runs.
--
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 #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1544699003
Thanks.
I started CI jobs.
If nobody reports our patches to upstream in this month, let's remove our patches and merge this.
--
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] ianmcook commented on pull request #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1553042189
@kou do you want to merge this and then we can do https://github.com/apache/arrow/issues/35669 after?
--
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 #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1498531589
Could you use pull request template content next time?
--
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 a diff in pull request #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #34919:
URL: https://github.com/apache/arrow/pull/34919#discussion_r1159317042
##########
cpp/src/arrow/CMakeLists.txt:
##########
@@ -237,12 +237,12 @@ set(ARROW_SRCS
vendored/base64.cpp
vendored/datetime/tz.cpp
vendored/double-conversion/bignum.cc
- vendored/double-conversion/double-conversion.cc
+ vendored/double-conversion/string-to-double.cc
+ vendored/double-conversion/double-to-string.cc
vendored/double-conversion/bignum-dtoa.cc
vendored/double-conversion/fast-dtoa.cc
vendored/double-conversion/cached-powers.cc
vendored/double-conversion/fixed-dtoa.cc
- vendored/double-conversion/diy-fp.cc
vendored/double-conversion/strtod.cc)
Review Comment:
Could you sort this list in alphabetical order?
--
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 #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1553672915
Yes.
But could you check the benchmark result before we merge this?
> ['Python', 'R'] benchmarks have high level of regressions. [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/32b4b885197b47e6be7afde1ef4eae14...ad7f8a6d1056462fa4f87b3a91e6f72b/)
It says that the tpch benchmarks were slow with this change.
(Should we re-run benchmark?)
--
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] ianmcook commented on pull request #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1553741609
@ursabot please benchmark
--
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] ianmcook commented on pull request #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1553748588
It looks like we will have to wait for a while to re-run the benchmarks. It seems hard to tell whether the performance regressions are just noise.
--
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 #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1550532240
Benchmark runs are scheduled for baseline = 9ef2f655c615974a2d76bbe8fe40563d60a01929 and contender = ed9907e9400363592b7c23a345d2170ffb69197e. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/7237d9800db9499faca244a065bcc1c1...c20c9444b4314fa79e139c4d0a47e62e/)
[Scheduled] [test-mac-arm](https://conbench.ursa.dev/compare/runs/7653c2b8167f47b4821d7b0b5085b7e9...736e986be5da450dadb176182c7ca9a0/)
[Scheduled :warning: ursa-i9-9960x is offline.] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/32b4b885197b47e6be7afde1ef4eae14...ad7f8a6d1056462fa4f87b3a91e6f72b/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/be7488ae8c664ecbb514764b71969040...8f7c877a5f7841c89ca13afe704f48c9/)
Buildkite builds:
[Scheduled] [`ed9907e9` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2880)
[Scheduled] [`ed9907e9` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2915)
[Scheduled] [`ed9907e9` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2880)
[Scheduled] [`ed9907e9` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2905)
[Finished] [`9ef2f655` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2854)
[Finished] [`9ef2f655` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2893)
[Finished] [`9ef2f655` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2858)
[Finished] [`9ef2f655` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2881)
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] ianmcook commented on pull request #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1554768264
@kou I am more confident now that these performance regressions are just noise. The biggest regressions are happening in 0.1 and 0.01 scale factor TPC-H benchmarks (which are [known to be very noisy](https://github.com/voltrondata-labs/benchmarks/pull/133)) and the other scale factors of the same benchmarks are not showing regressions.
--
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 #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1498530737
:warning: GitHub issue #34918 **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] github-actions[bot] commented on pull request #34919: GH-34918: [C++] Update vendored double-conversion
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34919:
URL: https://github.com/apache/arrow/pull/34919#issuecomment-1498530704
* Closes: #34918
--
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