feat: count query and tests #64
Conversation
WalkthroughThe changes extend the system's functionality by adding support for count operations. A new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant REPL
participant Engine
participant Serializer
User->>REPL: Enter query with Count expression
REPL->>Engine: Forward Count query for processing
Engine->>Engine: Execute run_count_expr() to compute count result
Engine->>REPL: Return ExpressionResult::Count
REPL->>Serializer: Format count result via to_table()
REPL->>User: Display count result in cyan
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/core/src/common/types.rs (1)
16-19: Add documentation for the new struct.Please add documentation comments explaining the purpose and usage of
CountExpression. This will help other developers understand how to use this new feature.+/// Represents a count operation on a query result. +/// The count is performed on the results of the contained query. #[derive(Debug, PartialEq)] pub struct CountExpression { pub query: GetExpression, }crates/core/src/common/serializer.rs (1)
111-172: Add test coverage for count serialization.Please add test cases for serializing count results in all formats (JSON, CSV, and Parquet) to maintain test coverage.
Would you like me to help generate the test cases for count serialization?
crates/core/src/interpreter/backend/execution_engine.rs (1)
44-44: Remove debug print statement.The debug print statement should be removed before merging.
-println!("Query results: {:#?}", query_results);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
crates/cli/src/main.rs(1 hunks)crates/cli/src/repl.rs(1 hunks)crates/core/src/common/logs.rs(1 hunks)crates/core/src/common/query_result.rs(2 hunks)crates/core/src/common/serializer.rs(2 hunks)crates/core/src/common/types.rs(1 hunks)crates/core/src/interpreter/backend/execution_engine.rs(16 hunks)crates/core/src/interpreter/backend/mod.rs(1 hunks)crates/macros/src/lib.rs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- crates/macros/src/lib.rs
- crates/core/src/interpreter/backend/mod.rs
🔇 Additional comments (8)
crates/core/src/common/types.rs (1)
13-14: LGTM! New variant added to support count operations.The
Countvariant is a clean addition to theExpressionenum.crates/cli/src/main.rs (1)
61-63: LGTM! Consistent handling of count results.The implementation follows the same pattern as other result types, maintaining consistency in the codebase.
crates/core/src/common/serializer.rs (2)
28-29: LGTM! CSV serialization for count results.The implementation correctly handles count results using the existing
serialize_csvfunction.
63-64: LGTM! Parquet serialization for count results.The implementation correctly handles count results using the existing
create_parquet_schema_and_datafunction.crates/core/src/common/logs.rs (1)
144-147: Review address checksum handling.The current implementation passes
Nonetwice:
- First to
Address::to_checksum- Then to
Address::parse_checksummedThis might not be the intended behavior. Please verify if this is correct or if one of the
Noneparameters should be different.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other usages of these methods to verify correct usage rg -A 2 "to_checksum|parse_checksummed" --type rustLength of output: 369
Checksum Handling Confirmation: Based on a repository-wide search, the only instance found for calling
Address::to_checksumandAddress::parse_checksummedis the one incrates/core/src/common/logs.rs, and both functions are consistently invoked withNonefor their optional parameters. There’s no evidence elsewhere in the codebase suggesting that a non-Nonevalue should be provided. Please verify against the function definitions to ensure that these default behaviors are indeed as intended.crates/cli/src/repl.rs (1)
244-249: LGTM!The implementation for displaying count results follows the same pattern as other result types and maintains color consistency with block and log results.
crates/core/src/common/query_result.rs (1)
28-30: LGTM!The implementation of
Countvariant andCountQueryResstruct follows the same pattern as other result types, with proper serde attributes and default implementation.Also applies to: 339-351
crates/core/src/interpreter/backend/execution_engine.rs (1)
108-175: LGTM!The test cases for counting logs and block fields are well-structured and verify the count functionality correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/core/src/interpreter/backend/execution_engine.rs (3)
44-44: Remove debug print statement.Debug print statements should not be committed to production code.
- println!("Query results: {:#?}", query_results);
72-85: Simplify count implementation usingmatchexpression.The count implementation can be simplified by directly returning the count result from the match expression.
async fn run_count_expr(&self, expr: &CountExpression) -> Result<ExpressionResult> { let query_result = self.run_get_expr(&expr.query).await?; - let count = match query_result { + Ok(ExpressionResult::Count(vec![CountQueryRes { count: match query_result { ExpressionResult::Block(data) => data.len(), ExpressionResult::Account(data) => data.len(), ExpressionResult::Transaction(data) => data.len(), ExpressionResult::Log(data) => data.len(), ExpressionResult::Count(_) => { return Err(anyhow::anyhow!("Unexpected Count result from run_get_expr")) } - }; - let result = ExpressionResult::Count(vec![CountQueryRes{ count }]); - Ok(result) + }}])) }
225-225: Remove debug print statement from test.Debug print statements in tests can clutter test output.
- println!("Results: {:#?}", execution_result);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/core/src/interpreter/backend/execution_engine.rs(16 hunks)
🔇 Additional comments (1)
crates/core/src/interpreter/backend/execution_engine.rs (1)
110-145: Enhance test coverage for count functionality.The current tests only verify the count=1 case. Consider adding test cases for:
- Empty results (count=0)
- Multiple results (count>1)
- Error cases (e.g., invalid queries)
Also applies to: 148-177
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/core/src/common/types.rs (1)
27-52: Consider implementing the commented-out error variants.The commented-out error variants in
CountExpressionErrorseem useful for handling various failure scenarios such as missing entities, chain/RPC issues, and URL parsing errors. These would provide more specific error handling and better error messages.Apply this diff to implement the error variants:
#[derive(thiserror::Error, Debug)] pub enum CountExpressionError { #[error("Unexpected token: {0}")] UnexpectedToken(String), -// #[error("Missing entity")] -// MissingEntity, -// #[error("Missing chain or RPC")] -// MissingChainOrRpc, -// #[error("URL parse error: {0}")] -// UrlParseError(String), -// #[error(transparent)] -// EntityError(#[from] EntityError), -// #[error(transparent)] -// ChainError(#[from] ChainError), -// #[error(transparent)] -// DumpError(#[from] DumpError), + #[error("Missing entity")] + MissingEntity, + #[error("Missing chain or RPC")] + MissingChainOrRpc, + #[error("URL parse error: {0}")] + UrlParseError(String), + #[error(transparent)] + EntityError(#[from] EntityError), + #[error(transparent)] + ChainError(#[from] ChainError), + #[error(transparent)] + DumpError(#[from] DumpError), }crates/core/src/interpreter/frontend/productions.pest (1)
3-11: Add documentation for the count rule.Consider adding a comment explaining the syntax and usage of the count rule for better maintainability.
Add this documentation above the count rule:
+// The count rule defines the syntax for counting entities: +// COUNT(<query>) where <query> is a valid get expression +// Example: COUNT(GET * FROM block 1:100 ON eth) count = { "COUNT" ~ WHITESPACE* ~ "(" ~ WHITESPACE* ~ query ~ WHITESPACE* ~ ")" }crates/core/src/interpreter/backend/execution_engine.rs (2)
109-144: Add more test cases for log counting.Consider adding test cases for:
- Empty result (no matching logs)
- Multiple matching logs
- Invalid filters
147-176: Add more test cases for block counting.Consider adding test cases for:
- Multiple blocks in range
- Block tag (latest, earliest)
- Invalid block range
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/core/src/common/types.rs(1 hunks)crates/core/src/interpreter/backend/execution_engine.rs(15 hunks)crates/core/src/interpreter/frontend/parser.rs(1 hunks)crates/core/src/interpreter/frontend/productions.pest(2 hunks)
🔇 Additional comments (7)
crates/core/src/common/types.rs (3)
11-14: LGTM! The Expression enum is correctly extended.The addition of the
Countvariant to handle count expressions aligns well with the existing structure.
16-25: LGTM! Well-structured CountExpression implementation.The struct design is clean and reuses the existing
GetExpressiontype, promoting code reuse.
54-71: LGTM! Robust TryFrom implementation.The implementation properly validates the input and handles error cases while converting from parser pairs to CountExpression.
crates/core/src/interpreter/frontend/productions.pest (2)
1-1: LGTM! Program rule correctly extended.The program rule now properly supports both
getandcountexpressions.
13-13: LGTM! Query rule is correctly defined.The query rule appropriately references the get rule, maintaining consistency with the
CountExpressionimplementation.crates/core/src/interpreter/backend/execution_engine.rs (2)
28-46: LGTM! Run method properly handles Count expressions.The implementation correctly processes both Get and Count expressions, maintaining consistency in result handling.
71-84: LGTM! Improved error handling in run_count_expr.The implementation now properly handles all cases and includes explicit error handling for unexpected Count results.
| Rule::count => { | ||
| expressions.push(Expression::Count(pair.into_inner().try_into()?)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add tests for count expression parsing.
The implementation looks good, but test coverage is needed for the count expression parsing. Consider adding tests for:
- Basic count syntax
- Invalid count syntax (missing parentheses, invalid query)
- Multiple count expressions
Example test to add:
#[test]
fn test_build_ast_with_count_expression() {
let source = "COUNT(GET * FROM block 1:100 ON eth)";
let expected = vec![Expression::Count(CountExpression {
query: GetExpression {
entity: Entity::Block(Block::new(
Some(vec![BlockId::Range(BlockRange::new(
BlockNumberOrTag::Number(1),
Some(BlockNumberOrTag::Number(100)),
))]),
None,
BlockField::all_variants().to_vec(),
)),
chains: vec![ChainOrRpc::Chain(Chain::Ethereum)],
dump: None,
}
})];
match Parser::new(source).parse_expressions() {
Ok(result) => assert_eq!(result, expected),
Err(e) => panic!("Error: {}", e),
}
}1 participant
Summary
The
CountExpressionis designed to count the number of entities returned by a given query. It utilizesrun_get_exprto fetch the query result and then determines the count based on the number of items in the result.Related Issue
#62
Technical details
Uses
run_get_exprto fetch results.Matches result types explicitly.
Converts
.len()to aCountresult.Prevents nested count operations.
Topics for Discussion
PR Checklist
Screenshots (if applicable)
Summary by CodeRabbit
Summary by CodeRabbit
countcommand.