[C++] Internal header file included from public one breaks build of external projects

[rtpsw]

Describe the bug, including details regarding any error messages, version, and platform.

There are a number of public header files that include internal header files:

$ find cpp/src/arrow/ -name "*.h" | xargs grep -l '_internal.h' | grep -v '_internal.h$'
cpp/src/arrow/compute/exec/swiss_join.h
cpp/src/arrow/compute/exec/util.h
cpp/src/arrow/compute/kernels/common.h
cpp/src/arrow/compute/kernels/row_encoder.h
cpp/src/arrow/dataset/test_util.h

This causes the build of external projects to break when including the public header because the internal one cannot be found since it is not part of the install.

Besides fixing the above public headers, it would be useful to add a (push-time?) check that ensures the above list is empty.

Component(s)

C++

[rtpsw]

take

[rtpsw]

@westonpace, I'm interested in very quickly resolving the bug at least for cpp/src/arrow/compute/exec/util.h, which is code you own. Remaining fixes will be left for later.

[westonpace]

I think the problem is actually that all the headers you listed should be marked internal.

[westonpace]

I can make a pass at this. A large portion of the headers in the compute folder are improperly labeled external.

[westonpace]

@rtpsw Are you trying to import cpp/src/arrow/compute/exec/util.h externally? If so, we might need to split the file. Which utilities are you after?

[rtpsw]

What about arrow/compute/exec/map_node.h? it includes arrow/compute/exec/util.h, so if the latter is made internal it would require the former to also be, but we need it to be public.

[rtpsw]

Are you trying to import cpp/src/arrow/compute/exec/util.h externally?

I am actually importing arrow/compute/exec/map_node.h externally. This is how I discovered this issue.

[rtpsw]

My plan for a fix is to refactor TracedNode to be a non-template class. @westonpace, WDYT?

[westonpace]

My plan for a fix is to refactor TracedNode to be a non-template class. @westonpace, WDYT?

Sounds good. I think I did it so I could access kind_name() but, in retrospect, you can probably just do:

class TracedNode {

  TracedNode(std::string_view kind_name);

};

[rtpsw]

Also accessing label(), which can be dynamically set, so I'd pass the ExecNode* instead.

[westonpace]

label() is a bit weirder because I don't think we actually assign labels until after construction. For now, just drop label from the tracing and we will fix it later (there is a larger fix needed for tracing to be useful I think).

[rtpsw]

See edit of previous comment.

[westonpace]

Passing ExecNode* sounds good to me.

[rtpsw]

I fixed the issues around cpp/src/arrow/compute/exec/util.h in the recent commit.

Looking at cpp/src/arrow/compute/exec/swiss_join.h, I see cpp/src/arrow/compute/exec/swiss_join.cc includes it at the top. IIUC, changing the include to cpp/src/arrow/compute/exec/swiss_join_internal.h would require it be moved to its sorted order in the includes?

[westonpace]

Looking at cpp/src/arrow/compute/exec/swiss_join.h, I see cpp/src/arrow/compute/exec/swiss_join.cc includes it at the top. IIUC, changing the include to cpp/src/arrow/compute/exec/swiss_join_internal.h would require it be moved to its sorted order in the includes?

Yes. Basically everything that is not in arrow/compute/api.h should be marked internal. So there are quite a few headers that will need to change.

[westonpace]

Also, you can add map_node.h to arrow/compute/api.h. I agree it is a good one to make public.

[rtpsw]

Basically everything that is not in arrow/compute/api.h should be marked internal. So there are quite a few headers that will need to change.

In this issue, I'd just like to avoid a breaking build of an external project due to inclusion of an installed header file, whether it is included in a api*.h or not. I'd defer on the systematic marking to a separate issue.