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

WIP: [C++] Get rid of code duplication in Decimal##bit_width #8417

Closed
wants to merge 6 commits into from

Conversation

dchigarev
Copy link

PR to get rid of code duplication while declaring new Decimal##BIT_WIDTH##Type. Since there is a plans to add support for lower bit Decimals also, I think Decimal256 branch is the best place to decide how we want to define the mechanism of adding support for new Decimal types without a lot of code duplications.

There are probably some places where we can remove redundant defines and template help structures, so this is just a draft PR to have a base for discussion.

Signed-off-by: Dmitry Chigarev <[email protected]>
@github-actions
Copy link

github-actions bot commented Oct 9, 2020

Thanks for opening a pull request!

Could you open an issue for this pull request on JIRA?
https://issues.apache.org/jira/browse/ARROW

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

See also:

Signed-off-by: Dmitry Chigarev <[email protected]>
@emkornfield
Copy link
Contributor

I can look in a bit more detail but two high level comments:

  1. Lets not use macros in the header files for declaration.
    The style guide strongly discourages it
  2. For the Base decimal type, we need to make sure that this doesn't break Gandiva in some way (I wouldn't expect it to but the reason for the separation was because certain headers don't play well will LLVM).

Signed-off-by: Dmitry Chigarev <[email protected]>
@dchigarev
Copy link
Author

@emkornfield thanks for your comment!

  1. I've changed this PR a bit and get rid of declaring API via macroses. I've left it only at declaring decimals at type_fwd.h and type_traits.h because these files already using this pattern to declare alike classes, if this is acceptable I can leave macroses instead of code duplicating at these two files.

  2. These changes should not break BasicDecimal at Gandiva, because everything that was added to base decimals is decimal_meta.h which I guess do not have references to anything that could break Gandiva

Moving forward, I think that it could be useful if we'll declare some interface for BasicDecimal and Decimal classes, which will be implemented by BasicDecimal/Decimal128, 256, 64 .... For now, amount of methods in BasicDecimal128 and 256 are noticeably different, is it a kind of TODO to implement in 256 all methods that 128 has, or 256-bit decimal will have less methods on purpose? If it is, is there a list of basic methods that any decimal must have and whose declaration could be moved to a decimal interface?

@emkornfield
Copy link
Contributor

took a little more of a look, this mostly seems reasonable. I've opened PR to merge decimal256 #8475 and we should reopen this against master once that is merged (I can take a closer look then and I think @pitrou might also be interested).

@emkornfield
Copy link
Contributor

Moving forward, I think that it could be useful if we'll declare some interface for BasicDecimal and Decimal classes, which will be implemented by BasicDecimal/Decimal128, 256, 64 ....

An interface could be helpful for documentation purposes but in general, I don't think we want virtual methods (especially for BasicDecimal).

For now, amount of methods in BasicDecimal128 and 256 are noticeably different, is it a kind of TODO to implement in 256 all methods that 128 has, or 256-bit decimal will have less methods on purpose? If it is, is there a list of basic methods that any decimal must have and whose declaration could be moved to a decimal interface?

This is a TODO item.

@dchigarev
Copy link
Author

Closing in glory of reopened PR against master #8578

@dchigarev dchigarev closed this Nov 3, 2020
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.

2 participants