Files that changed from the base of the PR and between 80908c5 and 861d4d1.

• src/config/src/config.rs (2 hunks)
• src/infra/src/db/mod.rs (2 hunks)
• src/infra/src/dist_lock.rs (1 hunks)
• src/infra/src/file_list/mod.rs (8 hunks)
• src/infra/src/file_list/mysql.rs (11 hunks)
• src/infra/src/file_list/postgres.rs (5 hunks)
• src/infra/src/file_list/sqlite.rs (9 hunks)
• src/infra/src/lib.rs (1 hunks)
• src/job/mod.rs (1 hunks)
• src/service/db/file_list/remote.rs (1 hunks)
• src/service/file_list.rs (8 hunks)
• src/service/search/cluster/flight.rs (3 hunks)
• src/service/search/grpc/flight.rs (5 hunks)
• src/service/search/grpc/storage.rs (1 hunks)

src/infra/src/db/mod.rs (2)

36-36: LGTM: Addition of LOCAL_CACHE static variable

The addition of the LOCAL_CACHE static variable using OnceCell is consistent with the existing code structure and aligns with the PR objective of implementing a local cache. This approach ensures thread-safe lazy initialization.

---

Line range hint 1-359: Overall assessment: Changes look good with minor suggestions

The implemented changes for adding a local SQLite cache are well-structured and align with the PR objectives. The code is consistent with the existing patterns in the file. Consider implementing the suggested improvements for documentation and error handling to further enhance the robustness of the implementation.

src/service/search/grpc/storage.rs (1)

555-559: Improved logging for bitmap results

The change replaces logging the entire bitmap with logging its length, which is a good improvement for large bitmaps. This change reduces log verbosity and potential performance impact for large result sets.

However, consider the following suggestions:

1. Add a debug-level log that includes the full bitmap for troubleshooting purposes.
2. Consider using a structured logging approach for better parsing and analysis of logs.

Here's an example of how you could implement these suggestions:

 log::info!(
-    "[trace_id {}] search->storage: Final bitmap for fts_terms {:?} and index_terms: {:?} length {}",
-    query.trace_id,
-    *full_text_terms,
-    index_terms,
-    res.len(),
+    "Final bitmap results for search terms";
+    "trace_id" => query.trace_id,
+    "fts_terms" => ?*full_text_terms,
+    "index_terms" => ?index_terms,
+    "bitmap_length" => res.len()
 );
+log::debug!(
+    "Full bitmap result";
+    "trace_id" => query.trace_id,
+    "bitmap" => ?res
+);

This change maintains the performance improvement while providing more detailed logging when needed.

src/infra/src/file_list/postgres.rs (2)

Line range hint 362-394: LGTM: query_by_ids method update looks good.

The changes to the query_by_ids method, including the updated return type and the inclusion of the id in the query and result mapping, are well-implemented and consistent.

---

501-508: LGTM: New get_min_pk_value method is well-implemented.

The get_min_pk_value method is correctly implemented to retrieve the minimum primary key value from the file_list table. This could be useful for various operations requiring knowledge of the oldest record.

src/config/src/config.rs (1)

991-992: Approved: Increased maximum file size for compaction

The default value for max_file_size has been increased from 256 MB to 512 MB. This change aligns with the PR objectives of enhancing efficiency in file retrieval processes. However, be aware that this may impact memory usage and processing time for larger files during compaction.

src/service/file_list.rs (2)

68-128: New caching logic in query_by_ids enhances performance

The updated query_by_ids function introduces local caching, effectively reducing remote database calls. The implementation correctly handles cached and uncached IDs and updates the local cache as needed.

---

228-236: Improved error handling in delete_parquet_file_s3 function

Mapping errors to Error::Message provides clearer error reporting, aligning with the overall error handling strategy.

src/service/search/grpc/flight.rs (4)

Line range hint 155-165: Correctly updated function call with error handling

The get_file_list_by_ids function is now called with the additional trace_id parameter, and the returned Result is appropriately handled using .await?.

---

Line range hint 273-282: Improved function signature for better traceability and error handling

The get_file_list_by_ids function now includes trace_id in its parameters, enhancing logging and traceability. The return type has been updated to Result<(Vec<FileKey>, usize), Error>, allowing for proper error propagation.

---

284-284: Proper error handling in query_by_ids call

The call to query_by_ids now includes trace_id and correctly handles errors using the ? operator, ensuring that any errors are propagated upwards.

---

326-326: Returning Result with accurate timing information

The function now returns a Result containing the files and the elapsed time in milliseconds, ensuring accurate performance metrics.

src/infra/src/file_list/mod.rs (6)

34-34: Initialization of LOCAL_CACHE is appropriate

The use of Lazy to initialize LOCAL_CACHE ensures thread-safe lazy initialization.

---

46-48: New function connect_local_cache added correctly

