diff --git a/Cargo.lock b/Cargo.lock index e82c7f0..f07c1e0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -156,7 +156,7 @@ dependencies = [ "objc2-foundation", "parking_lot", "percent-encoding", - "windows-sys 0.52.0", + "windows-sys 0.59.0", "x11rb", ] @@ -952,7 +952,7 @@ dependencies = [ [[package]] name = "client" -version = "0.8.24" +version = "0.8.25" dependencies = [ "anyhow", "async-trait", @@ -1045,7 +1045,7 @@ dependencies = [ [[package]] name = "common" -version = "0.8.24" +version = "0.8.25" dependencies = [ "prost", "prost-build", @@ -2257,7 +2257,7 @@ dependencies = [ "libc", "option-ext", "redox_users", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -2400,7 +2400,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" dependencies = [ "libc", - "windows-sys 0.52.0", + "windows-sys 0.59.0", ] [[package]] @@ -4242,7 +4242,7 @@ version = "0.50.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7957b9740744892f114936ab4a57b3f487491bbeafaf8083688b16841a4240e5" dependencies = [ - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -5719,7 +5719,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys", - "windows-sys 0.52.0", + "windows-sys 0.59.0", ] [[package]] @@ -5803,7 +5803,7 @@ checksum = "1c107b6f4780854c8b126e228ea8869f4d7b71260f962fefb57b996b8959ba6b" [[package]] name = "search" -version = "0.8.24" +version = "0.8.25" dependencies = [ "anyhow", "common", @@ -5953,7 +5953,7 @@ dependencies = [ [[package]] name = "server" -version = "0.8.24" +version = "0.8.25" dependencies = [ "analytics-graphs", "anyhow", @@ -6887,7 +6887,7 @@ dependencies = [ "getrandom 0.4.2", "once_cell", "rustix", - "windows-sys 0.52.0", + "windows-sys 0.59.0", ] [[package]] @@ -7451,7 +7451,7 @@ checksum = "e78122066b0cb818b8afd08f7ed22f7fdbc3e90815035726f0840d0d26c0747a" [[package]] name = "tui-canvas" -version = "0.8.24" +version = "0.8.25" dependencies = [ "anyhow", "arboard", @@ -7470,7 +7470,7 @@ dependencies = [ "toml", "tracing", "tracing-subscriber", - "tui-canvas-validation-core 0.8.24", + "tui-canvas-validation-core 0.8.25", "unicode-width 0.2.2", ] @@ -7487,7 +7487,7 @@ dependencies = [ [[package]] name = "tui-canvas-validation-core" -version = "0.8.24" +version = "0.8.25" dependencies = [ "regex", "serde", @@ -7497,7 +7497,7 @@ dependencies = [ [[package]] name = "tui-pages" -version = "0.8.24" +version = "0.8.25" dependencies = [ "crossterm", "nucleo", @@ -8140,7 +8140,7 @@ version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c2a7b1c03c876122aa43f3020e6c3c3ee5c05081c9a00739faf7503aeba10d22" dependencies = [ - "windows-sys 0.52.0", + "windows-sys 0.59.0", ] [[package]] diff --git a/client b/client index 0d20818..643795a 160000 --- a/client +++ b/client @@ -1 +1 @@ -Subproject commit 0d20818048e7919c153827891e5cb5fa663bd103 +Subproject commit 643795ac6f9dff97bd9595b344b0d659bb4c6c37 diff --git a/common/proto/search.proto b/common/proto/search.proto index bb68c45..344220a 100644 --- a/common/proto/search.proto +++ b/common/proto/search.proto @@ -25,6 +25,18 @@ message SearchRequest { repeated ColumnConstraint must = 4; optional uint32 limit = 5; optional uint32 offset = 6; + optional SearchOrder order = 7; +} + +message SearchOrder { + string column = 1; + SearchOrderDirection direction = 2; +} + +enum SearchOrderDirection { + SEARCH_ORDER_DIRECTION_UNSPECIFIED = 0; + SEARCH_ORDER_DIRECTION_ASC = 1; + SEARCH_ORDER_DIRECTION_DESC = 2; } message SearchResponse { message Hit { @@ -35,6 +47,7 @@ message SearchResponse { // Configured human-readable value for this row. string row_display_value = 5; string row_display_column = 6; + optional uint64 position = 7; } repeated Hit hits = 1; } diff --git a/common/src/proto/descriptor.bin b/common/src/proto/descriptor.bin index 359503d..a79f34b 100644 Binary files a/common/src/proto/descriptor.bin and b/common/src/proto/descriptor.bin differ diff --git a/common/src/proto/komp_ac.search.rs b/common/src/proto/komp_ac.search.rs index 56e91d5..fccc0dd 100644 --- a/common/src/proto/komp_ac.search.rs +++ b/common/src/proto/komp_ac.search.rs @@ -22,6 +22,15 @@ pub struct SearchRequest { pub limit: ::core::option::Option, #[prost(uint32, optional, tag = "6")] pub offset: ::core::option::Option, + #[prost(message, optional, tag = "7")] + pub order: ::core::option::Option, +} +#[derive(Clone, PartialEq, Eq, Hash, ::prost::Message)] +pub struct SearchOrder { + #[prost(string, tag = "1")] + pub column: ::prost::alloc::string::String, + #[prost(enumeration = "SearchOrderDirection", tag = "2")] + pub direction: i32, } #[derive(Clone, PartialEq, ::prost::Message)] pub struct SearchResponse { @@ -46,6 +55,8 @@ pub mod search_response { pub row_display_value: ::prost::alloc::string::String, #[prost(string, tag = "6")] pub row_display_column: ::prost::alloc::string::String, + #[prost(uint64, optional, tag = "7")] + pub position: ::core::option::Option, } } #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, ::prost::Enumeration)] @@ -77,6 +88,35 @@ impl MatchMode { } } } +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, ::prost::Enumeration)] +#[repr(i32)] +pub enum SearchOrderDirection { + Unspecified = 0, + Asc = 1, + Desc = 2, +} +impl SearchOrderDirection { + /// String value of the enum field names used in the ProtoBuf definition. + /// + /// The values are not transformed in any way and thus are considered stable + /// (if the ProtoBuf definition does not change) and safe for programmatic use. + pub fn as_str_name(&self) -> &'static str { + match self { + Self::Unspecified => "SEARCH_ORDER_DIRECTION_UNSPECIFIED", + Self::Asc => "SEARCH_ORDER_DIRECTION_ASC", + Self::Desc => "SEARCH_ORDER_DIRECTION_DESC", + } + } + /// Creates an enum from field names used in the ProtoBuf definition. + pub fn from_str_name(value: &str) -> ::core::option::Option { + match value { + "SEARCH_ORDER_DIRECTION_UNSPECIFIED" => Some(Self::Unspecified), + "SEARCH_ORDER_DIRECTION_ASC" => Some(Self::Asc), + "SEARCH_ORDER_DIRECTION_DESC" => Some(Self::Desc), + _ => None, + } + } +} /// Generated client implementations. pub mod searcher_client { #![allow( diff --git a/search/src/lib.rs b/search/src/lib.rs index c6f8efe..6307824 100644 --- a/search/src/lib.rs +++ b/search/src/lib.rs @@ -6,7 +6,9 @@ use std::sync::{Arc, Mutex}; use common::proto::komp_ac::search::searcher_server::Searcher; pub use common::proto::komp_ac::search::searcher_server::SearcherServer; -use common::proto::komp_ac::search::{SearchRequest, SearchResponse, search_response::Hit}; +use common::proto::komp_ac::search::{ + SearchOrderDirection, SearchRequest, SearchResponse, search_response::Hit, +}; use common::search::{SchemaFields, register_tokenizers, search_index_path}; use query_builder::{ ConstraintMode, SearchConstraint, SearchConstraintTarget, build_master_query, @@ -67,10 +69,11 @@ impl SearcherService { )); }; - let hits = fetch_latest_rows( + let hits = fetch_ordered_rows( &self.pool, &normalized.profile_name, table_name, + normalized.order.as_ref(), normalized.limit.unwrap_or(DEFAULT_LIST_LIMIT), normalized.offset, ) @@ -104,13 +107,16 @@ impl SearcherService { &resolved_must, normalized.limit.unwrap_or(DEFAULT_RESULT_LIMIT), normalized.offset, + normalized.order.as_ref(), ) .await?; - hits.sort_by(|left, right| right.score.total_cmp(&left.score)); - if let Some(limit) = normalized.limit { - if hits.len() > limit { - hits.truncate(limit); + if normalized.order.is_none() { + hits.sort_by(|left, right| right.score.total_cmp(&left.score)); + if let Some(limit) = normalized.limit { + if hits.len() > limit { + hits.truncate(limit); + } } } @@ -167,6 +173,13 @@ struct NormalizedSearchRequest { must: Vec, limit: Option, offset: usize, + order: Option, +} + +#[derive(Debug)] +struct NormalizedSearchOrder { + column: String, + direction: SearchOrderDirection, } #[derive(Debug)] @@ -315,6 +328,34 @@ fn normalize_request(req: SearchRequest) -> Result Result String { } } -async fn fetch_latest_rows( +enum ResolvedOrderColumn { + Position, + Column(String), +} + +fn order_direction_sql(direction: SearchOrderDirection) -> &'static str { + match direction { + SearchOrderDirection::Asc => "ASC", + SearchOrderDirection::Desc => "DESC", + SearchOrderDirection::Unspecified => unreachable!("order direction is normalized"), + } +} + +async fn resolve_order_column( pool: &PgPool, profile_name: &str, table_name: &str, + requested_column: &str, +) -> Result { + if requested_column.eq_ignore_ascii_case("position") { + return Ok(ResolvedOrderColumn::Position); + } + + let requested_column = if requested_column.eq_ignore_ascii_case("row_display_column") { + table_row_display_column(pool, profile_name, table_name).await? + } else { + requested_column.to_string() + }; + let physical_to_display = + table_physical_to_display_map(pool, profile_name, table_name).await?; + let physical_column = physical_to_display + .iter() + .find(|(physical, display)| { + physical.eq_ignore_ascii_case(&requested_column) + || display.eq_ignore_ascii_case(&requested_column) + }) + .map(|(physical, _)| physical.clone()) + .unwrap_or_else(|| requested_column.clone()); + + let physical_column = sqlx::query_scalar::<_, String>( + r#" + SELECT column_name + FROM information_schema.columns + WHERE table_schema = $1 AND table_name = $2 AND LOWER(column_name) = LOWER($3) + "#, + ) + .bind(profile_name) + .bind(table_name) + .bind(&physical_column) + .fetch_optional(pool) + .await + .map_err(|e| Status::internal(format!("Order column lookup failed: {}", e)))?; + let Some(physical_column) = physical_column else { + return Err(Status::invalid_argument(format!( + "Order column '{}' was not found in table '{}.{}'", + requested_column, profile_name, table_name + ))); + }; + + Ok(ResolvedOrderColumn::Column(physical_column)) +} + +fn order_clause(column: &ResolvedOrderColumn, direction: SearchOrderDirection) -> String { + let direction = order_direction_sql(direction); + match column { + ResolvedOrderColumn::Position => { + format!("picker_position {}", direction) + } + ResolvedOrderColumn::Column(column) => { + let quoted_column = format!("\"{}\"", column.replace('"', "\"\"")); + format!("{} {} NULLS LAST, id ASC", quoted_column, direction) + } + } +} + +async fn fetch_ordered_rows( + pool: &PgPool, + profile_name: &str, + table_name: &str, + order: Option<&NormalizedSearchOrder>, limit: usize, offset: usize, ) -> Result, Status> { let physical_to_display = table_physical_to_display_map(pool, profile_name, table_name).await?; let display_column = table_row_display_column(pool, profile_name, table_name).await?; + let (resolved_order, direction) = match order { + Some(order) => ( + resolve_order_column(pool, profile_name, table_name, &order.column).await?, + order.direction, + ), + None => (ResolvedOrderColumn::Position, SearchOrderDirection::Desc), + }; let sql = format!( - "SELECT id, to_jsonb(t) AS data FROM {} t WHERE deleted = FALSE ORDER BY id DESC LIMIT $1 OFFSET $2", - qualify_profile_table(profile_name, table_name) + "WITH positioned AS (\ + SELECT t.*, ROW_NUMBER() OVER (ORDER BY id ASC) AS picker_position \ + FROM {} t WHERE deleted = FALSE\ + ) \ + SELECT id, to_jsonb(positioned) - 'picker_position' AS data, picker_position \ + FROM positioned ORDER BY {} LIMIT $1 OFFSET $2", + qualify_profile_table(profile_name, table_name), + order_clause(&resolved_order, direction), ); let rows = sqlx::query(AssertSqlSafe(sql)) @@ -533,6 +664,7 @@ async fn fetch_latest_rows( .map(|row| { let id: i64 = row.try_get("id").unwrap_or_default(); let json_data: serde_json::Value = row.try_get("data").unwrap_or_default(); + let position: i64 = row.try_get("picker_position").unwrap_or_default(); let json_data = remap_json_to_display_names(json_data, &physical_to_display); let row_display_value = row_display_value(&json_data, &display_column); Hit { @@ -542,6 +674,7 @@ async fn fetch_latest_rows( table_name: table_name.to_string(), row_display_value, row_display_column: display_column.clone(), + position: u64::try_from(position).ok(), } }) .collect()) @@ -556,6 +689,7 @@ async fn run_search( must: &[SearchConstraint], limit: usize, offset: usize, + order: Option<&NormalizedSearchOrder>, ) -> Result, Status> { let master_query = build_master_query( &profile.index, @@ -566,7 +700,14 @@ async fn run_search( )?; let searcher = profile.reader.searcher(); - let window_limit = offset.saturating_add(limit); + let window_limit = if order.is_some() { + searcher.num_docs() as usize + } else { + offset.saturating_add(limit) + }; + if window_limit == 0 { + return Ok(Vec::new()); + } let top_docs = searcher .search(&*master_query, &TopDocs::with_limit(window_limit).order_by_score()) .map_err(|e| Status::internal(format!("Search failed: {}", e)))?; @@ -581,12 +722,19 @@ async fn run_search( } else { 0.0 }; - let page_docs = top_docs + let eligible_docs = top_docs .into_iter() - .skip(offset) .filter(|(score, _)| *score >= score_floor) - .take(limit) .collect::>(); + let page_docs = if order.is_some() { + eligible_docs + } else { + eligible_docs + .into_iter() + .skip(offset) + .take(limit) + .collect::>() + }; let mut candidates: Vec<(f32, i64, String)> = Vec::with_capacity(page_docs.len()); for (score, doc_address) in page_docs { @@ -612,6 +760,20 @@ async fn run_search( return Ok(vec![]); } + if let Some(order) = order { + let table_name = table_filter.expect("ordered searches require a normalized table filter"); + return fetch_ordered_candidate_rows( + pool, + profile_name, + table_name, + &candidates, + order, + limit, + offset, + ) + .await; + } + let mut ids_by_table: HashMap> = HashMap::new(); for (_, pg_id, table_name) in &candidates { ids_by_table @@ -661,11 +823,72 @@ async fn run_search( table_name, row_display_value: row_display_value.clone(), row_display_column: row_display_column.clone(), + position: None, }) }) .collect()) } +async fn fetch_ordered_candidate_rows( + pool: &PgPool, + profile_name: &str, + table_name: &str, + candidates: &[(f32, i64, String)], + order: &NormalizedSearchOrder, + limit: usize, + offset: usize, +) -> Result, Status> { + let physical_to_display = table_physical_to_display_map(pool, profile_name, table_name).await?; + let display_column = table_row_display_column(pool, profile_name, table_name).await?; + let resolved_order = + resolve_order_column(pool, profile_name, table_name, &order.column).await?; + let sql = format!( + "WITH positioned AS (\ + SELECT t.*, ROW_NUMBER() OVER (ORDER BY id ASC) AS picker_position \ + FROM {} t WHERE deleted = FALSE\ + ) \ + SELECT id, to_jsonb(positioned) - 'picker_position' AS data, picker_position \ + FROM positioned WHERE id = ANY($1) ORDER BY {} LIMIT $2 OFFSET $3", + qualify_profile_table(profile_name, table_name), + order_clause(&resolved_order, order.direction), + ); + let ids = candidates + .iter() + .map(|(_, id, _)| *id) + .collect::>(); + let scores = candidates + .iter() + .map(|(score, id, _)| (*id, *score)) + .collect::>(); + let rows = sqlx::query(AssertSqlSafe(sql)) + .bind(&ids) + .bind(limit as i64) + .bind(offset as i64) + .fetch_all(pool) + .await + .map_err(|e| Status::internal(format!("Ordered search query failed: {}", e)))?; + + Ok(rows + .into_iter() + .map(|row| { + let id: i64 = row.try_get("id").unwrap_or_default(); + let json_data: serde_json::Value = row.try_get("data").unwrap_or_default(); + let position: i64 = row.try_get("picker_position").unwrap_or_default(); + let json_data = remap_json_to_display_names(json_data, &physical_to_display); + let display_value = row_display_value(&json_data, &display_column); + Hit { + id, + score: scores.get(&id).copied().unwrap_or_default(), + content_json: json_data.to_string(), + table_name: table_name.to_string(), + row_display_value: display_value, + row_display_column: display_column.clone(), + position: u64::try_from(position).ok(), + } + }) + .collect()) +} + #[tonic::async_trait] impl Searcher for SearcherService { async fn search( @@ -675,3 +898,61 @@ impl Searcher for SearcherService { self.run_rpc(request).await } } + +#[cfg(test)] +mod tests { + use super::*; + use common::proto::komp_ac::search::{SearchOrder, SearchOrderDirection}; + + fn request(order: Option, table_name: Option<&str>) -> SearchRequest { + SearchRequest { + profile_name: "finance".to_string(), + table_name: table_name.map(str::to_string), + free_query: String::new(), + must: Vec::new(), + limit: Some(20), + offset: Some(40), + order, + } + } + + #[test] + fn normalizes_typed_column_order() { + let normalized = normalize_request(request( + Some(SearchOrder { + column: "created_at".to_string(), + direction: SearchOrderDirection::Desc as i32, + }), + Some("invoice"), + )) + .unwrap(); + let order = normalized.order.unwrap(); + + assert_eq!(order.column, "created_at"); + assert_eq!(order.direction, SearchOrderDirection::Desc); + } + + #[test] + fn ordered_request_requires_a_table() { + let result = normalize_request(request( + Some(SearchOrder { + column: "position".to_string(), + direction: SearchOrderDirection::Desc as i32, + }), + None, + )); + + assert!(result.is_err()); + } + + #[test] + fn column_order_has_stable_id_tie_breaker() { + assert_eq!( + order_clause( + &ResolvedOrderColumn::Column("1".to_string()), + SearchOrderDirection::Asc, + ), + "\"1\" ASC NULLS LAST, id ASC" + ); + } +}