Skip to content
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

Merged
merged 7 commits into from
Nov 13, 2023
Merged

Conversation

carlopi
Copy link
Contributor

@carlopi carlopi commented Nov 13, 2023

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):

  • Have that DECIMAL and DECIMAL(X,Y) are a match while looking for overloads in src/function/function_set.cpp
  • Encode (as a optionally serialisable field) type information (both on function and argument)

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?

@carlopi carlopi requested review from Maxxen and Mytherin November 13, 2023 09:01
Copy link
Collaborator

@Mytherin Mytherin left a 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:

src/core_functions/aggregate/holistic/quantile.cpp Outdated Show resolved Hide resolved
src/core_functions/aggregate/holistic/quantile.cpp Outdated Show resolved Hide resolved
LogicalType type;
deserializer.ReadProperty(104, "logical_type", type);

switch (deserialization_kind) {
Copy link
Member

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.

@github-actions github-actions bot marked this pull request as draft November 13, 2023 12:12
@carlopi carlopi marked this pull request as ready for review November 13, 2023 12:12
@github-actions github-actions bot marked this pull request as draft November 13, 2023 12:29
@carlopi carlopi marked this pull request as ready for review November 13, 2023 12:29
@carlopi carlopi marked this pull request as draft November 13, 2023 12:47
@Mytherin Mytherin marked this pull request as ready for review November 13, 2023 12:49
@carlopi
Copy link
Contributor Author

carlopi commented Nov 13, 2023

Thanks @Maxxen and @Mytherin for helping iterating on this.

Agree with @Maxxen that the implementation via inheritance would be cleaner, but I have to say I was not completely comfortable seeing how to do it.

@Mytherin Mytherin merged commit fb59632 into duckdb:main Nov 13, 2023
@carlopi carlopi deleted the serialize_quantiles branch November 13, 2023 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants