Add struct utility functions. #10776
Conversation
bdice
added
improvement
github-actions
bot
added
CMake
bdice
changed the title
|
I don't think you need to change the includes if the header file is in the same directory as the source. |
@rwlee Is there a reason this change was adopted in #9452? If we prefer to revert these, I can do that. I don't have a strong opinion. |
Codecov Report
@@ Coverage Diff @@
## branch-22.06 #10776 +/- ##
================================================
+ Coverage 86.40% 86.43% +0.03%
================================================
Files 143 143
Lines 22448 22448
================================================
+ Hits 19396 19403 +7
+ Misses 3052 3045 -7
Continue to review full report at Codecov.
|
bdice
changed the title
|
@davidwendt Good call. I discussed with @rwlee and reverted those changes. I'll also revert them on #9452. |
|
@bdice since the PR note will end up in the Git log, could you write a more descriptive message of the changes? It's much more helpful to see what this PR changes rather than knowing that it was pulled from another PR, especially since that PR will eventually be changed to remove these changes. |
| * and structs containing null values. Null structs add a column to the result of the flatten column | ||
| * utility and necessitates column_nullability::FORCE when flattening the column for comparison |
There was a problem hiding this comment.
fyi, given that this is for the legacy flattening, I suspect the need for this function will go away with using the new comparators. It's fine to merge for now, but just keep this in mind.
| bool contains_null_structs(column_view const& col) | ||
| { | ||
| return (is_struct(col) && col.has_nulls()) || | ||
| std::any_of(col.child_begin(), col.child_end(), contains_null_structs); | ||
| } | ||
|
|
There was a problem hiding this comment.
Given that this is very short, I wonder why don't we inline it in the header?
There was a problem hiding this comment.
Given that this is very short, I wonder why don't we inline it in the header?
I don't agree with this assessment for moving a function to a header. Moving it to a header may require additional includes in the header that are unnecessary to the caller. I think we should only inline host functions in headers when absolutely necessary. It can improve compile time and also perhaps encapsulation such that changing the implementation (as in this case) would not require recompiling all the source files that include the header.
|
@gpucibot merge |
This PR adds some struct utility functions. This change is needed for the eventual support of structs in binary operations. See also: PR #9452.