-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Serialize decimal quantiles #9653
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - LGTM: two comments:
LogicalType type; | ||
deserializer.ReadProperty(104, "logical_type", type); | ||
|
||
switch (deserialization_kind) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only comment is that I think this would be cleaner and more consistent with the rest of the code if it were turned into an inheritance chain. You could then use the serialization generation script to generate this code.
Also turning the "flag" into "type" enum. Remember to run the generate_enum_util
script too.
Previous one was autogenerated
Solution is somehow custom-made, open to suggestions on how to make it more similar to the rest of the Serialization code.
Two main changes (independent but both needed):
src/function/function_set.cpp
This PR will make so that databases that serialize quantile decimal to storage could not be read by previous version of DuckDB. But given the current case is that those statement are illegal to serialize, it should be anyhow be an improvement.
Main improvement I can think of (and that probably I should implement straight away), is moving the integer flag to an enum. There is anything else that needs addressing?