feature/choose-hyper-version #296
Conversation
|
@davidsonhr1 Have you checked what happens if a version 2 writer attempts to write to an existing version 0 or version 1 hyper file? |
So far I have only tested with files that I use in my company and they are version 2, I have only tested the scenario of a 100% new file, but I can test other scenarios |
|
How old is the version 2? I very loosely only have pantab supporting technologies that are 1.5 to 2 years max. If version 2 has been out that long, I would also be fine to make that the default value. Of course still want to know a little bit more about what @jorwoods is asking - I would hate to change that value and break things for users that are appending to Hyper databases created with prior verions of pantab |
|
If it's fully backwards compatible, then defaulting to version 2 might be preferable. |
| @@ -540,7 +541,8 @@ void write_to_hyper( | |||
| } | |||
|
|
|||
| const std::unordered_map<std::string, std::string> params = { | |||
| {"log_config", ""}}; | |||
| {"log_config", ""}, {"default_database_version", std::to_string(version)}}; | |||
There was a problem hiding this comment.
Are there any other configuration items worth passing through? The other design consideration is if we want to explicitly enumerate these in our API (which puts some onus on us to manage backwards compatability) or if we would rather just accept a dictionary argument for hyper_params
There was a problem hiding this comment.
I like the way of letting the user pass a dictionary of parameters, as each person may want to use it differently, there are other parameters in the hyperprocess, such as "use_tcp_port"
There was a problem hiding this comment.
Sounds good. Let's try that here then. I don't know that the Hyper C++ API can handle complete forwarding of the dict, but we can at least unpack the ones we know about in the extension layer via nb::try_cast and warn for any others we cannot process
| @@ -519,7 +519,8 @@ void write_to_hyper( | |||
| const std::map<SchemaAndTableName, nb::capsule> &dict_of_capsules, | |||
| const std::string &path, const std::string &table_mode, | |||
| nb::iterable not_null_columns, nb::iterable json_columns, | |||
| nb::iterable geo_columns) { | |||
| nb::iterable geo_columns, | |||
| const int8_t version) { | |||
There was a problem hiding this comment.
Since the HyperProcess constructor expects all parameters to be strings, I think you can just declare a parameter here of const std::map<std::string, std::string> process_params and pass that to the HyperProcess constructor.
To make it easier you can map all of the parameter values to strings in Python before sending through to the extension
A hyper file version support on hyperprocess