-
Notifications
You must be signed in to change notification settings - Fork 163
feat: configurable decimal precision/scale for unbounded numeric #3694
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
base: main
Are you sure you want to change the base?
feat: configurable decimal precision/scale for unbounded numeric #3694
Conversation
|
@Raviikumar001 appreciate the initiative and contribution here. Configurable decimal precision/scale for unbounded numeric is a great feature, however it's important that it is implemented at the column-level rather than via global environment variable, in order to accommodate different columns with different precision/scale requirement. (historically we have used env var to store data-type state, and each time it had bit us back and we'd regret going down that path). In the codebase we already have the concept of "SupportedDestinationTypes". To support configurable decimal precision/scale, that would be the better entry point to introduce this feature (i.e. introduce a TL;DR is this piece is probably going to be a bigger project. If you have ideas on improving the architecture to make type conversion more extensible for scale/precision, would love to hear them. |
Thanks for you're response, really appreciate it, i'll have to think on this, pls give me some time, i'll share my thoughts on you're query soon! |
@jgao54 Apologies for the lengthy comment 🙇♂️ PR 1 — Foundation (column-level spec + ClickHouse support) Core changes:
Tests:
Migration:
PR 2 — Expansion (UI, additional destinations, policies, deprecation) Enhancements:
Testing:
Splitting this into two PRs keeps the core architectural change (single, column-scoped decimal spec + schema/value unification) focused and reviewable, while the second PR handles UI and multi-destination extensions once the foundation is solid. Please let me know you're thoughts on this — happy to adjust. Though some testing cannot done on my end(from my laptop) as mentioned above in testing section, i'd really apppreciate you thoughts on my suggestions. |
|
@Raviikumar001 thanks for providing your feedback, the general direction does sound right to me. Unfortunately, at the moment given our team's limited capacity, frankly we would not be able to support architectural changes at this scale, mostly due to the state changes this requires across the board (temporal/db/avro). It's probably not the best time to introduce this feature; but I expect this to be something we'll revisit sometimes next year. |
Thanks for the clarification, that makes sense. I'll be sure to ping you next year, regarding this if you're planning to go through to implement this feature, I'll be more adept in my skills and i'd love to contribute in it in any capacity. Thanks!! |
closes #3336
Summary
clickhouse_numeric_default_precisionandclickhouse_numeric_default_scale.PEERDB_CLICKHOUSE_NUMERIC_DEFAULT_PRECISIONandPEERDB_CLICKHOUSE_NUMERIC_DEFAULT_SCALEDetermineNumericSettingForDWH, ToDWHColumnType, GetNumericDestinationType) to accept overrides; bounded NUMERIC unchanged.[clickhouseNumericDefaultPrecision], [clickhouseNumericDefaultScale]) and update handlers.
-Rust: update FlowConnectionConfigs initializer to include the new optional fields.