-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-35134: [C++] Add arrow_vendored
namespace around double-conversion library
#35135
Conversation
|
f7fb098
to
b708420
Compare
b708420
to
ddbcd59
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Could you rebase on main for #35145?
I want us to upstream our changes instead of having our patches for easy to maintain.
Could you wait for a response from Gandiva developers?
#34919 (comment)
ddbcd59
to
3b2677f
Compare
This PR doesn't change the current status-quo of the patched changes, and instead just makes it explicit. Would it make sense to merge this now, and allow for the upstream changes for the version update? The patch in the script would actually fail since those files don't exist in the newest version of double-conversion (the version being updated in that PR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Would it make sense to merge this now, and allow for the upstream changes for the version update?
OK. I see.
Benchmark runs are scheduled for baseline = 6fe4813 and contender = 74c9a78. 74c9a78 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Could you do "add arrow_vendored namespace" by the script added by this pull request? |
…nversion library (apache#35135) ### Rationale for this change See apache#35134 This change avoids potential symbol collisions when linking Arrow within other libraries. This actually seems like the original intention (a private namespace) per the following merge: 767c953 The message in that merge is confusing, because it implies that the merge made the library private, when it actually did the opposite (specifically in 3b89b19). A quote is below: > Also, make its use private, because of Windows DLL exports. ### What changes are included in this PR? * an `update.sh` script to * automatically pull a new version, * apply the namespace change, * apply a custom patch for seemingly Gandiva (15137e2) * this patch will break in future versions of double-conversion, but it is at least documented in the update script * the update script is run for the current version The script is based on: https://github.com/apache/arrow/blob/main/cpp/src/arrow/vendored/fast_float/update.sh ### Are these changes tested? Yes. Built and tests run ### Are there any user-facing changes? Only if users used to depend on using the internal vendored double-conversion libraries for their own code. In such a case, when they upgrade, they would have to specify the `arrow_vendored` namespace. * Closes: apache#35134 Authored-by: Mike Lui <mikelui@meta.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
…nversion library (apache#35135) ### Rationale for this change See apache#35134 This change avoids potential symbol collisions when linking Arrow within other libraries. This actually seems like the original intention (a private namespace) per the following merge: 767c953 The message in that merge is confusing, because it implies that the merge made the library private, when it actually did the opposite (specifically in 3b89b19). A quote is below: > Also, make its use private, because of Windows DLL exports. ### What changes are included in this PR? * an `update.sh` script to * automatically pull a new version, * apply the namespace change, * apply a custom patch for seemingly Gandiva (15137e2) * this patch will break in future versions of double-conversion, but it is at least documented in the update script * the update script is run for the current version The script is based on: https://github.com/apache/arrow/blob/main/cpp/src/arrow/vendored/fast_float/update.sh ### Are these changes tested? Yes. Built and tests run ### Are there any user-facing changes? Only if users used to depend on using the internal vendored double-conversion libraries for their own code. In such a case, when they upgrade, they would have to specify the `arrow_vendored` namespace. * Closes: apache#35134 Authored-by: Mike Lui <mikelui@meta.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
…nversion library (apache#35135) ### Rationale for this change See apache#35134 This change avoids potential symbol collisions when linking Arrow within other libraries. This actually seems like the original intention (a private namespace) per the following merge: 767c953 The message in that merge is confusing, because it implies that the merge made the library private, when it actually did the opposite (specifically in 3b89b19). A quote is below: > Also, make its use private, because of Windows DLL exports. ### What changes are included in this PR? * an `update.sh` script to * automatically pull a new version, * apply the namespace change, * apply a custom patch for seemingly Gandiva (15137e2) * this patch will break in future versions of double-conversion, but it is at least documented in the update script * the update script is run for the current version The script is based on: https://github.com/apache/arrow/blob/main/cpp/src/arrow/vendored/fast_float/update.sh ### Are these changes tested? Yes. Built and tests run ### Are there any user-facing changes? Only if users used to depend on using the internal vendored double-conversion libraries for their own code. In such a case, when they upgrade, they would have to specify the `arrow_vendored` namespace. * Closes: apache#35134 Authored-by: Mike Lui <mikelui@meta.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Rationale for this change
See #35134
This change avoids potential symbol collisions when linking Arrow within other libraries.
This actually seems like the original intention (a private namespace) per the following merge: 767c953
The message in that merge is confusing, because it implies that the merge made the library private, when it actually did the opposite (specifically in 3b89b19). A quote is below:
What changes are included in this PR?
update.sh
script toThe script is based on: https://github.com/apache/arrow/blob/main/cpp/src/arrow/vendored/fast_float/update.sh
Are these changes tested?
Yes. Built and tests run
Are there any user-facing changes?
Only if users used to depend on using the internal vendored double-conversion libraries for their own code. In such a case, when they upgrade, they would have to specify the
arrow_vendored
namespace.