Skip to content

Conversation

@emkornfield
Copy link
Contributor

@emkornfield emkornfield commented May 16, 2019

The best way I could think of accomplishing this was to introduce another top level namespace arrow_vendored, so "duration" does not conflict between the two. Other suggestions welcome.

@emkornfield emkornfield changed the title Arrow 5346 ARROW-5346: [C++] Revert changed to vendored datetime library May 16, 2019
@pravindra
Copy link
Contributor

The best way I could think of accomplishing this was to introduce another top level namespace arrow_vendored, so "duration" does not conflict between the two. Other suggestions welcome.

lgtm.

@emkornfield
Copy link
Contributor Author

Looks like I missed a gandiva file, will fix tomorrow.

@pitrou
Copy link
Member

pitrou commented May 16, 2019

Have you tried reporting the issue upstream?

@wesm
Copy link
Member

wesm commented May 16, 2019

I agree that the issue should get fixed upstream. If we are having this arrow_vendored namespace (which actually seems like a good idea to isolate the vendored implementations from namespace contamination) then the util:: is probably superfluous

@emkornfield
Copy link
Contributor Author

I can ask for a fix upstream, but it isn't small and it really is an edge case with how we wrapped the library into our own namespace. The only other way this occurs is if someone has "duration" included in a file for before "date.h" without any namespace.

Essentially the code boiled down to:

namespace b {
void print_me() {
        std::cout << "buz";
}
}

namespace f {
void print_me() {
        std::cout << "foo";
}
        void run() {
                using namespace b;
                print_me();
        }
}

So print_me was getting resolved to f::print_me (in our case arrow::duration) instead of b::print_me (std::chrono::duration).

@emkornfield
Copy link
Contributor Author

Opened an issue upstream. I think this is a worthwhile fix anyways (i've removed the util namespace).

@codecov-io
Copy link

Codecov Report

Merging #4324 into master will increase coverage by 0.97%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #4324      +/-   ##
=========================================
+ Coverage   88.22%   89.2%   +0.97%     
=========================================
  Files         777     632     -145     
  Lines       97881   86483   -11398     
  Branches     1251       0    -1251     
=========================================
- Hits        86360   77147    -9213     
+ Misses      11285    9336    -1949     
+ Partials      236       0     -236
Impacted Files Coverage Δ
cpp/src/gandiva/precompiled/time.cc 97.19% <ø> (ø) ⬆️
cpp/src/arrow/vendored/datetime/date.h 30.62% <ø> (ø) ⬆️
cpp/src/arrow/vendored/datetime/tz_private.h 33.33% <ø> (ø) ⬆️
cpp/src/arrow/vendored/datetime/tz.h 43.47% <ø> (ø) ⬆️
cpp/src/arrow/vendored/datetime/tz.cpp 65.45% <ø> (ø) ⬆️
cpp/src/gandiva/cast_time.cc 100% <ø> (ø) ⬆️
cpp/src/gandiva/date_utils.h 100% <100%> (ø) ⬆️
cpp/src/gandiva/precompiled/epoch_time_point.h 100% <100%> (ø) ⬆️
cpp/src/arrow/util/parsing.h 96.11% <100%> (ø) ⬆️
cpp/src/arrow/pretty_print.cc 83.48% <100%> (ø) ⬆️
... and 147 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c63c79...3d2581e. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants