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

chore: enable correct_pagination_assets_after_creating_new_one test #5192

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

chore: enable correct_pagination_assets_after_creating_new_one test #5192

Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d3bb566
chore: enable correct_pagination_assets_after_creating_new_one test
aoyako Oct 24, 2024
64e1d1b
Merge branch 'main' into restore_pagination_test
aoyako Oct 24, 2024
8027386
Merge branch 'main' into restore_pagination_test
aoyako Oct 25, 2024
f7bd0c8
Merge branch 'main' into restore_pagination_test
aoyako Oct 28, 2024
c5d0af5
Merge branch 'main' into restore_pagination_test
aoyako Oct 29, 2024
0e59aec
Merge branch 'main' into restore_pagination_test
aoyako Oct 31, 2024
5d95541
Merge branch 'main' into restore_pagination_test
aoyako Nov 13, 2024
5aacf7e
Merge branch 'main' into restore_pagination_test
aoyako Dec 16, 2024
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
3 changes: 1 addition & 2 deletions crates/iroha/tests/sorting.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,10 @@ use nonzero_ext::nonzero;
use rand::{seq::SliceRandom, thread_rng};

#[test]
#[ignore]
#[allow(clippy::cast_possible_truncation)]
fn correct_pagination_assets_after_creating_new_one() {
// FIXME transaction is rejected for more than a certain number of instructions
const N_ASSETS: usize = 12;
const N_ASSETS: usize = 9;
Comment on lines 17 to +18
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this test has failed for some other reason than the FIXME item. I see the same result by iterating over submit_blocking and there are no transaction rejections due to too many instructions in submit_all_blocking. In my local 10 and 11 are the threshold of whether it passes or not

Copy link
Contributor

Choose a reason for hiding this comment

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

what's the default fetch size, 10? this might be related to result batching

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test fails due to the lexicographical sorting of strings ("1" -> "10" -> "11", not "1" -> "2" -> "3").
All of the sorting tests have this limit of 10 because they will also fail if the number is increased.
This test should verify that adding an asset in the middle should not break the ordering, which was not checked in the previous version.

Copy link
Contributor

Choose a reason for hiding this comment

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

So the issue would be that metadata leaf values are implicitly transmuted from numbers to strings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, since the metadata value is a json string. I'm not sure if we have a functionality to specify a mapping/key function for sorting. Isn't the purpose of having json string to generalize metadata and defer custom processing to the user?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since PartialOrd and Ord cannot be naturally derived from serde_json::Value, we could:

  • totally discard the concept of sorting metadata value
  • originally define order between Value variants

Copy link
Contributor

Choose a reason for hiding this comment

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

Since PartialOrd and Ord cannot be naturally derived from serde_json::Value, we could:

good point. My previous comment should be disregarded. Now I think this is the way to go:

totally discard the concept of sorting metadata value

Copy link
Contributor

Choose a reason for hiding this comment

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

If we go this way, the problem then is currently we are sorting query results only by metadata

Copy link
Contributor

@mversic mversic Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this is where it all started. But since then we moved to untyped metadata value. Maybe it would be good to provide a sorting function? The comparison function will get untyped metadata, parse it and then compare the objects

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, there seems to be a trade-off between keeping metadata values untyped and serving query sorting on the peer side

// 0 < pagination.start < missing_idx < pagination.end < N_ASSETS
let missing_indices = vec![N_ASSETS / 2];
let pagination = Pagination {
Expand Down
Loading