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.

Sign up for GitHub

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

Jump to bottom

fix!: limit will now decrease when subquery has no elements #277

Merged
merged 2 commits into from
Feb 22, 2024
Merged

fix!: limit will now decrease when subquery has no elements #277

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
67896ea
fix: limit will now decrease when subquery has no elements
QuantumExplorer Oct 29, 2023
b40dd1a
Merge branch 'develop' into fix/NoLimitDecreaseForSubqueries
shumkov Nov 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 79 additions & 28 deletions grovedb/src/element/query.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,20 @@
#[cfg(any(feature = "full", feature = "verify"))]
use crate::{Element, SizedQuery};

#[cfg(feature = "full")]
#[derive(Copy, Clone, Debug)]
pub struct QueryOptions {
pub allow_get_raw: bool,
pub allow_cache: bool,
/// Should we decrease the limit of elements found when we have no
/// subelements in the subquery? This should generally be set to true,
/// as having it false could mean very expensive queries. The queries
/// would be expensive because we could go through many many trees where the
/// sub elements have no matches, hence the limit would not decrease and
/// hence we would continue on the increasingly expensive query.
pub decrease_limit_on_range_with_no_sub_elements: bool,
}

#[cfg(feature = "full")]
/// Path query push arguments
pub struct PathQueryPushArgs<'db, 'ctx, 'a>
Expand All @@ -73,8 +87,7 @@
pub subquery_path: Option<Path>,
pub subquery: Option<Query>,
pub left_to_right: bool,
pub allow_get_raw: bool,
pub allow_cache: bool,
pub query_options: QueryOptions,
pub result_type: QueryResultType,
pub results: &'a mut Vec<QueryResultElement>,
pub limit: &'a mut Option<u16>,
Expand All @@ -97,6 +110,7 @@
merk_path,
&sized_query,
true,
true,
result_type,
transaction,
)
Expand Down Expand Up @@ -139,8 +153,7 @@
storage: &RocksDbStorage,
path: &[&[u8]],
sized_query: &SizedQuery,
allow_get_raw: bool,
allow_cache: bool,
query_options: QueryOptions,
result_type: QueryResultType,
transaction: TransactionArg,
add_element_function: fn(PathQueryPushArgs) -> CostResult<(), Error>,
Expand All @@ -166,8 +179,7 @@
transaction,
&mut limit,
&mut offset,
allow_get_raw,
allow_cache,
query_options,
result_type,
add_element_function,
)
Expand All @@ -189,8 +201,7 @@
transaction,
&mut limit,
&mut offset,
allow_get_raw,
allow_cache,
query_options,
result_type,
add_element_function,
)
Expand All @@ -216,6 +227,7 @@
storage: &RocksDbStorage,
path_query: &PathQuery,
allow_cache: bool,
decrease_limit_on_range_with_no_sub_elements: bool,
result_type: QueryResultType,
transaction: TransactionArg,
) -> CostResult<(QueryResultElements, u16), Error> {
Expand All @@ -228,8 +240,11 @@
storage,
path_slices.as_slice(),
&path_query.query,
false,
allow_cache,
QueryOptions {
allow_get_raw: false,
allow_cache,
decrease_limit_on_range_with_no_sub_elements,
},
result_type,
transaction,
Element::path_query_push,
Expand All @@ -243,6 +258,7 @@
storage: &RocksDbStorage,
path_query: &PathQuery,
allow_cache: bool,
decrease_limit_on_range_with_no_sub_elements: bool,
result_type: QueryResultType,
transaction: TransactionArg,
) -> CostResult<(QueryResultElements, u16), Error> {
Expand All @@ -255,8 +271,11 @@
storage,
path_slices.as_slice(),
&path_query.query,
true,
allow_cache,
QueryOptions {
allow_get_raw: true,
allow_cache,
decrease_limit_on_range_with_no_sub_elements,
},
result_type,
transaction,
Element::path_query_push,
Expand All @@ -270,15 +289,19 @@
path: &[&[u8]],
sized_query: &SizedQuery,
allow_cache: bool,
decrease_limit_on_range_with_no_sub_elements: bool,
result_type: QueryResultType,
transaction: TransactionArg,
) -> CostResult<(QueryResultElements, u16), Error> {
Element::get_query_apply_function(
storage,
path,
sized_query,
false,
allow_cache,
QueryOptions {
allow_get_raw: false,
allow_cache,
decrease_limit_on_range_with_no_sub_elements,
},
result_type,
transaction,
Element::path_query_push,
Expand All @@ -299,13 +322,17 @@
subquery_path,
subquery,
left_to_right,
allow_get_raw,
allow_cache,
query_options,
result_type,
results,
limit,
offset,
} = args;
let QueryOptions {
allow_get_raw,
allow_cache,
decrease_limit_on_range_with_no_sub_elements,
} = query_options;
if element.is_tree() {
let mut path_vec = path.to_vec();
let key = cost_return_on_error_no_add!(
Expand All @@ -331,13 +358,19 @@
storage,
&inner_path_query,
allow_cache,
decrease_limit_on_range_with_no_sub_elements,
result_type,
transaction
)
);

if let Some(limit) = limit {
*limit = limit.saturating_sub(sub_elements.len() as u16);
if sub_elements.is_empty() && decrease_limit_on_range_with_no_sub_elements {
// we should decrease by 1 in this case
*limit = limit.saturating_sub(1);
} else {
*limit = limit.saturating_sub(sub_elements.len() as u16);
}
}
if let Some(offset) = offset {
*offset = offset.saturating_sub(skipped);
Expand Down Expand Up @@ -455,8 +488,7 @@
subquery_path,
subquery,
left_to_right,
allow_get_raw,
allow_cache,
query_options,
result_type,
results,
limit,
Expand All @@ -483,8 +515,7 @@
subquery_path,
subquery,
left_to_right,
allow_get_raw,
allow_cache,
query_options,
result_type,
results,
limit,
Expand Down Expand Up @@ -530,22 +561,27 @@
(subquery_path, subquery)
}

/// `decrease_limit_on_range_with_no_sub_elements` should generally be set
/// to true, as having it false could mean very expensive queries.
/// The queries would be expensive because we could go through many many
/// trees where the sub elements have no matches, hence the limit would
/// not decrease and hence we would continue on the increasingly
/// expensive query.
#[cfg(feature = "full")]
// TODO: refactor
fn query_item(
storage: &RocksDbStorage,
item: &QueryItem,
results: &mut Vec<QueryResultElement>,
path: &[&[u8]],
sized_query: &SizedQuery,
transaction: TransactionArg,
limit: &mut Option<u16>,
offset: &mut Option<u16>,
allow_get_raw: bool,
allow_cache: bool,
query_options: QueryOptions,
result_type: QueryResultType,
add_element_function: fn(PathQueryPushArgs) -> CostResult<(), Error>,
) -> CostResult<(), Error> {

Check warning on line 584 in grovedb/src/element/query.rs

View workflow job for this annotation

GitHub Actions / clippy

this function has too many arguments (11/7) 

warning: this function has too many arguments (11/7)
   --> grovedb/src/element/query.rs:572:5
    |
572 | /     fn query_item(
573 | |         storage: &RocksDbStorage,
574 | |         item: &QueryItem,
575 | |         results: &mut Vec<QueryResultElement>,
...   |
583 | |         add_element_function: fn(PathQueryPushArgs) -> CostResult<(), Error>,
584 | |     ) -> CostResult<(), Error> {
    | |______________________________^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments
    = note: `#[warn(clippy::too_many_arguments)]` on by default
let mut cost = OperationCost::default();

let subtree_path: SubtreePath<_> = path.into();
Expand All @@ -560,7 +596,10 @@
None,
transaction,
subtree,
{ Element::get(&subtree, key, allow_cache).unwrap_add_cost(&mut cost) }
{
Element::get(&subtree, key, query_options.allow_cache)
.unwrap_add_cost(&mut cost)
}
);
match element_res {
Ok(element) => {
Expand All @@ -575,8 +614,7 @@
subquery_path,
subquery,
left_to_right: sized_query.query.left_to_right,
allow_get_raw,
allow_cache,
query_options,
result_type,
results,
limit,
Expand Down Expand Up @@ -630,8 +668,7 @@
subquery_path,
subquery,
left_to_right: sized_query.query.left_to_right,
allow_get_raw,
allow_cache,
query_options,
result_type,
results,
limit,
Expand Down Expand Up @@ -939,6 +976,7 @@
&[TEST_LEAF],
&ascending_query,
true,
true,
QueryKeyElementPairResultType,
None,
)
Expand Down Expand Up @@ -973,6 +1011,7 @@
&[TEST_LEAF],
&backwards_query,
true,
true,
QueryKeyElementPairResultType,
None,
)
Expand Down Expand Up @@ -1062,6 +1101,7 @@
&[TEST_LEAF],
&ascending_query,
true,
true,
QueryKeyElementPairResultType,
None,
)
Expand All @@ -1079,6 +1119,7 @@
&[TEST_LEAF],
&backwards_query,
true,
true,
QueryKeyElementPairResultType,
None,
)
Expand All @@ -1099,6 +1140,7 @@
&[TEST_LEAF],
&backwards_query,
true,
true,
QueryKeyElementPairResultType,
None,
)
Expand Down Expand Up @@ -1161,6 +1203,7 @@
&[TEST_LEAF],
&backwards_query,
true,
true,
QueryKeyElementPairResultType,
None,
)
Expand All @@ -1187,6 +1230,7 @@
&[TEST_LEAF],
&backwards_query,
true,
true,
QueryKeyElementPairResultType,
None,
)
Expand All @@ -1208,6 +1252,7 @@
&[TEST_LEAF],
&limit_query,
true,
true,
QueryKeyElementPairResultType,
None,
)
Expand All @@ -1229,6 +1274,7 @@
&[TEST_LEAF],
&limit_query,
true,
true,
QueryKeyElementPairResultType,
None,
)
Expand All @@ -1249,6 +1295,7 @@
&[TEST_LEAF],
&limit_offset_query,
true,
true,
QueryKeyElementPairResultType,
None,
)
Expand All @@ -1274,6 +1321,7 @@
&[TEST_LEAF],
&limit_offset_backwards_query,
true,
true,
QueryKeyElementPairResultType,
None,
)
Expand All @@ -1298,6 +1346,7 @@
&[TEST_LEAF],
&limit_full_query,
true,
true,
QueryKeyElementPairResultType,
None,
)
Expand All @@ -1323,6 +1372,7 @@
&[TEST_LEAF],
&limit_offset_backwards_query,
true,
true,
QueryKeyElementPairResultType,
None,
)
Expand All @@ -1348,6 +1398,7 @@
&[TEST_LEAF],
&limit_backwards_query,
true,
true,
QueryKeyElementPairResultType,
None,
)
Expand Down
Loading
Loading