jorgecarleitao edited a comment on pull request #9232: URL: https://github.com/apache/arrow/pull/9232#issuecomment-766319983
Hi @ovr , I went through what is here so far. First of all, great stuff that you are taking this on. Broadly speaking, this PR currently contains the following "steps": 1. make changes to the arrow crate to enable Decimal type without adding any new functionality (e.g. rename `datatypes.rs`) 2. add the native type to the arrow crate 3. add the Array type to the arrow crate 4. add support to parquet 5. add support to DataFusion I suggest that 4-5 are done on a separate PR. 1-3 can be done on this PR, but I suggest that we split them on separate commits. Concerning step 2, the two main checks in the list are: 1. the type's logical representation exists in the arrows' spec 2. the type's physical representation matches arrows' spec Looking at [the spec](https://github.com/apache/arrow/blob/master/format/Schema.fbs#L186), a decimal type only supports 128 and 256 bits. So, I am do not understand why we are trying to add support for `Int32,Int64,Int128,LargeDecimal` here. Since Rust only supports `i128`, I would say that we should only go for `i128` (the same reason we do not support `f16`). EDIT: I searched for the wrong crate name ^_^ Could you clarify some of these points? ---------------------------------------------------------------- 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: us...@infra.apache.org
