iajoiner commented on pull request #9702: URL: https://github.com/apache/arrow/pull/9702#issuecomment-1016225237
@pitrou Please review again. There are several issues we still need to discuss: 1. What exactly should be done to `FileVersion`? Shall I create an `options.cc` which currently doesn't exist in order to put the `FileVersion::ToString` there? 2. What are the best practices related to generating a single random integer? Surprisingly I can't find any instance of system time being used as the seed in Arrow. Furthermore seeds in the code base are often constants. I wonder how to do so without hitting the same value all the time. 3. Shall I make `bloom_filter_columns` in `WriteOptions` a `std::shared_ptr<std::vector<int64_t>>`? It's usually a good practice to have a vector of shared pointers (as opposed to a shared pointer of a vector). Moreover given that int64_t is by default on the stack it doesn't seem that having a vector of ints is problematic. `std::vector<int64_t>` which I currently use both makes sense conceptually and is widely used in the Arrow source code. If we use this type then the move suggestion you made can not be acted upon. -- 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. To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
