You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/10/09 21:24:53 UTC
[GitHub] [arrow] dchigarev opened a new pull request #8417: WIP: [C++] Get rid of code duplication in Decimal##bit_width
dchigarev opened a new pull request #8417:
URL: https://github.com/apache/arrow/pull/8417
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #8417: WIP: [C++] Get rid of code duplication in Decimal##bit_width
Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #8417:
URL: https://github.com/apache/arrow/pull/8417#issuecomment-713343383
took a little more of a look, this mostly seems reasonable. I've opened PR to merge decimal256 https://github.com/apache/arrow/pull/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).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] dchigarev commented on pull request #8417: WIP: [C++] Get rid of code duplication in Decimal##bit_width
Posted by GitBox <gi...@apache.org>.
dchigarev commented on pull request #8417:
URL: https://github.com/apache/arrow/pull/8417#issuecomment-707133198
@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`](https://github.com/dchigarev/arrow/blob/4346e4e7553af0ef053668c24ca32df61a8deeb1/cpp/src/arrow/type_fwd.h#L147-L157) and [`type_traits.h`](https://github.com/dchigarev/arrow/blob/4346e4e7553af0ef053668c24ca32df61a8deeb1/cpp/src/arrow/type_traits.h#L285-L297) because these files [already using](https://github.com/dchigarev/arrow/blob/4346e4e7553af0ef053668c24ca32df61a8deeb1/cpp/src/arrow/type_fwd.h#L183-L202) 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`](https://github.com/dchigarev/arrow/blob/d256/cpp/src/arrow/util/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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #8417: WIP: [C++] Get rid of code duplication in Decimal##bit_width
Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #8417:
URL: https://github.com/apache/arrow/pull/8417#issuecomment-706437971
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](https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros)
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).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #8417: WIP: [C++] Get rid of code duplication in Decimal##bit_width
Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #8417:
URL: https://github.com/apache/arrow/pull/8417#issuecomment-714209176
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8417: WIP: [C++] Get rid of code duplication in Decimal##bit_width
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8417:
URL: https://github.com/apache/arrow/pull/8417#issuecomment-706413313
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
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:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] dchigarev commented on pull request #8417: WIP: [C++] Get rid of code duplication in Decimal##bit_width
Posted by GitBox <gi...@apache.org>.
dchigarev commented on pull request #8417:
URL: https://github.com/apache/arrow/pull/8417#issuecomment-721101059
Closing in glory of reopened PR against master #8578
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] dchigarev closed pull request #8417: WIP: [C++] Get rid of code duplication in Decimal##bit_width
Posted by GitBox <gi...@apache.org>.
dchigarev closed pull request #8417:
URL: https://github.com/apache/arrow/pull/8417
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org