[ENH]: Get collections impl #6146
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Implement sysdb Adds a full Rust-side implementation of the SysDB Key Changes• Introduced Affected Areas• rust/rust-sysdb/src/types.rs This summary was automatically generated by @propel-code-bot |
![propel-code-bot[bot]](https://avatars.githubusercontent.com/in/1179004?s=60&v=4){kind=small}
| if let Some(limit) = req.limit { | ||
| filter = filter.limit(limit as u32); | ||
| } | ||
| if let Some(offset) = req.offset { | ||
| filter = filter.offset(offset as u32); | ||
| } |
There was a problem hiding this comment.
[Logic] Potential integer underflow/wrapping risk: If the protobuf limit or offset fields are signed int32 (standard in proto3), casting a negative value to u32 will result in a very large number (e.g., -1 becomes u32::MAX).
Use try_from to safely validate that values are non-negative before casting.
| if let Some(limit) = req.limit { | |
| filter = filter.limit(limit as u32); | |
| } | |
| if let Some(offset) = req.offset { | |
| filter = filter.offset(offset as u32); | |
| } | |
| // Handle limit and offset | |
| if let Some(limit) = req.limit { | |
| let limit = u32::try_from(limit).map_err(|_| SysDbError::InvalidArgument("Limit must be non-negative".to_string()))?; | |
| filter = filter.limit(limit); | |
| } | |
| if let Some(offset) = req.offset { | |
| let offset = u32::try_from(offset).map_err(|_| SysDbError::InvalidArgument("Offset must be non-negative".to_string()))?; | |
| filter = filter.offset(offset); | |
| } |
Context for Agents
Potential integer underflow/wrapping risk: If the protobuf `limit` or `offset` fields are signed `int32` (standard in proto3), casting a negative value to `u32` will result in a very large number (e.g., `-1` becomes `u32::MAX`).
Use `try_from` to safely validate that values are non-negative before casting.
```suggestion
// Handle limit and offset
if let Some(limit) = req.limit {
let limit = u32::try_from(limit).map_err(|_| SysDbError::InvalidArgument("Limit must be non-negative".to_string()))?;
filter = filter.limit(limit);
}
if let Some(offset) = req.offset {
let offset = u32::try_from(offset).map_err(|_| SysDbError::InvalidArgument("Offset must be non-negative".to_string()))?;
filter = filter.offset(offset);
}
```
File: rust/rust-sysdb/src/types.rs
Line: 315| SELECT c.collection_id | ||
| FROM collections c | ||
| WHERE {where_clause} | ||
| ORDER BY c.created_at ASC |
There was a problem hiding this comment.
[Logic] Ordering by created_at alone is non-deterministic for collections created at the same timestamp. This can lead to unstable pagination (skipping or duplicating items across pages). Add collection_id as a tie-breaker to ensure a consistent sort order.
| ORDER BY c.created_at ASC | |
| ORDER BY c.created_at ASC, c.collection_id ASC |
Additional affected locations:
- rust/rust-sysdb/src/spanner.rs:713
Context for Agents
Ordering by `created_at` alone is non-deterministic for collections created at the same timestamp. This can lead to unstable pagination (skipping or duplicating items across pages). Add `collection_id` as a tie-breaker to ensure a consistent sort order.
```suggestion
ORDER BY c.created_at ASC, c.collection_id ASC
```
**Additional affected locations:**
- rust/rust-sysdb/src/spanner.rs:713
File: rust/rust-sysdb/src/spanner.rs
Line: 684
sanketkedia
force-pushed
the
01-07-_enh_create_collection_impl_in_rust_sysdb
branch
from
0197b36 to
ecca61f
Compare
sanketkedia
force-pushed
the
01-10-_enh_get_collections_impl
branch
from
75c2da6 to
578c9ec
Compare
Description of changes
Summarize the changes made by this PR.
Test plan
How are these changes tested?
Added test
pytestfor python,yarn testfor js,cargo testfor rustMigration plan
None
Observability plan
None
Documentation Changes
None