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

Add starting_at option to CryptoGetAccountRecordsQuery #144

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Add starting_at option to CryptoGetAccountRecordsQuery #144

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
da96b78
Add starting_at option to CryptoGetAccountRecordsQuery
Jan 24, 2022
a6eab26
Add timestamp.proto import
Jan 24, 2022
0dafd6f
Improve comment on starting_at restriction
Jan 25, 2022
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
20 changes: 20 additions & 0 deletions services/crypto_get_account_records.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ package proto;
option java_package = "com.hederahashgraph.api.proto.java";
option java_multiple_files = true;

import "timestamp.proto";
import "basic_types.proto";
import "transaction_record.proto";
import "query_header.proto";
Expand All @@ -43,6 +44,25 @@ message CryptoGetAccountRecordsQuery {
* The account ID for which the records should be retrieved
*/
AccountID accountID = 2;

/**
* If set, a restriction on the first payer record to be returned. The queried node will
* return this record (assuming one qualifies), and up to to the next 179 records, in
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the limit (180) configurable? In which case we shouldn't document 179 here, we should just say we will return all records up to some maximum amount (put it in better words, but that's the idea). Same with the 180 on line 54.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated comment with link to the Services property.

* chronological order of consensus time.
*
* Without a restriction, the node will return the most recent payer records (in
* chronological order, up to a maximum of 180 records).
*/
oneof starting_at {
/**
* The transaction id of the first record to return.
*/
TransactionID transaction_id = 3;
/**
* The earliest consensus time of the first record to return.
*/
Timestamp earliest_consensus_time = 4;
Comment on lines +61 to +68
Copy link
Member

Choose a reason for hiding this comment

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

Is there any difference in performance depending on which of these two is used? In both cases are we going to iterate through the records looking for the first that meets the criteria?

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, with current FCQ implementation, we need to iterate through all records no matter what. (If random access was supported, we could do binary search for earliest consensus time, but it isn't.)

Copy link
Member

Choose a reason for hiding this comment

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

Not sure we should add this feature until the data structure has a performant way to allow it...

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 current implementation already iterates through all records. 😬

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but we're adding a limit now so it will only iterate through the first K records where K is the limit and N is the total number of records. But with this feature they could pick the last transaction id so it will be O(N).

Copy link
Member

Choose a reason for hiding this comment

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

Oh, unless it has to iterate through all to filter down by account ID. I'm guessing it's not in a map by account id.

Copy link
Contributor Author

@tinker-michaelj tinker-michaelj Jan 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Right, these records are in an FCQueue which doesn't support random access (or even reverse traversal).

No matter how the user restricts (or doesn't restrict) the query, we must iterate through the entire FCQ.

}
}

/**
Expand Down