feat: priority recompute restructure #279

pratikmishra356 · 2024-11-07T10:15:31Z

Problem

User has to declare this exponential value, as value is exponential it has limitation on the total number of dimensions that can be defined

Solution

Postgresql's Numeric Type Approach

For dimension we can have another column named order which will store order of dimension's specificity ,
the higher order is means more specific
For context , we will have another column named weightage , where we will calculate the value just like we do now
it will summation of (2^ dimension's order value)

since context's weightage value can be too large , so we will use postgresql Numeric type

CREATE TABLE large_numbers (
id SERIAL PRIMARY KEY,
big_value NUMERIC
);
Rust/ Diesel supports BigDecimal type which is compatible with Numeric type

Environment variable changes

What ENVs need to be added or changed

Pre-deployment activity

Things needed to be done before deploying this change (if any)

Post-deployment activity

link for migration/backward compatibility: #277 (comment)

API changes

Endpoint	Method	Request body	Response Body
API	GET/POST, etc	request	response

Possible Issues in the future

Describe any possible issues that could occur because of this change

Datron · 2024-11-18T11:21:00Z

crates/context_aware_config/migrations/2024-11-05-083200_add_priority_restructure/up.sql

+ALTER TABLE public.dimensions
+add column position integer DEFAULT 1 NOT NULL;
+
+ALTER TABLE public.contexts


Can we write a query to copy priority to weightage

@Datron we can write , but we will anyway have to re populate dimension position and then recompute

migration approach: #277 (comment)

we can use LOG function to backfill
UPDATE public.contexts SET position = FLOOR(LOG(2, priority)) WHERE priority > 0;

crates/context_aware_config/migrations/2024-11-05-083200_add_priority_restructure/up.sql

Datron · 2024-11-18T11:23:35Z

crates/context_aware_config/Cargo.toml

@@ -11,6 +11,7 @@ actix-http = "3.3.1"
 actix-web = { workspace = true }
 anyhow = { workspace = true }
 base64 = { workspace = true }
+bigdecimal = { version = "0.3.1" , features= ["serde"]}


Do we need bigdecimal?

Yes @Datron we need to calculate higher value

Datron · 2024-11-18T11:26:24Z

crates/context_aware_config/src/helpers.rs

@@ -38,6 +42,12 @@ use crate::db::{
    },
 };

+pub struct DimensionData {


derive debug and Clone

Datron · 2024-11-18T11:26:59Z

crates/context_aware_config/src/helpers.rs

@@ -38,6 +42,12 @@ use crate::db::{
    },
 };

