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

Inconsistencies to consider for cleanup #141

Open
AlexanderSaydakov opened this issue Mar 7, 2025 · 0 comments
Open

Inconsistencies to consider for cleanup #141

AlexanderSaydakov opened this issue Mar 7, 2025 · 0 comments

Comments

@AlexanderSaydakov
Copy link
Contributor

Despite of this code being quite recent we have already accumulated some inconsistencies. This is about aggregate functions.

  • Initialize state object (sketch or union) in initialState() or not. At some point we realized that aggregate() can be called after deserialize(), in which case initialState() is not called, so we still need lazy initialization in the aggregate(), and therefore there is no point having it in initialState(). but we did not clean up everywhere
  • At some point we added parameters to functions, but forgot to pass them through serialize-deserialize. So we thought that instead of creating a new state in serialize() it might be a better approach to return the existing state. While creating a new state we need to remember to put into it all parameters which might be needed to create a sketch or union object later. While passing the existing one, we need to remember to remove the sketch or union object (after converting it to a binary blob). It is not conclusive which way is cleaner, and we have a mix of both approaches now.
  • It seems that aggregate() is not even called by BQ with null inputs, so checking for nulls there is useless, and we should be ready that aggregate() might never be called
  • Regarding naming convention. We decided to have “agg” in the names of aggregate functions, but somehow did not follow through. For instance, we have kll_sketch_float_build instead of kll_sketch_float_agg_build. Same with merge function and with some other sketches.
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

No branches or pull requests

1 participant