GH-36433: [C++] Update fast_float version to 3.10.1 by xxlaykxx · Pull Request #36434 · apache/arrow

xxlaykxx · 2023-07-02T13:11:20Z

Rationale for this change

Need this for parsing Infinity values with + sign.

What changes are included in this PR?

updated version of fast_float to version 3.10.1 (used this version because in higher versions c++ 20 started using that cause a lot of build errors)

Are these changes tested?

in scope of fast_float.

Are there any user-facing changes?

no

Closes: [C++] Update fast_float version for parsing Infinity values with + sign #36433

github-actions · 2023-07-02T13:11:46Z

⚠️ GitHub issue #36433 has been automatically assigned in GitHub to PR creator.

kou

Could you also update version information in cpp/src/arrow/vendored/fast_float/README.md?
(It's better that you update cpp/src/arrow/vendored/fast_float/update.sh to update README.md automatically.)

Could you fix build error?

https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/47446241#L1415

FAILED: src/arrow/CMakeFiles/arrow_shared.dir/Unity/unity_12_cxx.cxx.obj 
C:\Miniconda38-x64\envs\arrow\Library\bin\ccache.exe C:\PROGRA~2\MICROS~1\2019\COMMUN~1\VC\Tools\MSVC\1429~1.301\bin\Hostx64\x64\cl.exe  /nologo /TP -DABSL_CONSUME_DLL -DARROW_EXPORTING -DARROW_HAVE_RUNTIME_AVX2 -DARROW_HAVE_RUNTIME_AVX512 -DARROW_HAVE_RUNTIME_BMI2 -DARROW_HAVE_RUNTIME_SSE4_2 -DARROW_HAVE_SSE4_2 -DARROW_HDFS -DARROW_MIMALLOC -DARROW_S3_HAS_CRT -DARROW_WITH_BROTLI -DARROW_WITH_BZ2 -DARROW_WITH_LZ4 -DARROW_WITH_RE2 -DARROW_WITH_SNAPPY -DARROW_WITH_UTF8PROC -DARROW_WITH_ZLIB -DARROW_WITH_ZSTD -DAWS_AUTH_USE_IMPORT_EXPORT -DAWS_CAL_USE_IMPORT_EXPORT -DAWS_CHECKSUMS_USE_IMPORT_EXPORT -DAWS_COMMON_USE_IMPORT_EXPORT -DAWS_COMPRESSION_USE_IMPORT_EXPORT -DAWS_CRT_CPP_USE_IMPORT_EXPORT -DAWS_EVENT_STREAM_USE_IMPORT_EXPORT -DAWS_HTTP_USE_IMPORT_EXPORT -DAWS_IO_USE_IMPORT_EXPORT -DAWS_MQTT_USE_IMPORT_EXPORT -DAWS_MQTT_WITH_WEBSOCKETS -DAWS_S3_USE_IMPORT_EXPORT -DAWS_SDKUTILS_USE_IMPORT_EXPORT -DAWS_SDK_VERSION_MAJOR=1 -DAWS_SDK_VERSION_MINOR=10 -DAWS_SDK_VERSION_PATCH=13 -DAWS_USE_IO_COMPLETION_PORTS -DBOOST_ALL_DYN_LINK -DBOOST_ALL_NO_LIB -DPROTOBUF_USE_DLLS -DURI_STATIC_BUILD -DUSE_IMPORT_EXPORT -DUSE_IMPORT_EXPORT=1 -DUSE_WINDOWS_DLL_SEMANTICS -D_CRT_SECURE_NO_WARNINGS -D_ENABLE_EXTENDED_ALIGNED_STORAGE -Darrow_shared_EXPORTS -IC:\projects\arrow\cpp\build\src -IC:\projects\arrow\cpp\src -IC:\projects\arrow\cpp\src\generated -external:IC:\projects\arrow\cpp\thirdparty\flatbuffers\include -external:IC:\projects\arrow\cpp\thirdparty\hadoop\include -external:IC:\Miniconda38-x64\envs\arrow\Library\include -external:IC:\projects\arrow\cpp\build\mimalloc_ep\src\mimalloc_ep\include\mimalloc-2.0 -external:W0 /DWIN32 /D_WINDOWS /GR /EHsc /D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING  /EHsc /wd5105 /bigobj /utf-8 /W3 /wd4800 /wd4996 /wd4065  /WX /MP /MD /Od /UNDEBUG -std:c++17 -MD /showIncludes /Fosrc\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_12_cxx.cxx.obj /Fdsrc\arrow\CMakeFiles\arrow_shared.dir\ /FS -c C:\projects\arrow\cpp\build\src\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_12_cxx.cxx
C:\projects\arrow\cpp\src\arrow/vendored/fast_float/fast_float.h(30): error C2988: unrecognizable template declaration/definition
C:\projects\arrow\cpp\src\arrow/vendored/fast_float/fast_float.h(30): error C2143: syntax error: missing ';' before '<'
C:\projects\arrow\cpp\src\arrow/vendored/fast_float/fast_float.h(30): error C2059: syntax error: '<'
C:\projects\arrow\cpp\src\arrow/vendored/fast_float/fast_float.h(31): error C2653: 'chars_format': is not a class or namespace name
C:\projects\arrow\cpp\src\arrow/vendored/fast_float/fast_float.h(38): error C2988: unrecognizable template declaration/definition
C:\projects\arrow\cpp\src\arrow/vendored/fast_float/fast_float.h(38): error C2143: syntax error: missing ';' before '<'
C:\projects\arrow\cpp\src\arrow/vendored/fast_float/fast_float.h(38): error C2059: syntax error: '<'
C:\projects\arrow\cpp\src\arrow\vendored\fast_float\ascii_number.h(266): error C2061: syntax error: identifier 'parse_options_t'
...

kou · 2023-07-02T21:15:01Z

cpp/src/arrow/vendored/fast_float/update.sh

-sed -i.bak -E -e "s/v[0-9.]+/v${version}/g" *.h
-sed -i.bak -E \
+gsed -i.bak -E -e "s/v[0-9.]+/v${version}/g" *.h
+gsed -i.bak -E \


Why do we need this?
If we use gsed, it works only on macOS (and *BSD).

kou · 2023-07-05T02:13:16Z

cpp/src/arrow/vendored/fast_float/README.md


 The files in this directory are vendored from fast_float
-git tag `v3.8.1`.
+git tag `v3.10.1`.


Do you really want to use 3.10.1?
It seems that the latest version is 5.2.0: https://github.com/fastfloat/fast_float/releases

after 3.10.1 they started using c++ 20 and it cause a lot of build errors. In reality - we just need in parse_number.h

#ifdef FASTFLOAT_ALLOWS_LEADING_PLUS // disabled by default if (*first == UC('+')) { ++first; }

OK. Could you describe it in descriptions of the PR and the associated issue?

kou · 2023-07-06T02:25:20Z

It seems that we need to define FASTFLOAT_ALLOWS_LEADING_PLUS=1 explicitly to support +Infinity.
Do you want to do it in this pull request or in a follow-up pull request?

xxlaykxx · 2023-07-06T08:21:20Z

@kou i'm not sure that everyone need this - for my side, we can just build it with -DFASTFLOAT_ALLOWS_LEADING_PLUS option for enabling this functionality.

kou · 2023-07-06T21:22:40Z

I'm not familiar with +Infinity needs but it seems that +Infinity is useful for all cases. So I think that it's better that we always set FASTFLOAT_ALLOWS_LEADING_PLUS=1.

Could you share your use-case?
Could you also share downsides when we always set FASTFLOAT_ALLOWS_LEADING_PLUS=1?

xxlaykxx · 2023-07-07T10:06:26Z

not sure that downsides exist with using FASTFLOAT_ALLOWS_LEADING_PLUS 1.

My user case - in dremio some customer want to parse infinity value with + sign when they use gandiva impl of cast varchar to double/float function(same logic exist in Java implementation for cast function). So we want to add this to arrow side too

kou

OK.
Let's enable +Infinity unconditionally.

kou · 2023-07-09T07:34:21Z

cpp/src/arrow/vendored/fast_float/parse_number.h

@@ -1,5 +1,6 @@
 #ifndef FASTFLOAT_PARSE_NUMBER_H
 #define FASTFLOAT_PARSE_NUMBER_H
+#define FASTFLOAT_ALLOWS_LEADING_PLUS 1


Could you define this in cpp/src/arrow/util/value_parsing.cc instead of here?
If we define this here, this change will be discarded when we update fast_float next time.
And could you also add a Gandiva test to check +Infinity is accepted or not?

kou · 2023-07-16T08:39:58Z

cpp/src/arrow/util/value_parsing.cc

 // KIND, either express or implied.  See the License for the
 // specific language governing permissions and limitations
 // under the License.
+#ifndef FASTFLOAT_ALLOWS_LEADING_PLUS


Why do we need this ifndef? What situation are you thinking? I think that this is needless...

cpp/src/arrow/util/value_parsing.cc

kou

+1

I'll merge this after the tiny style improvement is applied.

Co-authored-by: Sutou Kouhei <[email protected]>

xxlaykxx · 2023-07-19T08:11:37Z

@kou style improvement was merged. Thanks

kou · 2023-07-19T20:41:04Z

Thanks.
I've merged this.

…) (#34) ### Rationale for this change Need this for parsing Infinity values with + sign. ### What changes are included in this PR? updated version of fast_float to version 3.10.1 (used this version because in higher versions c++ 20 started using that cause a lot of build errors) ### Are these changes tested? in scope of fast_float. ### Are there any user-facing changes? no * Closes: apache#36433 Lead-authored-by: Ivan Chesnov <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]>

…) (#35) ### Rationale for this change Need this for parsing Infinity values with + sign. ### What changes are included in this PR? updated version of fast_float to version 3.10.1 (used this version because in higher versions c++ 20 started using that cause a lot of build errors) ### Are these changes tested? in scope of fast_float. ### Are there any user-facing changes? no * Closes: apache#36433 Lead-authored-by: Ivan Chesnov <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]>

conbench-apache-arrow · 2023-07-28T12:06:30Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 3f0b620.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.

…) (#35) ### Rationale for this change Need this for parsing Infinity values with + sign. ### What changes are included in this PR? updated version of fast_float to version 3.10.1 (used this version because in higher versions c++ 20 started using that cause a lot of build errors) ### Are these changes tested? in scope of fast_float. ### Are there any user-facing changes? no * Closes: apache#36433 Lead-authored-by: Ivan Chesnov <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]>

### Rationale for this change Need this for parsing Infinity values with + sign. ### What changes are included in this PR? updated version of fast_float to version 3.10.1 (used this version because in higher versions c++ 20 started using that cause a lot of build errors) ### Are these changes tested? in scope of fast_float. ### Are there any user-facing changes? no * Closes: apache#36433 Lead-authored-by: Ivan Chesnov <[email protected]> Co-authored-by: Ivan Chesnov <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

apacheGH-36433 updated fast_float version to 5.2.0

68c7eec

github-actions bot added Component: C++ awaiting review Awaiting review labels Jul 2, 2023

kou reviewed Jul 2, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Jul 2, 2023

pitrou changed the title ~~GH-36433: [C++] updated fast_float version to 5.2.0~~ GH-36433: [C++] Update fast_float version to 5.2.0 Jul 4, 2023

xxlaykxx changed the title ~~GH-36433: [C++] Update fast_float version to 5.2.0~~ GH-36433: [C++] Update fast_float version Jul 4, 2023

Update fast_float version

92b5adf

github-actions bot added Component: Documentation awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 4, 2023

xxlaykxx requested a review from kou July 4, 2023 15:41

kou reviewed Jul 5, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jul 5, 2023

xxlaykxx requested a review from kou July 5, 2023 11:28

kou changed the title ~~GH-36433: [C++] Update fast_float version~~ GH-36433: [C++] Update fast_float version to 3.10.1 Jul 6, 2023

Define FASTFLOAT_ALLOWS_LEADING_PLUS

81abdaa

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 7, 2023

define FASTFLOAT_ALLOWS_LEADING_PLUS

99f4b61

kou reviewed Jul 9, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jul 9, 2023

fixed formating

b0ddd25

xxlaykxx requested a review from kou July 14, 2023 13:23

kou reviewed Jul 16, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jul 16, 2023

fixed comment

5045b35

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 17, 2023

xxlaykxx requested a review from kou July 17, 2023 11:09

kou reviewed Jul 18, 2023

View reviewed changes

cpp/src/arrow/util/value_parsing.cc Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jul 18, 2023

kou approved these changes Jul 18, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Jul 18, 2023

xxlaykxx and others added 3 commits July 18, 2023 10:16

Update cpp/src/arrow/util/value_parsing.cc

dbdaea0

Co-authored-by: Sutou Kouhei <[email protected]>

Merge branch 'apache:main' into apacheGH-36433

2a5eef6

Merge branch 'apache:main' into apacheGH-36433

89e16f0

kou merged commit 3f0b620 into apache:main Jul 19, 2023

kou removed the awaiting merge Awaiting merge label Jul 19, 2023

xxlaykxx mentioned this pull request Jul 21, 2023

DX-61115: [C++] Update fast_float version to 3.10.1 (#36434) dremio/arrow#34

Merged

Conversation

xxlaykxx commented Jul 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Jul 2, 2023

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

kou Jul 2, 2023

Choose a reason for hiding this comment

Uh oh!

kou Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

xxlaykxx Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

kou Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

kou commented Jul 6, 2023

Uh oh!

xxlaykxx commented Jul 6, 2023

Uh oh!

kou commented Jul 6, 2023

Uh oh!

xxlaykxx commented Jul 7, 2023

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

kou Jul 9, 2023

Choose a reason for hiding this comment

Uh oh!

kou Jul 16, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

xxlaykxx commented Jul 19, 2023

Uh oh!

kou commented Jul 19, 2023

Uh oh!

conbench-apache-arrow bot commented Jul 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

xxlaykxx commented Jul 2, 2023 •

edited

Loading