+pub struct DimensionData {
+    pub schema: JSONSchema,
+    pub priority: i32,


Suggested change

pub priority: i32,

pub weight: i32,

@Datron it will be priority only,
I refactored a bit here , changed from tuples to struct

@pratikmishra356 will this go away once we migrate all data and the frontend?

Datron · 2024-11-18T11:29:48Z

crates/context_aware_config/src/helpers.rs

+            .get(dimension.as_str())
+            .map(|x| x.position)
+            .ok_or_else(|| {
+                let msg = String::from("Dimension not found in Dimension schema map");


Can we also log the name of the dimension we couldn't parse?

Datron · 2024-11-18T11:30:48Z

crates/context_aware_config/migrations/2024-11-05-083200_add_priority_restructure/up.sql

+add column position integer DEFAULT 1 NOT NULL;
+
+ALTER TABLE public.contexts
+add column weightage numeric(1000,0) DEFAULT 1 NOT NULL;


Should we call this weight?

we can , lets check with team also

Datron · 2024-11-18T11:31:49Z

crates/context_aware_config/src/api/config/handlers.rs

@@ -527,7 +529,8 @@ async fn reduce_config(
        .and_then(|value| value.to_str().ok().and_then(|s| s.parse::<bool>().ok()))
        .unwrap_or(false);

-    let dimensions_schema_map = get_all_dimension_schema_map(&mut conn)?;
+    let dimensions_vec = get_dimension_data(&mut conn)?;


Should get_dimension_data return a map?

No , it does not need to be , i have separated it into 2 function and get_dimension_data_map this function returns only values which are needed

but do we need the 2 functions separately ?

Datron · 2024-11-18T11:35:55Z

crates/context_aware_config/src/api/context/types.rs

+    pub old_weightage: BigDecimal,
+    pub new_weightage: BigDecimal,


Suggested change

pub old_weightage: BigDecimal,

pub new_weightage: BigDecimal,

pub old_weight: BigDecimal,

pub new_weight: BigDecimal,

Datron · 2024-11-18T11:51:11Z

crates/context_aware_config/src/api/dimension/types.rs

+impl Position {
+    fn validate_data(position_val: Option<i32>) -> Result<Self, String> {
+        if let Some(val) = position_val {
+            if val < 0 {


We should allow 0 right? 2^0 is 1

yes we are allowing 0

Datron · 2024-11-25T07:29:59Z

crates/context_aware_config/src/api/dimension/handlers.rs

+    conn.transaction::<_, superposition::AppError, _>(|transaction_conn| {
+        match (prev_index, new_index.clone(), req_position_val.clone()) {
+            (Some(prev_val), Some(new_val), None) => {
+                println!("1");


Remove this

Datron · 2024-11-25T07:31:49Z

crates/context_aware_config/src/helpers.rs

@@ -38,6 +42,12 @@ use crate::db::{
    },
 };

+pub struct DimensionData {
+    pub schema: JSONSchema,
+    pub priority: i32,


@pratikmishra356 will this go away once we migrate all data and the frontend?

Datron · 2024-11-25T07:33:15Z

crates/context_aware_config/src/api/dimension/handlers.rs

+        .order(priority.asc())
+        .select((dimension, priority, position))
+        .load::<(String, i32, i32)>(&mut conn)
+        .expect("Error loading dimensions");


No expects, we should throw DB error

yes priority field will go away.

Datron · 2024-11-25T07:37:32Z

crates/context_aware_config/src/api/dimension/handlers.rs

+            (Some(prev_val), Some(new_val), None) => {
+                println!("1");
+                if prev_val < new_val {
+                    new_dimension.position = new_val.clone() as i32 - 1;


Why are we doing this check? @pratikmishra356 can we add some comments here and use better variable names to document whats happening?

Sorry , missed this will have to write comments in code to explain this

Datron · 2024-11-25T07:42:54Z

crates/context_aware_config/src/api/dimension/types.rs

@@ -35,6 +36,36 @@ impl TryFrom<i32> for Priority {
    }
 }

+#[derive(Debug, Deserialize, AsRef, Deref, DerefMut, Into)]
+#[serde(try_from = "Option<i32>")]
+pub struct Position(Option<i32>);


Any reason for wrapping Option instead of i32 directly? You can do Option then, rather than deal with None and Some everywhere

For now from api request it should be optional till we move completely from priority
i can do either Option<Position(i32)> or Position<option)
either way i will have to handle some and None.
option will not be there post migration

either case, it should be Option<Position>, where it is Position(i64)
because the parameter is optional, but currently it reads the parameter is mandatory which in turn takes optional value

and when you would want to make it mandatory, you would want to change only the request type and not the properties of the type

ayushjain17 · 2024-11-25T08:16:17Z

Cargo.toml

@@ -43,6 +43,7 @@ diesel = { version = "2.1.0", features = [
    "chrono",
    "uuid",
    "postgres_backend",
+    "numeric",


should we move this feature to context-aware-config only

ayushjain17 · 2024-11-25T08:35:10Z

crates/context_aware_config/migrations/2024-11-05-083200_add_priority_restructure/up.sql

+ALTER TABLE public.dimensions
+add column position integer DEFAULT 1 NOT NULL;
+
+ALTER TABLE public.contexts


we can use LOG function to backfill
UPDATE public.contexts SET position = FLOOR(LOG(2, priority)) WHERE priority > 0;

ayushjain17 · 2024-11-25T08:40:09Z

crates/context_aware_config/src/api/config/handlers.rs

@@ -527,7 +529,8 @@ async fn reduce_config(
        .and_then(|value| value.to_str().ok().and_then(|s| s.parse::<bool>().ok()))
        .unwrap_or(false);

-    let dimensions_schema_map = get_all_dimension_schema_map(&mut conn)?;
+    let dimensions_vec = get_dimension_data(&mut conn)?;


but do we need the 2 functions separately ?

ayushjain17 · 2024-11-25T09:30:18Z

crates/context_aware_config/src/api/dimension/types.rs

@@ -10,6 +10,7 @@ use crate::db::models::Dimension;
 pub struct CreateReq {
    pub dimension: DimensionName,
    pub priority: Priority,
+    pub position: Position,


shouldn't the type here be Option<Position>

get_dimension_data_map is a util function on dimensions data
so its better to have fetch dimensions function separately

we can use LOG function to backfill
UPDATE public.contexts SET position = FLOOR(LOG(2, priority)) WHERE priority > 0;

during deployment priority value might need not be of power 2
not needed i have added a new api for this

crates/context_aware_config/src/api/dimension/types.rs

ayushjain17 · 2024-11-25T10:34:35Z

crates/context_aware_config/src/helpers.rs

+    let base = BigUint::from(2u32);
+    let result = base.pow(index);
+    let biguint_str = &result.to_str_radix(10);
+    BigDecimal::from_str_radix(&biguint_str, 10).map_err(|err| {


is this correct ?
seems like we are sending address of address over here

biguint_str is already storing an address and then we are passing &biguint_str

crates/context_aware_config/src/helpers.rs

ayushjain17 · 2024-11-25T11:22:20Z

crates/context_aware_config/src/api/dimension/handlers.rs

pending review

Datron · 2024-11-25T11:52:41Z

crates/context_aware_config/src/api/dimension/handlers.rs

+        .map_err(|err| {
+            log::error!("failed to fetch dimensions with error: {}", err);
+            unexpected_error!("Something went wrong")
+        })?;


Should we throw a DB error. Also

Suggested change

.map_err(|err| {

log::error!("failed to fetch dimensions with error: {}", err);

unexpected_error!("Something went wrong")

})?;

.map_err(|err| {

log::error!("failed to fetch dimensions with error: {}", err);

unexpected_error!("Could not process this request due to a database error. Please reach out to your admin")

})?;

Datron

Small nitpick, can we resolve this quickly

pratikmishra356 requested a review from a team as a code owner November 7, 2024 10:15

pratikmishra356 force-pushed the priority-value-restructure branch 9 times, most recently from 14c0833 to 40cd334 Compare November 18, 2024 06:57

Datron requested changes Nov 18, 2024

View reviewed changes

pratikmishra356 force-pushed the priority-value-restructure branch 3 times, most recently from 286c0d0 to ec03437 Compare November 22, 2024 06:55

pratikmishra356 requested a review from Datron November 22, 2024 12:18

pratikmishra356 force-pushed the priority-value-restructure branch 2 times, most recently from 94a06eb to a7fba91 Compare November 22, 2024 12:37

Datron requested changes Nov 25, 2024

View reviewed changes

pratikmishra356 force-pushed the priority-value-restructure branch from a7fba91 to 6ce4b15 Compare November 25, 2024 11:18

ayushjain17 requested changes Nov 25, 2024

View reviewed changes

Datron reviewed Nov 25, 2024

View reviewed changes

Datron approved these changes Nov 25, 2024

View reviewed changes

pratikmishra356 force-pushed the priority-value-restructure branch from ed78722 to 94f2875 Compare November 27, 2024 06:41

pratikmishra356 requested a review from ayushjain17 November 27, 2024 06:49

Pratik Mishra added 6 commits November 27, 2024 18:09

feat: priority recompute restructure

1523d2f

feat: weightage recompute api

1412cf0

fix: logic for consistent migration

67e7b7f

fix: weightage to weight

87312ac

fix: refactored logic

04e7ef4

fix: db types migration rebase

10b00bf

pratikmishra356 force-pushed the priority-value-restructure branch from 94f2875 to 10b00bf Compare November 27, 2024 12:39

		pub old_weightage: BigDecimal,
		pub new_weightage: BigDecimal,

feat: priority recompute restructure #279

Are you sure you want to change the base?

feat: priority recompute restructure #279

Conversation

pratikmishra356 commented Nov 7, 2024 • edited Loading

Problem

Solution

Environment variable changes

Pre-deployment activity

Post-deployment activity

API changes

Possible Issues in the future

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayushjain17 Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayushjain17 Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Datron left a comment

Choose a reason for hiding this comment

pratikmishra356 commented Nov 7, 2024 •

edited

Loading

ayushjain17 Nov 25, 2024 •

edited

Loading

ayushjain17 Nov 25, 2024 •

edited

Loading