ARROW-17517: [C++] Remove internal headers from substrait API #14131
Conversation
|
I figured I should tackle this to brush up on the rules for includes myself to try and help keep things clean going forwards. While I was at it I also did a bit of general header cleanup. The only structural changes were moving MapNode into its own file (I think @save-buffer has a PR to remove this at some point anyways so it seemed easier to do this than try and hide it's impl in some way) and converting IdStorage to a virtual class to hide its implementation (depended on internal hashing stuff). Attaching a snippet showing the compile tree from compiling |
|
It seems that you refer wrong Jira issue. |
westonpace
changed the title
Oops, thanks :) |
|
|
|
I'm a little stumped by this Windows error. MapNode is an abstract base class. It is used by project_node.cc, filter_node.cc (both in arrow compute) and also by file_base.cc (in datasets). In order for it to be seen by datasets I have to mark it as ARROW_EXPORT. However, when I do so, I get duplicate symbols when linking arrow compute (presumably because project_node and filter_node each have implementations?) If I remove ARROW_EXPORT then datasets fails to link because it can't find the symbols. |
|
The weird thing is that it works fine on AppVeyor. Might be an issue with ccache... |
|
You probably need to rebase on master now and fix conflicts (switch to |
westonpace
force-pushed
the
bugfix/ARROW-17157--remove-internal-headers-from-substrait
branch
from
47a7cce to
55af4d7
Compare
Looks like the rebase magically solved the Windows problem so maybe it was ccache then. I think the remaining CI failures are unrelated. |
…s module for tee node so I think I'll have to figure out why it causes duplicate symbols
pitrou
force-pushed
the
bugfix/ARROW-17157--remove-internal-headers-from-substrait
branch
from
55af4d7 to
ac9de3c
Compare
|
Rebased and fixed a small error compiling the Flight examples. |
|
Hmm, the macOS build failures look unexpected but also unrelated to this. |
|
Thanks @westonpace ! I'm keeping the JIRA open for the small followup PR. |
|
Benchmark runs are scheduled for baseline = 081b70b and contender = 529f653. 529f653 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
['Python', 'R'] benchmarks have high level of regressions. |
…#14131) Lead-authored-by: Weston Pace <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>
…#14131) Lead-authored-by: Weston Pace <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>
No description provided.