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

fix: update queries refactor #390

Merged
merged 1 commit into from
Feb 25, 2025
Merged

fix: update queries refactor #390

merged 1 commit into from
Feb 25, 2025

Conversation

pratikmishra356
Copy link
Collaborator

Problem

Redundant fetch and all columns update

Solution

Created new changeset request type for normal api request and added a function to map to changeset 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

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

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

@pratikmishra356 pratikmishra356 requested a review from a team as a code owner January 29, 2025 09:47
@pratikmishra356 pratikmishra356 force-pushed the update-query-refactor branch 4 times, most recently from f01a002 to 73a9e34 Compare January 30, 2025 08:17
Comment on lines 16 to 25
impl UpdateFunctionRequest {
pub fn as_changeset(self) -> UpdateFunctionRequestChangeset {
UpdateFunctionRequestChangeset {
draft_code: self.function.map(|x| BASE64_STANDARD.encode(x)),
draft_runtime_version: self.runtime_version,
description: self.description,
change_reason: self.change_reason,
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
impl UpdateFunctionRequest {
pub fn as_changeset(self) -> UpdateFunctionRequestChangeset {
UpdateFunctionRequestChangeset {
draft_code: self.function.map(|x| BASE64_STANDARD.encode(x)),
draft_runtime_version: self.runtime_version,
description: self.description,
change_reason: self.change_reason,
}
}
}
impl From<UpdateFunctionRequest> for UpdateFunctionRequestChangeset {
fn from(req: UpdateFunctionRequest) -> Self {
Self {
draft_code: req.function.map(|x| BASE64_STANDARD.encode(x)),
draft_runtime_version: req.runtime_version,
description: req.description,
change_reason: req.change_reason,
}
}
}

Comment on lines +128 to +123
dsl::draft_edited_by.eq(user.get_email()),
dsl::draft_edited_at.eq(Utc::now().naive_utc()),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

last_modified_at and last_modified_by not being updated here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ayushjain17 it is not meant to be updated here,
for published code changes it gets updated

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the update endpoint
and previously we were very much updating these two fields
ref

Comment on lines 57 to 37
function_name: match self.function_name {
Some(FunctionNameEnum::Name(val)) => Some(Some(val)),
Some(FunctionNameEnum::Remove) => Some(None),
_ => None,
},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets extract this as a separate property of FunctionNameEnum
something like:

impl FunctionNameEnum {
   fn to_option(&self) -> Option<String> {
      match self {
         Self::Name(name) => Some(name),
         Self::Remove => None,
      }
   }
}

Comment on lines 52 to 38
impl UpdateReq {
pub fn as_changeset(self) -> UpdateReqChangeset {
UpdateReqChangeset {
position: self.position.map(|x| x.into()),
schema: self.schema,
function_name: match self.function_name {
Some(FunctionNameEnum::Name(val)) => Some(Some(val)),
Some(FunctionNameEnum::Remove) => Some(None),
_ => None,
},
description: self.description,
change_reason: self.change_reason,
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
impl UpdateReq {
pub fn as_changeset(self) -> UpdateReqChangeset {
UpdateReqChangeset {
position: self.position.map(|x| x.into()),
schema: self.schema,
function_name: match self.function_name {
Some(FunctionNameEnum::Name(val)) => Some(Some(val)),
Some(FunctionNameEnum::Remove) => Some(None),
_ => None,
},
description: self.description,
change_reason: self.change_reason,
}
}
}
impl From<UpdateReq> for UpdateReqChangeset {
pub fn from(req: UpdateReq) -> Self {
Self {
position: req.position,
schema: req.schema,
function_name: req.function_name.map(FunctionNameEnum::to_option),
description: req.description,
change_reason: req.change_reason,
}
}
}

impl UpdateReq {
pub fn as_changeset(self) -> UpdateReqChangeset {
UpdateReqChangeset {
position: self.position.map(|x| x.into()),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is using into needed here ?
Position derives Deref, so it should be automatic na ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes its needed , deref does not removes the wrapper type

@@ -13,7 +14,7 @@ pub struct CreateReq {
pub change_reason: String,
}

#[derive(Debug, Deserialize)]
#[derive(Debug, Deserialize, Clone)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cloning entire req should not be needed

Comment on lines 218 to 217
let function_name = match req.function_name.clone() {
Some(FunctionNameEnum::Name(func_name)) => Some(func_name),
Some(FunctionNameEnum::Remove) => None,
None => existing.function_name.clone(),
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this still needed ?
AsChangeset already handles this, right ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is being used for validation in another function , thats why need to extract it

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in that case, can we use this:

Suggested change
let function_name = match req.function_name.clone() {
Some(FunctionNameEnum::Name(func_name)) => Some(func_name),
Some(FunctionNameEnum::Remove) => None,
None => existing.function_name.clone(),
};
let function_name = req.function_name.map_or_else(|| existing.function_name.clone(), |f| f.to_option());

@@ -75,6 +76,7 @@ pub fn put(
pub fn r#move(
old_ctx_id: String,
req: Json<MoveReq>,
req_description: String,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

req already contains the needed Description

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is of optional type ,
in move handler , at multiple places we needed non-option description
thats why we first check request or from db and then pass it to other utils function

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we not fix all of these and clean all these mess

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its not the scope of this PR,
anyway context apis are critical and a bit complex comparing to other apis , we can take it as an individual task to refactor implementation of context apis

@@ -495,8 +498,19 @@ async fn reduce_config_key(
if is_approve {
let _ = context::delete(cid.clone(), user, conn, schema_name);
if let Ok(put_req) = construct_new_payload(request_payload) {
let description = match put_req.description.clone() {
Some(val) => val,
None => ensure_description(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something seems wrong if we still need to use ensure_description even after all these changes

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pr was to remove redundant call not ensure description ,
this pr removes redundant update query and redundant ensure description call

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All that I am saying is that ensure_description should not at all be needed after removing these redundant queries

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its needed for config version table

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ayushjain17 ensure_description is being used in context handlers only
context apis are different like put and move request , where we need to have description either from the request
or from the db , ensure_description checks if contexts exist then fetch description from db
and if not then consider the request description and if missing in request then throw error

@@ -67,21 +67,23 @@ async fn put_handler(
schema_name: SchemaName,
) -> superposition::Result<HttpResponse> {
let tags = parse_config_tags(custom_headers.config_tags)?;
let description = match req.description.clone() {
Some(val) => val,
None => ensure_description(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not reviewing the changes around ensure_description

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is still pending on my part

@Datron Datron linked an issue Feb 7, 2025 that may be closed by this pull request
@pratikmishra356 pratikmishra356 force-pushed the update-query-refactor branch 5 times, most recently from f8f0e65 to 3ec789f Compare February 11, 2025 07:06
@pratikmishra356 pratikmishra356 force-pushed the update-query-refactor branch 3 times, most recently from d126437 to 8c9e659 Compare February 12, 2025 11:27
@@ -13,7 +13,6 @@ diesel::table! {
config_hash -> Text,
tags -> Nullable<Array<Varchar>>,
created_at -> Timestamp,
description -> Text,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't change_reason be removed and description be kept ?
because the change_reason of the action which created the snapshot becomes the description to the snapshot entry

@@ -1,8 +1,15 @@
use std::collections::{HashMap, HashSet};

use derive_more::{AsRef, Deref, DerefMut, Into};
// use diesel::{deserialize::{self, FromSql}, pg::{Pg, PgValue}, serialize::{self, Output, ToSql}};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be removed

#[cfg_attr(feature = "diesel_derives", derive(AsExpression, FromSqlRow))]
#[cfg_attr(feature = "diesel_derives", diesel(sql_type = Integer))]
#[serde(try_from = "i32")]
pub struct Position(i32);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this type (Position) and FunctionNameEnum be part of this file?
i feel like Position should be a part of models/cac.rs and for FunctionNameEnum it should be a part of the api subfolder in superposition_types

Copy link
Collaborator Author

@pratikmishra356 pratikmishra356 Feb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since Position is a part of Api as well as weight is derived from Position so it can be either in config.rs or in api/dimension.rs
FunctionNameEnum moved to function.rs file inside api

Copy link
Collaborator

@ayushjain17 ayushjain17 Feb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see, In Api we re-use stuff from other places which are more core in nature
models is the very core type on top which everything else is structured, so Apis do use types from other places and Position is not being used at that core level itself

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Position cant be in models/cac.rs , I think models folder should refer to only table struct ,
other core types can be moved to other folders or files like api, config etc

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing we can do is keep them in both places. You can define them in models & then re-export it from api & then use the api import to have the same effect.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

api and config are not core types, thats the misunderstanding we are having here
the very basis of both api and config types are models
not vice versa

#[cfg(feature = "diesel_derives")]
impl ToSql<Integer, Pg> for Position {
fn to_sql<'b>(&'b self, out: &mut Output<'b, '_, Pg>) -> SResult {
let value = self.to_owned().into();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why to_owned ?
any ways you need to pass reference to to_sql function

wont this be enough ?

Suggested change
let value = self.to_owned().into();
let value = self.into();

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No this doesn't work as Into trait implies for Position and not referenced values

let update_position =
Position::try_from(new_position).map_err(|err| unexpected_error!(err))?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this still needed ?

Comment on lines 218 to 217
let function_name = match req.function_name.clone() {
Some(FunctionNameEnum::Name(func_name)) => Some(func_name),
Some(FunctionNameEnum::Remove) => None,
None => existing.function_name.clone(),
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in that case, can we use this:

Suggested change
let function_name = match req.function_name.clone() {
Some(FunctionNameEnum::Name(func_name)) => Some(func_name),
Some(FunctionNameEnum::Remove) => None,
None => existing.function_name.clone(),
};
let function_name = req.function_name.map_or_else(|| existing.function_name.clone(), |f| f.to_option());

@@ -75,6 +76,7 @@ pub fn put(
pub fn r#move(
old_ctx_id: String,
req: Json<MoveReq>,
req_description: String,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we not fix all of these and clean all these mess

@@ -67,21 +67,23 @@ async fn put_handler(
schema_name: SchemaName,
) -> superposition::Result<HttpResponse> {
let tags = parse_config_tags(custom_headers.config_tags)?;
let description = match req.description.clone() {
Some(val) => val,
None => ensure_description(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is still pending on my part

@pratikmishra356 pratikmishra356 force-pushed the update-query-refactor branch 2 times, most recently from 503d81a to 4d11865 Compare February 13, 2025 07:58
@pratikmishra356 pratikmishra356 force-pushed the update-query-refactor branch 6 times, most recently from b172c42 to 7b50e2f Compare February 14, 2025 07:12
pub schema: Option<Value>,
//function_name is nullable column, so to support null update with changeset
//we had to make it Option<Option<String>>
pub function_name: Option<Option<String>>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also why do we support removal of the function name? Isn't allowing people to change it good enough?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AND, why do we have FunctionNameEnum? Why not just Option<Option<String>> there as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes , it has been tested
that's subjective decision , if we let user to remove or only replace
Option<Option>> wont work on the request type since we cant derive option<option type after deserliaze

impl From<UpdateFunctionRequest> for UpdateFunctionRequestChangeset {
fn from(req: UpdateFunctionRequest) -> Self {
Self {
draft_code: req.function.map(|x| BASE64_STANDARD.encode(x)),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we create a new-type for this which automatically b64 decodes & encodes in to-and-from sql?
CC: @ayushjain17

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That way you might be able to avoid creating this interface.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this PR was just for refactoring update queries 🤔 💭

use serde_json::Value;

#[derive(Debug, Clone)]
pub enum FunctionNameEnum {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can just use Option<FunctionName>.

Copy link
Collaborator Author

@pratikmishra356 pratikmishra356 Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No we cant, as we have to support remove case

#[cfg_attr(feature = "diesel_derives", derive(AsExpression, FromSqlRow))]
#[cfg_attr(feature = "diesel_derives", diesel(sql_type = Integer))]
#[serde(try_from = "i32")]
pub struct Position(i32);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing we can do is keep them in both places. You can define them in models & then re-export it from api & then use the api import to have the same effect.

@pratikmishra356 pratikmishra356 force-pushed the update-query-refactor branch 8 times, most recently from e73ea65 to b0c5943 Compare February 21, 2025 10:14
D: Deserializer<'de>,
{
let map: Option<Value> = Deserialize::deserialize(deserializer)?;
log::error!("function xxx {:?}", map.clone());
Copy link
Collaborator

@ayushjain17 ayushjain17 Feb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return of Xander Cage ?

@@ -16,6 +16,7 @@ use std::future::{ready, Ready};

#[cfg(feature = "server")]
use actix_web::{dev::Payload, FromRequest, HttpMessage, HttpRequest};
use base64 as _;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this ?
simply make bse64 option dependency and add it in the list of dependencies for diesel_derives

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the diff in this file ?
ideally there should not be any change in this file as there is no db level change being made in this PR

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh just checked, we are removing a column, but the patch file does not seem to be correct for that change, @pratikmishra356 can you please verify

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have verified we do not need any change in schema.patch
and this mandatory dimensions change is already there in main , the changes are same ideally it should not show something like this , will check after commit squash , not a breaking change

#[serde(try_from = "i32")]
pub struct Position(i32);
impl Position {
fn validate_data(position_val: i32) -> Result<Self, String> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this separately ?
should we move this directly inside try_from ?

use serde_json::Value;
#[cfg(feature = "diesel_derives")]
use std::str;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this being used ?

also, can we follow this:

std package imports
{{new_line}}
package imports
{{new_line}}
crate imports
{{new_line}}
super imports
{{new_line}}
self imports

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes its being used

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is not right, std::str is a standard package, which should be moved at the top

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I feel that maintaining such things w/o CI checks is moot. You can't really rely on it being there through out the code base, just in files you were able to manage. Moreover it's there today & somebody may forget/screw it up tomorrow. To me it seems like fruitless work. How about we add the necessary checks via rustfmt instead?

@@ -9,6 +9,7 @@ edition = "2021"
# env
actix-web = { workspace = true, optional = true }
anyhow = { workspace = true, optional = true }
base64 = { workspace = true }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
base64 = { workspace = true }
base64 = { workspace = true, optional = true }

@@ -294,7 +294,6 @@ pub fn add_config_version(
state: &Data<AppState>,
tags: Option<Vec<String>>,
description: String,
change_reason: String,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be kept and the other removed, right ?

Copy link
Collaborator Author

@pratikmishra356 pratikmishra356 Feb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are removing change_reason and keeping description
https://github.com/juspay/superposition/pull/390/files#r1952583143

Comment on lines 12 to 19
pub struct UpdateFunctionRequest {
pub function: Option<String>,
pub runtime_version: Option<String>,
#[serde(rename = "function")]
pub draft_code: Option<FunctionCode>,
#[serde(rename = "runtime_version")]
pub draft_runtime_version: Option<String>,
pub description: Option<String>,
pub change_reason: String,
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we avoid such stuff?
since we dont have an api doc, these types are our only source for api spec and this sort of makes it complicated

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, we're planning to do a breaking change to this API & the frontend already has the contract so not needed now.
CC: @pratikmishra356

pub struct UpdateReq {
pub position: Option<Position>,
pub schema: Option<Value>,
pub function_name: Option<FunctionNameEnum>,
#[serde(default, deserialize_with = "deserialize_function_name")]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default is not needed, right ?

Suggested change
#[serde(default, deserialize_with = "deserialize_function_name")]
#[serde(deserialize_with = "deserialize_function_name")]

pub struct UpdateReq {
pub position: Option<Position>,
pub schema: Option<Value>,
pub function_name: Option<FunctionNameEnum>,
#[serde(default, deserialize_with = "deserialize_function_name")]
pub function_name: Option<Option<String>>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, same point, changing here its like, when these types become common, which they are on the its way of becoming common

in the future sdk, the user would have to send data like: Some(None), Some(Some(function_name))
does not make sense in that way

over here, as long as we are seeing this type as something which is constructed only while deserialising, it can make sense, but in the longer run it just simply does not look correct

pub schema: Option<Map<String, Value>>,
pub function_name: Option<FunctionNameEnum>,
pub schema: Option<Value>,
#[serde(deserialize_with = "deserialize_function_name")]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and then this requires us to keep writing this everywhere,
I would still say that there should be a separate type for this use case, as this is an action item which needs to be specify and not simply a param

making a separate type for this, clarifies the intent as well

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since were making a type, what's a good name for the field? I think something like:
function_association: Option<FunctionAssociation>

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do this in next Pr ,
created the issue
#428

@pratikmishra356 pratikmishra356 force-pushed the update-query-refactor branch 3 times, most recently from e008f27 to cfca186 Compare February 24, 2025 13:52
Copy link
Collaborator

@ayushjain17 ayushjain17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pending points being picked up later

@Datron Datron linked an issue Feb 25, 2025 that may be closed by this pull request
@Datron Datron linked an issue Feb 25, 2025 that may be closed by this pull request
@pratikmishra356 pratikmishra356 merged commit 258933f into main Feb 25, 2025
4 checks passed
@pratikmishra356 pratikmishra356 deleted the update-query-refactor branch February 25, 2025 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants