From d95965e4564a6defd841fe167ec8c78b07410642 Mon Sep 17 00:00:00 2001 From: Christopher Hotchkiss Date: Sat, 4 Sep 2021 00:09:08 -0400 Subject: [PATCH] Mass clippy fixes now that I know how to run clippy on save --- benches/feophant_benchmark.rs | 14 ++++-- src/constants/system_tables/pg_attribute.rs | 2 +- src/constants/system_tables/pg_class.rs | 2 +- src/constants/system_tables/pg_constraint.rs | 2 +- src/constants/system_tables/pg_index.rs | 2 +- src/engine/analyzer.rs | 2 +- src/engine/analyzer/definition_lookup.rs | 20 ++++---- src/engine/io/constraint_manager.rs | 1 - src/engine/io/file_manager.rs | 2 - src/engine/io/file_manager/file_executor.rs | 9 ++-- src/engine/io/free_space_manager.rs | 49 ++++++++------------ src/engine/io/lock_cache_manager.rs | 11 ++--- src/engine/io/page_formats/page_data.rs | 7 +-- src/engine/io/page_formats/page_header.rs | 6 +++ src/engine/io/page_formats/page_offset.rs | 24 +++------- src/engine/io/row_formats/row_data.rs | 2 +- src/engine/io/row_manager.rs | 19 +++++--- src/engine/io/visible_row_manager.rs | 1 - src/processor/ssl_and_gssapi_parser.rs | 9 ++-- src/processor/startup_parser.rs | 6 +-- tests/common/mod.rs | 5 -- tests/sql_primary_key_insert.rs | 4 +- tests/sql_simple_select.rs | 2 +- 23 files changed, 85 insertions(+), 116 deletions(-) diff --git a/benches/feophant_benchmark.rs b/benches/feophant_benchmark.rs index f44ffdf..f75f36d 100644 --- a/benches/feophant_benchmark.rs +++ b/benches/feophant_benchmark.rs @@ -1,7 +1,6 @@ use criterion::BenchmarkId; use criterion::Criterion; use criterion::{criterion_group, criterion_main}; -use feophantlib::constants::Nullable; use feophantlib::engine::get_row; use feophantlib::engine::get_table; use feophantlib::engine::io::row_formats::RowData; @@ -47,10 +46,15 @@ async fn row_manager_mass_insert(row_count: usize) -> Result<(), Box Vec { ] } -pub fn get_index(attrs: &Vec) -> Arc { +pub fn get_index(attrs: &[Attribute]) -> Arc { Arc::new(Index { id: Uuid::from_bytes(hex!("516B20412CF145A2AD9E39A8BDEB30A8")), name: NAME.to_string() + "_name_index", diff --git a/src/constants/system_tables/pg_class.rs b/src/constants/system_tables/pg_class.rs index c7cd0c2..e26c882 100644 --- a/src/constants/system_tables/pg_class.rs +++ b/src/constants/system_tables/pg_class.rs @@ -30,7 +30,7 @@ pub fn get_columns() -> Vec { ] } -pub fn get_index(attrs: &Vec) -> Arc { +pub fn get_index(attrs: &[Attribute]) -> Arc { Arc::new(Index { id: Uuid::from_bytes(hex!("516B20412CF145A2AD9E39A8BDEB30A8")), name: NAME.to_string() + "_name_index", diff --git a/src/constants/system_tables/pg_constraint.rs b/src/constants/system_tables/pg_constraint.rs index b651b0a..2b5af9f 100644 --- a/src/constants/system_tables/pg_constraint.rs +++ b/src/constants/system_tables/pg_constraint.rs @@ -51,7 +51,7 @@ pub fn get_columns() -> Vec { ] } -pub fn get_index(attrs: &Vec) -> Arc { +pub fn get_index(attrs: &[Attribute]) -> Arc { Arc::new(Index { id: Uuid::from_bytes(hex!("27182DE783AB42D8B5DD43BFC0154F0F")), name: NAME.to_string() + "_name_index", diff --git a/src/constants/system_tables/pg_index.rs b/src/constants/system_tables/pg_index.rs index 0473f50..c6420a6 100644 --- a/src/constants/system_tables/pg_index.rs +++ b/src/constants/system_tables/pg_index.rs @@ -51,7 +51,7 @@ pub fn get_columns() -> Vec { ] } -pub fn get_index(attrs: &Vec) -> Arc { +pub fn get_index(attrs: &[Attribute]) -> Arc { Arc::new(Index { id: Uuid::from_bytes(hex!("5F59466782874C568F1C0C09E99C9249")), name: NAME.to_string() + "_name_index", diff --git a/src/engine/analyzer.rs b/src/engine/analyzer.rs index d8ac0cd..b10b25d 100644 --- a/src/engine/analyzer.rs +++ b/src/engine/analyzer.rs @@ -69,7 +69,7 @@ impl Analyzer { //Insert columns will be the target targets: Arc::new(output_type), range_tables: vec![target_tbl.clone(), anon_tbl.clone()], - joins: vec![((JoinType::Inner, target_tbl, anon_tbl))], + joins: vec![(JoinType::Inner, target_tbl, anon_tbl)], }) } diff --git a/src/engine/analyzer/definition_lookup.rs b/src/engine/analyzer/definition_lookup.rs index 3139da4..7d1992a 100644 --- a/src/engine/analyzer/definition_lookup.rs +++ b/src/engine/analyzer/definition_lookup.rs @@ -99,7 +99,7 @@ impl DefinitionLookup { let row_stream = self .vis_row_man .clone() - .get_stream(tran_id, SystemTables::PgClass.value().clone()); + .get_stream(tran_id, SystemTables::PgClass.value()); pin!(row_stream); while let Some(row_res) = row_stream.next().await { let row = row_res?; @@ -175,13 +175,13 @@ impl DefinitionLookup { &self, tran_id: TransactionId, class_id: Uuid, - attributes: &Vec, + attributes: &[Attribute], ) -> Result>, DefinitionLookupError> { let mut rows = vec![]; let row_stream = self .vis_row_man .clone() - .get_stream(tran_id, SystemTables::PgIndex.value().clone()); + .get_stream(tran_id, SystemTables::PgIndex.value()); pin!(row_stream); while let Some(row_res) = row_stream.next().await { let row = row_res?; @@ -214,9 +214,7 @@ impl DefinitionLookup { cols.push( attributes .get(i_usize) - .ok_or_else(|| { - DefinitionLookupError::WrongColumnIndex(i_usize) - })? + .ok_or(DefinitionLookupError::WrongColumnIndex(i_usize))? .clone(), ); } @@ -249,13 +247,13 @@ impl DefinitionLookup { &self, tran_id: TransactionId, class_id: Uuid, - indexes: &Vec>, + indexes: &[Arc], ) -> Result, DefinitionLookupError> { let mut rows = vec![]; let row_stream = self .vis_row_man .clone() - .get_stream(tran_id, SystemTables::PgConstraint.value().clone()); + .get_stream(tran_id, SystemTables::PgConstraint.value()); pin!(row_stream); while let Some(row_res) = row_stream.next().await { let row = row_res?; @@ -368,9 +366,8 @@ mod tests { .get_definition(tran_id, "something_random".to_string()) .await; match pg_class_def { - Ok(_) => assert!(false), - Err(DefinitionLookupError::TableDoesNotExist(_)) => assert!(true), - _ => assert!(false), + Err(DefinitionLookupError::TableDoesNotExist(_)) => {} + _ => panic!("Should not have gotten here!"), } Ok(()) } @@ -397,7 +394,6 @@ mod tests { dl.get_definition(tran, "foo".to_string()).await?; tm.commit_trans(tran).await?; - assert!(true); Ok(()) } } diff --git a/src/engine/io/constraint_manager.rs b/src/engine/io/constraint_manager.rs index 45483f2..348d7b6 100644 --- a/src/engine/io/constraint_manager.rs +++ b/src/engine/io/constraint_manager.rs @@ -15,7 +15,6 @@ use crate::{ }; use super::{ - page_formats::PageData, row_formats::{ItemPointer, RowData}, VisibleRowManager, VisibleRowManagerError, }; diff --git a/src/engine/io/file_manager.rs b/src/engine/io/file_manager.rs index 8ba84d7..bb93847 100644 --- a/src/engine/io/file_manager.rs +++ b/src/engine/io/file_manager.rs @@ -1,9 +1,7 @@ //! This is a different approach than I had done before. This file manager runs its own loop based on a spawned task //! since the prior approach was too lock heavy and I couldn't figure out an approach that didn't starve resources. use super::page_formats::{PageId, PageOffset, UInt12, UInt12Error}; -use async_stream::try_stream; use bytes::{Bytes, BytesMut}; -use futures::Stream; use std::convert::TryFrom; use std::ffi::OsString; use std::num::TryFromIntError; diff --git a/src/engine/io/file_manager/file_executor.rs b/src/engine/io/file_manager/file_executor.rs index cf81e99..9f76fc3 100644 --- a/src/engine/io/file_manager/file_executor.rs +++ b/src/engine/io/file_manager/file_executor.rs @@ -6,7 +6,6 @@ use crate::constants::PAGE_SIZE; use crate::engine::io::file_manager::ResourceFormatter; use crate::engine::io::page_formats::{PageId, PageOffset}; use bytes::{Bytes, BytesMut}; -use futures::SinkExt; use lru::LruCache; use std::collections::{HashMap, VecDeque}; use std::convert::TryFrom; @@ -628,9 +627,9 @@ mod tests { let mut test_file = FileOperations::open_path(tmp_dir, &page_id, PageOffset(0).get_file_number()).await?; - let mut test_page = get_test_page(1); + let test_page = get_test_page(1); - test_file.write_all(&mut test_page).await?; + test_file.write_all(&test_page).await?; drop(test_file); //Now let's test add @@ -672,9 +671,9 @@ mod tests { ) .await?; - let mut test_page = get_test_page(1); + let test_page = get_test_page(1); - test_file.write_all(&mut test_page).await?; + test_file.write_all(&test_page).await?; drop(test_file); //Now let's test add diff --git a/src/engine/io/free_space_manager.rs b/src/engine/io/free_space_manager.rs index 50de9f0..5d75fbc 100644 --- a/src/engine/io/free_space_manager.rs +++ b/src/engine/io/free_space_manager.rs @@ -116,22 +116,6 @@ impl FreeSpaceManager { None } - /// Gets the status of a field inside a page, you MUST pass an offset - /// that fits in the buffer. - //TODO Decide if I end up keeping this or maybe move it to the unit test section? - fn get_status_inside_page(buffer: &BytesMut, offset: usize) -> FreeStat { - let offset_index = offset / 8; - let offset_subindex = offset % 8; - - let offset_value = buffer[offset_index]; - let bit_value = (offset_value >> offset_subindex) & 0x1; - if bit_value == 0 { - FreeStat::Free - } else { - FreeStat::InUse - } - } - /// Sets the status of a field inside a page, you MUST pass an offset /// that fits in the buffer. fn set_status_inside_page(buffer: &mut BytesMut, offset: usize, status: FreeStat) { @@ -181,6 +165,22 @@ mod tests { use super::*; + /// Gets the status of a field inside a page, you MUST pass an offset + /// that fits in the buffer. + //This was in the implementation, I just only needed it for unit tests + fn get_status_inside_page(buffer: &BytesMut, offset: usize) -> FreeStat { + let offset_index = offset / 8; + let offset_subindex = offset % 8; + + let offset_value = buffer[offset_index]; + let bit_value = (offset_value >> offset_subindex) & 0x1; + if bit_value == 0 { + FreeStat::Free + } else { + FreeStat::InUse + } + } + ///This test works by toggling each bit repeatedly and making sure it gives the correct result each time. #[test] fn test_get_and_set() -> Result<(), Box> { @@ -188,20 +188,11 @@ mod tests { test.put_u16(0x0); for i in 0..test.capacity() * 8 { - assert_eq!( - FreeSpaceManager::get_status_inside_page(&test, i), - FreeStat::Free - ); + assert_eq!(get_status_inside_page(&test, i), FreeStat::Free); FreeSpaceManager::set_status_inside_page(&mut test, i, FreeStat::InUse); - assert_eq!( - FreeSpaceManager::get_status_inside_page(&test, i), - FreeStat::InUse - ); + assert_eq!(get_status_inside_page(&test, i), FreeStat::InUse); FreeSpaceManager::set_status_inside_page(&mut test, i, FreeStat::Free); - assert_eq!( - FreeSpaceManager::get_status_inside_page(&test, i), - FreeStat::Free - ); + assert_eq!(get_status_inside_page(&test, i), FreeStat::Free); } Ok(()) @@ -220,7 +211,7 @@ mod tests { FreeSpaceManager::set_status_inside_page(&mut test, i, FreeStat::InUse); } assert_eq!( - FreeSpaceManager::find_first_free_page_in_page(&mut test.clone()), + FreeSpaceManager::find_first_free_page_in_page(&mut test), None ); diff --git a/src/engine/io/lock_cache_manager.rs b/src/engine/io/lock_cache_manager.rs index b2f45e8..b1d97af 100644 --- a/src/engine/io/lock_cache_manager.rs +++ b/src/engine/io/lock_cache_manager.rs @@ -61,7 +61,7 @@ impl LockCacheManager { ) -> Result>>, LockCacheManagerError> { let mut cache = self.cache.lock().await; match cache.get(&(page_id, offset)) { - Some(s) => return Ok(s.clone()), + Some(s) => Ok(s.clone()), None => { //Cache miss, let's make the RwLock and drop the mutex let page_lock = Arc::new(RwLock::new(None)); @@ -70,12 +70,9 @@ impl LockCacheManager { drop(cache); //Now we can load the underlying page without blocking everyone - match self.file_manager.get_page(&page_id, &offset).await? { - Some(s) => { - page_lock_write.replace(s); - } - None => {} - }; + if let Some(s) = self.file_manager.get_page(&page_id, &offset).await? { + page_lock_write.replace(s); + } drop(page_lock_write); Ok(page_lock) diff --git a/src/engine/io/page_formats/page_data.rs b/src/engine/io/page_formats/page_data.rs index 0be91f6..c660311 100644 --- a/src/engine/io/page_formats/page_data.rs +++ b/src/engine/io/page_formats/page_data.rs @@ -163,14 +163,11 @@ pub enum PageDataError { #[cfg(test)] mod tests { - use crate::constants::{Nullable, PAGE_SIZE}; + use crate::constants::PAGE_SIZE; use crate::engine::get_table; use crate::engine::objects::SqlTuple; - use super::super::super::super::objects::{ - types::{BaseSqlTypes, BaseSqlTypesMapper}, - Attribute, Table, - }; + use super::super::super::super::objects::types::BaseSqlTypes; use super::super::super::super::transactions::TransactionId; use super::*; use bytes::BytesMut; diff --git a/src/engine/io/page_formats/page_header.rs b/src/engine/io/page_formats/page_header.rs index 693a1a4..15de859 100644 --- a/src/engine/io/page_formats/page_header.rs +++ b/src/engine/io/page_formats/page_header.rs @@ -69,6 +69,12 @@ impl PageHeader { } } +impl Default for PageHeader { + fn default() -> Self { + Self::new() + } +} + impl ConstEncodedSize for PageHeader { fn encoded_size() -> usize { 3 diff --git a/src/engine/io/page_formats/page_offset.rs b/src/engine/io/page_formats/page_offset.rs index 11196f0..ffe6efb 100644 --- a/src/engine/io/page_formats/page_offset.rs +++ b/src/engine/io/page_formats/page_offset.rs @@ -203,27 +203,15 @@ mod tests { #[test] fn test_is_same_file() -> Result<(), Box> { - assert_eq!( - PageOffset(0).is_same_file(&PageOffset(PAGES_PER_FILE)), - false - ); - assert_eq!(PageOffset(0).is_same_file(&PageOffset(0)), true); - assert_eq!( - PageOffset(0).is_same_file(&PageOffset(PAGES_PER_FILE - 1)), - true - ); - - assert_eq!( - PageOffset(PAGES_PER_FILE).is_same_file(&PageOffset(0)), - false - ); - assert_eq!( - PageOffset(PAGES_PER_FILE - 1).is_same_file(&PageOffset(0)), - true - ); + assert!(!PageOffset(0).is_same_file(&PageOffset(PAGES_PER_FILE))); + assert!(PageOffset(0).is_same_file(&PageOffset(0))); + assert!(PageOffset(0).is_same_file(&PageOffset(PAGES_PER_FILE - 1))); + assert!(!PageOffset(PAGES_PER_FILE).is_same_file(&PageOffset(0))); + assert!(PageOffset(PAGES_PER_FILE - 1).is_same_file(&PageOffset(0))); Ok(()) } + #[test] fn test_increment_and_hash_map() -> Result<(), Box> { let test = PageOffset(0); diff --git a/src/engine/io/row_formats/row_data.rs b/src/engine/io/row_formats/row_data.rs index 45b78ae..bc9d751 100644 --- a/src/engine/io/row_formats/row_data.rs +++ b/src/engine/io/row_formats/row_data.rs @@ -468,7 +468,7 @@ mod tests { let mut buffer = buffer.freeze(); let test_parse = RowData::parse(table, &mut buffer)?; - assert_eq!(test, test_parse.clone()); + assert_eq!(test, test_parse); let column_val = test_parse.get_column_not_null("header")?; assert_eq!(column_val, BaseSqlTypes::Text("this is a test".to_string())); diff --git a/src/engine/io/row_manager.rs b/src/engine/io/row_manager.rs index c391ad1..f4a3b57 100644 --- a/src/engine/io/row_manager.rs +++ b/src/engine/io/row_manager.rs @@ -61,7 +61,7 @@ impl RowManager { .as_mut() .ok_or(RowManagerError::NonExistentPage(row_pointer.page))?; - let mut page = PageData::parse(table, row_pointer.page, &page_buffer)?; + let mut page = PageData::parse(table, row_pointer.page, page_buffer)?; let mut row = page .get_row(row_pointer.count) .ok_or(RowManagerError::NonExistentRow( @@ -111,7 +111,7 @@ impl RowManager { .as_mut() .ok_or(RowManagerError::NonExistentPage(row_pointer.page))?; - let mut old_page = PageData::parse(table.clone(), row_pointer.page, &old_page_buffer)?; + let mut old_page = PageData::parse(table.clone(), row_pointer.page, old_page_buffer)?; let mut old_row = old_page .get_row(row_pointer.count) @@ -172,7 +172,7 @@ impl RowManager { let page_bytes = page_handle .as_ref() .ok_or(RowManagerError::NonExistentPage(row_pointer.page))?; - let page = PageData::parse(table, row_pointer.page, &page_bytes)?; + let page = PageData::parse(table, row_pointer.page, page_bytes)?; let row = page .get_row(row_pointer.count) @@ -344,10 +344,15 @@ mod tests { .await; assert_eq!(result_rows.len(), 50); - for i in 0..50 { - let sample_row = get_row(i.to_string()); - assert_eq!(result_rows[i].user_data, sample_row); - } + result_rows + .iter() + .enumerate() + .take(50) + .map(|(i, row)| { + let sample_row = get_row(i.to_string()); + assert_eq!(row.user_data, sample_row); + }) + .for_each(drop); Ok(()) } diff --git a/src/engine/io/visible_row_manager.rs b/src/engine/io/visible_row_manager.rs index 516332c..54bf8ed 100644 --- a/src/engine/io/visible_row_manager.rs +++ b/src/engine/io/visible_row_manager.rs @@ -9,7 +9,6 @@ use super::super::transactions::{ TransactionId, TransactionManager, TransactionManagerError, TransactionStatus, }; use super::{ - page_formats::PageData, row_formats::{ItemPointer, RowData}, RowManager, RowManagerError, }; diff --git a/src/processor/ssl_and_gssapi_parser.rs b/src/processor/ssl_and_gssapi_parser.rs index 426c374..e763f18 100644 --- a/src/processor/ssl_and_gssapi_parser.rs +++ b/src/processor/ssl_and_gssapi_parser.rs @@ -25,19 +25,16 @@ mod tests { #[test] fn test_ssl_match() { - let check = is_ssl_request(&hex!("04 D2 16 2F")); - assert_eq!(check, true); + assert!(is_ssl_request(&hex!("04 D2 16 2F"))); } #[test] fn test_ssl_not_match() { - let check = is_ssl_request(&hex!("12 34 56")); - assert_eq!(check, false); + assert!(!is_ssl_request(&hex!("12 34 56"))); } #[test] fn test_gsspai_match() { - let check = is_gssapi_request(&hex!("04 D2 16 30")); - assert_eq!(check, true); + assert!(is_gssapi_request(&hex!("04 D2 16 30"))); } } diff --git a/src/processor/startup_parser.rs b/src/processor/startup_parser.rs index cd1ab9e..11588f6 100644 --- a/src/processor/startup_parser.rs +++ b/src/processor/startup_parser.rs @@ -65,13 +65,11 @@ mod tests { } #[test] + #[should_panic] fn test_invalid_utf8_till_null() { let test_string = b"\xc3\x28\0"; - match till_null(test_string) { - Ok(_) => assert!(false), - Err(_) => assert!(true), - } + till_null(test_string).unwrap(); } #[test] diff --git a/tests/common/mod.rs b/tests/common/mod.rs index a9a11dd..7d24fdb 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -1,8 +1,3 @@ -use std::sync::Arc; - -use feophantlib::constants::Nullable; -use feophantlib::engine::objects::types::{BaseSqlTypes, BaseSqlTypesMapper}; -use feophantlib::engine::objects::{Attribute, SqlTuple, Table}; use feophantlib::feophant::FeOphant; use log::LevelFilter; use simplelog::{ColorChoice, CombinedLogger, Config, TermLogger, TerminalMode}; diff --git a/tests/sql_primary_key_insert.rs b/tests/sql_primary_key_insert.rs index 3bb706f..cd79d31 100644 --- a/tests/sql_primary_key_insert.rs +++ b/tests/sql_primary_key_insert.rs @@ -1,4 +1,4 @@ -use tokio_postgres::{SimpleQueryMessage, SimpleQueryRow}; +use tokio_postgres::SimpleQueryMessage; mod common; @@ -24,7 +24,7 @@ async fn primary_key_insert() -> Result<(), Box> { let row_count = rows.into_iter().fold(0, |acc, x| -> usize { if let SimpleQueryMessage::Row(_) = x { - return acc + 1; + acc + 1 } else { acc } diff --git a/tests/sql_simple_select.rs b/tests/sql_simple_select.rs index 946a76b..c9c64f7 100644 --- a/tests/sql_simple_select.rs +++ b/tests/sql_simple_select.rs @@ -25,7 +25,7 @@ async fn simple_select() -> Result<(), Box> { assert_eq!(s.get(1).unwrap(), "three"); assert_eq!(s.get(2), None); } - _ => assert!(false), + _ => panic!("Shouldn't get here"), } common::_request_shutdown(request_shutdown).await