The function connect_local_cache correctly returns a default SqliteFileList, facilitating local caching.

---

58-58: New method batch_add_with_id added to FileList trait

The method batch_add_with_id is correctly defined in the FileList trait.

---

105-106: New methods get_min_pk_value and clean_by_min_pk_value added

The methods are correctly added to the FileList trait for cache garbage collection purposes.

---

306-308: Function get_min_pk_value correctly implemented

The function correctly delegates to CLIENT.get_min_pk_value().await.

---

421-445: Implementation of local_cache_gc is appropriate

The background task for garbage collection is correctly implemented using tokio::task::spawn and runs at the specified hourly interval.

src/service/search/cluster/flight.rs (3)

121-124: Verify the Correctness of use_fst_inverted_index Assignment

The variable use_fst_inverted_index is assigned from the get_inverted_index_file_lists function and then used to set req.use_inverted_index. Ensure that the logic determining use_fst_inverted_index is correct and aligns with the intended behavior, especially considering different configurations of inverted_index_type.

---

799-800: Ensure Early Return Handles All Cases Appropriately

The early return when !use_parquet_inverted_index is true results in:

return Ok((use_fst_inverted_index, vec![], 0, 0));

Verify that returning empty idx_file_list, and zero values for idx_scan_size and idx_took is appropriate in this context. Ensure that downstream code properly handles these values without causing unintended side effects.

---

822-827: Check Consistency of Return Values

At the end of the get_inverted_index_file_lists function, the return statement is:

Ok((
    use_fst_inverted_index,
    idx_file_list,
    idx_scan_size,
    idx_took,
))

Confirm that idx_file_list, idx_scan_size, and idx_took are correctly initialized in all execution paths, especially when use_parquet_inverted_index is false and the function returns early. This will prevent potential runtime errors due to uninitialized or default values.

src/infra/src/file_list/sqlite.rs (5)

Line range hint 366-396: Validate handling of empty result sets in query_by_ids

In the query_by_ids method, ensure that the function correctly handles cases where the provided ids do not exist in the database, returning an empty vector as expected.

You can verify this behavior by adding unit tests that pass non-existent IDs and checking that the method returns an empty result without errors.

---

505-511: Confirm correct default behavior in get_min_pk_value

The method get_min_pk_value returns the minimum primary key value or defaults to zero if none is found. Ensure that this default behavior aligns with the application's expectations when the table is empty.

Consider adding a unit test to validate that the method returns zero when the file_list table is empty.

---

513-521: Ensure proper error handling in clean_by_min_pk_value

In clean_by_min_pk_value, lines 513-521, ensure that the method handles scenarios where the deletion might fail due to database constraints or locks.

Add error handling to catch exceptions from the delete operation and log appropriate error messages. This will aid in troubleshooting issues in production environments.

---

968-974: Handle optional IDs appropriately in inner_add_with_id

In inner_add_with_id, ensure that when id is None, the database handles the auto-increment of the primary key as expected.

Review the database schema and constraints to confirm that inserting a NULL value for id results in the primary key being auto-generated.

---

72-78: Ensure consistency with existing methods in batch_add_with_id

The newly added batch_add_with_id method at lines 72-78 introduces functionality to batch add files with IDs. Ensure that this method aligns with the behavior of batch_add and properly handles cases where IDs might already exist in the database.

It's important to confirm that the method handles duplicate IDs gracefully. You can use the following script to check for any inconsistencies:

src/infra/src/file_list/mysql.rs (1)

297-299: Potential performance impact due to removal of FORCE INDEX

The removal of FORCE INDEX (file_list_stream_ts_idx) from multiple SQL queries may lead to suboptimal query performance if the database does not automatically choose the optimal index. It's important to ensure that the queries still utilize the appropriate indexes to maintain efficiency.

Please verify that the database query planner continues to use the desired indexes. If not, consider adding optimizer hints or analyzing the need for additional indexing.

Also applies to: 314-316, 328-330

Files that changed from the base of the PR and between b102fed and 2a3d67a.

• src/config/src/config.rs (4 hunks)
• src/infra/src/file_list/mod.rs (8 hunks)
• src/infra/src/file_list/mysql.rs (11 hunks)
• src/infra/src/file_list/postgres.rs (5 hunks)
• src/infra/src/file_list/sqlite.rs (9 hunks)

• src/infra/src/file_list/mod.rs
• src/infra/src/file_list/mysql.rs
• src/infra/src/file_list/postgres.rs
• src/infra/src/file_list/sqlite.rs

src/config/src/config.rs (1)

1337-1340: LGTM! Good default value handling.

The addition of a check for meta_id_batch_size in the init function is a good practice. It ensures that the value is always set to a reasonable default (5000) if not specified or set to 0. This prevents potential issues that could arise from an invalid batch size.