-
Notifications
You must be signed in to change notification settings - Fork 0
Basic put/get Query record #98
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
Conversation
| let parsed_start_time = Self::parse_start_time(&query.start_time)?; | ||
| let pk = Self::query_pk(&parsed_start_time); | ||
| let sk = Self::query_sk(&parsed_start_time); | ||
| let pk = Self::query_pk(&query.start_time); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while we have pk as start_time here? I assumed pk should be shorten to date for pk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check the function query_pk
crates/state-store/src/models.rs
Outdated
| #[derive(Clone, Debug, Serialize, Deserialize)] | ||
| #[derive(Clone, Debug, Serialize, Deserialize, Default)] | ||
| pub struct Query { | ||
| pub query_id: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use strict data types?
pub query_id: Uuid,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about session_id, it's &str everywhere (but the value itself in uuid format)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean all fields datatypes, which we want to see a part of inerface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uuid is already there for query_id and request_id, the question about session_id.
in service we have &str as session_id, should we parse it to uuid for state store?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shortly I think it's worth to use strict type for persisted session_id as well. But it's up to you, I do not insist, especially if such datatype leads to significant efforts as of legacy code.
DanCodedThis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@osipovartem LGTM
Added basic example how to create inital record
Changed types of timestamps
Current problem
GSI cannot work with nested json field for indexing, that's why we insert additional columns for Query
the record looks like