Skip to content
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

schema_tree_node needs a constructor. #11395

Open
hyperbolic2346 opened this issue Jul 28, 2022 · 2 comments
Open

schema_tree_node needs a constructor. #11395

hyperbolic2346 opened this issue Jul 28, 2022 · 2 comments
Labels
0 - Waiting on Author Waiting for author to respond to review cuIO cuIO issue libcudf Affects libcudf (C++/CUDA) code. proposal Change current process or code

Comments

@hyperbolic2346
Copy link
Contributor

Nitpick: Looks like schema_tree_node needs a constructor. :]

I would've recommended aggregate initialization, but I'm not sure how appropriate it would be, since it's initializing parent members:

        schema_tree_node col_schema {
          .type            = Type::BYTE_ARRAY,
          .converted_type  = ConvertedType::UNKNOWN,
          .stats_dtype     = statistics_dtype::dtype_byte_array,
          .repetition_type = col_nullable ? OPTIONAL : REQUIRED,
          .name = (schema[parent_idx].name == "list") ? "element" : col_meta.get_name(),
          .parent_idx  = parent_idx,
          .leaf_column = col
        };

My personal preference would be to avoid construct-then-initialize. But this is purely stylistic. Please feel free to ignore.

Originally posted by @mythrocks in #11328 (comment)

@github-actions
Copy link

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

@vyasr
Copy link
Contributor

vyasr commented Oct 21, 2022

@mythrocks @hyperbolic2346 I looked through the code in writer_impl.cu and I'm not sure what the best approach is here. Aggregate initialization is totally fine with (public) parent classes and members, but the issue with either aggregate initialization or the use of a constructor is that I see at least three variants of these objects in terms of what members are set. It's not clear to me if those are the only options, or if there might even be further combinations of attributes set in future iterations, each of which would necessitate a different constructor signature. With C++20 designated initializers we could do this very neatly, but I don't see a clean solution right now. Am I understanding correctly how the object could be used, or do you think there is a simpler constructor we could define to improve the code in its current state?

@GregoryKimball GregoryKimball added 0 - Waiting on Author Waiting for author to respond to review proposal Change current process or code cuIO cuIO issue and removed good first issue Good for newcomers labels Nov 21, 2022
@GregoryKimball GregoryKimball added the libcudf Affects libcudf (C++/CUDA) code. label Apr 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 - Waiting on Author Waiting for author to respond to review cuIO cuIO issue libcudf Affects libcudf (C++/CUDA) code. proposal Change current process or code
Projects
None yet
Development

No branches or pull requests

3 participants