feat: redis as vector store#450
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a Redis-backed vector store with KNN, CRUD, batching, namespace management and tests; extends vectorstore factory and CreateNamespace to accept dimension; makes semanticcache namespace and dimension configurable; improves stream_chunks parsing; consolidates vector-store docs and adds a UI Dimension field. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Factory as NewVectorStore
participant Redis as Redis Server
participant RedisStore as RedisStore
App->>Factory: NewVectorStore(ctx, {Type: "redis", Config: RedisConfig})
Factory->>Factory: unmarshal -> RedisConfig
Factory->>Redis: Dial + Ping (newRedisStore)
Redis-->>Factory: Pong
Factory-->>App: RedisStore instance
App->>RedisStore: CreateNamespace(namespace, dimension, properties)
RedisStore->>Redis: FT.CREATE (HASH + VECTOR field HNSW)
Redis-->>RedisStore: OK
sequenceDiagram
autonumber
participant Client
participant Plugin as SemanticCache
participant Store as VectorStore
Client->>Plugin: performSemanticSearch(req)
Plugin->>Store: GetNearest(plugin.config.VectorStoreNamespace, vector, filters, threshold, limit)
Store-->>Plugin: []SearchResult
Plugin->>Plugin: parseStreamChunks(stream_chunks)
Plugin-->>Client: semantic response (streaming if valid)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/semanticcache/test_helpers.go (1)
198-201: Bug: “RedisCluster” setup still uses WeaviateFunction name and comment say Redis Cluster, but it constructs a Weaviate store. Switch to Redis (and consider adding cluster-specific env later).
- store, err := vectorstore.NewVectorStore(ctx, &vectorstore.Config{ - Type: vectorstore.VectorStoreTypeWeaviate, - Config: getWeaviateConfigFromEnv(), - }, logger) + store, err := vectorstore.NewVectorStore(ctx, &vectorstore.Config{ + Type: vectorstore.VectorStoreTypeRedis, + Config: getRedisConfigFromEnv(), + }, logger)
🧹 Nitpick comments (14)
framework/vectorstore/test_helpers..go (4)
30-36: Make embeddings deterministic to avoid test flakesUse a dedicated, seeded RNG instead of the global
rand. This stabilizes vector-similarity expectations in tests.Apply:
@@ -import ( +import ( "math/rand" "os" "strconv" "github.com/google/uuid" ) +// Seeded RNG for deterministic tests +var testRand = rand.New(rand.NewSource(42)) + @@ func generateTestEmbedding(dim int) []float32 { embedding := make([]float32, dim) for i := range embedding { - embedding[i] = rand.Float32()*2 - 1 // Random values between -1 and 1 + embedding[i] = testRand.Float32()*2 - 1 // Random values between -1 and 1 } return embedding }
38-46: Clamp similarity and use deterministic noisePrevent out-of-range inputs from increasing noise and reuse the seeded RNG for determinism.
func generateSimilarEmbedding(original []float32, similarity float32) []float32 { + if similarity < 0 { + similarity = 0 + } else if similarity > 1 { + similarity = 1 + } similar := make([]float32, len(original)) for i := range similar { // Add small random noise to create similar but not identical embedding - noise := (rand.Float32()*2 - 1) * (1 - similarity) * 0.1 + noise := (testRand.Float32()*2 - 1) * (1 - similarity) * 0.1 similar[i] = original[i] + noise } return similar }
19-24: Consider soft-fail on parse errorReturning an error for a bad env int forces callers to plumb/fail. For test helpers, falling back to
defaultValue(with a log) typically yields more resilient tests.
1-1: Nit: filename has a double dotRename to
test_helpers.gofor consistency.framework/go.mod (1)
68-68: go-redis should likely be a direct dependency
github.com/redis/go-redis/v9is used byframework/vectorstore/redis.go; mark it direct and rungo mod tidyto drop unused deps (e.g., Weaviate if fully migrated).framework/vectorstore/store.go (2)
157-166: Accept both value and pointer configs in factoryType assertions will fail if callers pass
*RedisConfig. Handle both to reduce footguns.- case VectorStoreTypeRedis: - if config.Config == nil { - return nil, fmt.Errorf("redis config is required") - } - redisConfig, ok := config.Config.(RedisConfig) - if !ok { - return nil, fmt.Errorf("invalid redis config") - } - return newRedisStore(ctx, redisConfig, logger) + case VectorStoreTypeRedis: + if config.Config == nil { + return nil, fmt.Errorf("redis config is required") + } + var redisConfig RedisConfig + switch v := config.Config.(type) { + case RedisConfig: + redisConfig = v + case *RedisConfig: + redisConfig = *v + default: + return nil, fmt.Errorf("invalid redis config") + } + return newRedisStore(ctx, redisConfig, logger)
148-156: Mirror pointer/value handling for Weaviate pathFor parity and future-proofing, accept both
WeaviateConfigand*WeaviateConfig.- case VectorStoreTypeWeaviate: - if config.Config == nil { - return nil, fmt.Errorf("weaviate config is required") - } - weaviateConfig, ok := config.Config.(WeaviateConfig) - if !ok { - return nil, fmt.Errorf("invalid weaviate config") - } - return newWeaviateStore(ctx, &weaviateConfig, logger) + case VectorStoreTypeWeaviate: + if config.Config == nil { + return nil, fmt.Errorf("weaviate config is required") + } + var weaviateConfig WeaviateConfig + switch v := config.Config.(type) { + case WeaviateConfig: + weaviateConfig = v + case *WeaviateConfig: + weaviateConfig = *v + default: + return nil, fmt.Errorf("invalid weaviate config") + } + return newWeaviateStore(ctx, &weaviateConfig, logger)plugins/semanticcache/test_helpers.go (1)
134-145: Use the created context instead of context.Background()Minor consistency/readability improvement.
- store, err := vectorstore.NewVectorStore(context.Background(), &vectorstore.Config{ + store, err := vectorstore.NewVectorStore(ctx, &vectorstore.Config{ Type: vectorstore.VectorStoreTypeRedis, Config: getRedisConfigFromEnv(), Enabled: true, }, logger)framework/vectorstore/redis_test.go (3)
228-233: Close store in validation test to avoid leaksEnsure resources are released for the “valid config” branch.
} else { // For valid configs, store creation should succeed // (connection will fail later when actually using Redis) assert.NoError(t, err) assert.NotNil(t, store) + if store != nil { + _ = store.Close(ctx, TestNamespace) + } }
329-336: Drop heavy JSON logging in hot pathsPretty-printing results in tests is noisy and slows test runs. Keep only on failures or behind
t.Logfguarded bytesting.Verbose().
380-387: Replace fixed sleeps with eventual pollingUse polling with a timeout to reduce flakiness and speed up tests on fast environments.
I can propose a small
waitUntilhelper if you want it added.Also applies to: 457-466, 653-663, 719-726, 801-816
framework/vectorstore/redis.go (3)
580-605: TTL logic should handle all numeric types safelyThe TTL extraction code handles multiple numeric types but could be simplified and made more robust using type assertion with ok pattern consistently.
// Set TTL if expires_at is provided in metadata if expiresAtRaw, exists := metadata["expires_at"]; exists && expiresAtRaw != nil { - var expiresAt int64 - var validType bool - switch v := expiresAtRaw.(type) { - case float64: - expiresAt = int64(v) - validType = true - case int64: - expiresAt = v - validType = true - case int: - expiresAt = int64(v) - validType = true - } - if validType { + expiresAt := extractInt64(expiresAtRaw) + if expiresAt > 0 { currentTime := time.Now().Unix() ttlSeconds := expiresAt - currentTime if ttlSeconds > 0 { // Set Redis TTL if err := s.client.Expire(ctx, key, time.Duration(ttlSeconds)*time.Second).Err(); err != nil { s.logger.Debug(fmt.Sprintf("Failed to set TTL for key %s: %v", key, err)) } } } }Add a helper function:
func extractInt64(v interface{}) int64 { switch val := v.(type) { case float64: return int64(val) case int64: return val case int: return int64(val) case float32: return int64(val) case int32: return int64(val) default: return 0 } }
516-516: Unnecessary RETURN clause in FT.SEARCHThe RETURN clause with
"3", "id", "score", "*"appears to be ignored based on the comment on line 531 about fetching properties separately.Since the properties are fetched separately anyway, consider simplifying:
- "RETURN", "3", "id", "score", "*", + "RETURN", "1", "score",
846-863: Document and standardize score field handling: Add a comment in framework/vectorstore/redis.go explaining that__embedding_scoreis a legacy field (RediSearch dialect 1) andscoreis used in dialect 2, then remove the fallback or update tests/docs if the legacy key is no longer needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
framework/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
framework/go.mod(2 hunks)framework/vectorstore/redis.go(1 hunks)framework/vectorstore/redis_test.go(1 hunks)framework/vectorstore/store.go(3 hunks)framework/vectorstore/test_helpers..go(1 hunks)framework/vectorstore/weaviate_test.go(0 hunks)plugins/semanticcache/test_helpers.go(2 hunks)
💤 Files with no reviewable changes (1)
- framework/vectorstore/weaviate_test.go
🧰 Additional context used
🧬 Code graph analysis (4)
framework/vectorstore/redis_test.go (5)
framework/vectorstore/redis.go (2)
RedisStore(37-41)RedisConfig(17-34)core/schemas/logger.go (2)
Logger(28-55)LogLevelInfo(11-11)framework/vectorstore/store.go (14)
Config(92-96)VectorStoreProperties(63-66)VectorStorePropertyTypeString(71-71)VectorStorePropertyTypeInteger(72-72)VectorStorePropertyTypeBoolean(73-73)Query(20-24)QueryOperatorEqual(29-29)QueryOperatorGreaterThan(31-31)QueryOperatorLike(35-35)QueryOperatorLessThan(32-32)QueryOperatorGreaterThanOrEqual(33-33)VectorStore(78-89)VectorStoreTypeRedis(16-16)NewVectorStore(138-168)core/logger.go (1)
NewDefaultLogger(40-49)framework/vectorstore/weaviate_test.go (1)
TestEmbeddingDim(21-21)
framework/vectorstore/redis.go (3)
core/schemas/logger.go (1)
Logger(28-55)framework/vectorstore/store.go (16)
VectorStoreProperties(63-66)SearchResult(43-47)Query(20-24)QueryOperatorIsNull(38-38)QueryOperatorEqual(29-29)QueryOperatorNotEqual(30-30)QueryOperatorLike(35-35)QueryOperatorIsNotNull(39-39)QueryOperatorGreaterThan(31-31)QueryOperatorGreaterThanOrEqual(33-33)QueryOperatorLessThan(32-32)QueryOperatorLessThanOrEqual(34-34)QueryOperator(26-26)DeleteResult(50-54)DeleteStatusError(60-60)DeleteStatusSuccess(59-59)plugins/semanticcache/main.go (1)
VectorStoreProperties(150-187)
plugins/semanticcache/test_helpers.go (2)
framework/vectorstore/redis.go (1)
RedisConfig(17-34)framework/vectorstore/store.go (2)
VectorStoreTypeRedis(16-16)Config(92-96)
framework/vectorstore/store.go (1)
framework/vectorstore/redis.go (1)
RedisConfig(17-34)
🔇 Additional comments (3)
plugins/semanticcache/test_helpers.go (1)
138-142: LGTM: switch tests to Redis via factoryUsing
VectorStoreTypeRediswithgetRedisConfigFromEnv()aligns semantic-cache tests with the new backend.framework/vectorstore/redis.go (2)
358-363: IsNull operator logic may be incorrect for missing fieldsWhen a field doesn't exist, the IsNull operator continues (line 360), but line 362 returns false for other operators. This means IsNull returns true for missing fields, which might be the intended behavior, but it's worth confirming.
Can you confirm that treating missing fields as null (returning true for IsNull) is the intended behavior? This differs from some databases where NULL and missing are distinct concepts.
1-991: LGTM! Comprehensive Redis vector store implementationThe implementation provides a well-structured Redis-backed vector store with proper error handling, context management, and batch operations for efficiency. The code integrates cleanly with the existing VectorStore interface and includes thoughtful features like TTL support and pipeline-based operations.
TejasGhatte
force-pushed
the
09-02-feat_redis_as_vector_store
branch
from
12a4848 to
e625742
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
framework/vectorstore/redis_test.go (1)
144-150: Good: skip when RediSearch/schema creation fails.Switching to t.Skipf prevents cascading failures on environments without RediSearch.
framework/vectorstore/redis.go (5)
24-31: Consider removing or deprecating MaxActiveConns from RedisConfig.Since go-redis doesn’t support it, keeping it is misleading. Either drop it or document it as ignored.
56-105: Index numeric fields as NUMERIC (not TEXT) for efficient comparisons.All metadata fields are added as TEXT; numeric filters then require SCAN+client filtering. If properties include integers, map those to NUMERIC in FT schema.
// Add all metadata fields as TEXT with exact matching // All values are converted to strings for consistent searching for _, field := range metadataFields { - args = append(args, field, "TEXT", "NOSTEM") + if prop, ok := properties[field]; ok && prop.DataType == VectorStorePropertyTypeInteger { + args = append(args, field, "NUMERIC") + } else if ok && prop.DataType == VectorStorePropertyTypeBoolean { + // Store booleans as TAGs for exact match + args = append(args, field, "TAG") + } else { + args = append(args, field, "TEXT", "NOSTEM") + } }
778-792: Close duplicates cleanup; delegate to DeleteNamespace.Close currently only drops the index, leaving data behind. Delegate to DeleteNamespace for consistent teardown.
func (s *RedisStore) Close(ctx context.Context, namespace string) error { - ctx, cancel := s.withTimeout(ctx) - defer cancel() - - // Drop the index using FT.DROPINDEX - if err := s.client.Do(ctx, "FT.DROPINDEX", namespace).Err(); err != nil { - // Check if error is "Unknown Index name" - that's OK, index doesn't exist - if strings.Contains(err.Error(), "Unknown Index name") { - return nil // Index doesn't exist, nothing to drop - } - return fmt.Errorf("failed to drop semantic index %s: %w", namespace, err) - } - - return nil + return s.DeleteNamespace(ctx, namespace) }
972-982: Compile error: redis.Options has no MaxActiveConns field.go-redis/v9’s Options doesn’t define MaxActiveConns. This line won’t compile.
client := redis.NewClient(&redis.Options{ Addr: config.Addr, Username: config.Username, Password: config.Password, DB: config.DB, PoolSize: config.PoolSize, - MaxActiveConns: config.MaxActiveConns, MinIdleConns: config.MinIdleConns, MaxIdleConns: config.MaxIdleConns, ConnMaxLifetime: config.ConnMaxLifetime, ConnMaxIdleTime: config.ConnMaxIdleTime, DialTimeout: config.DialTimeout, ReadTimeout: config.ReadTimeout, WriteTimeout: config.WriteTimeout, })
425-469: Fix incorrect fallback for numeric comparisons.Falling back to string equality for >, >=, <, <= is wrong when parsing fails. Return false for ordering operators when values aren’t numeric.
if err1 != nil || err2 != nil { // If parsing as float fails, try as int64 @@ - if err1 != nil || err2 != nil { - // If both fail, fall back to string comparison - return propStr == queryStr - } + if err1 != nil || err2 != nil { + switch operator { + case QueryOperatorGreaterThan, QueryOperatorGreaterThanOrEqual, + QueryOperatorLessThan, QueryOperatorLessThanOrEqual: + return false + default: + return propStr == queryStr + } + }
🧹 Nitpick comments (7)
framework/vectorstore/store.go (1)
157-165: Accept both value and pointer Redis configs in NewVectorStore.Today only the value-typed RedisConfig works. Supporting pointers avoids surprises when callers construct configs programmatically.
case VectorStoreTypeRedis: if config.Config == nil { return nil, fmt.Errorf("redis config is required") } - redisConfig, ok := config.Config.(RedisConfig) - if !ok { - return nil, fmt.Errorf("invalid redis config") - } - return newRedisStore(ctx, redisConfig, logger) + var redisConfig RedisConfig + switch v := config.Config.(type) { + case RedisConfig: + redisConfig = v + case *RedisConfig: + if v == nil { + return nil, fmt.Errorf("invalid redis config: nil pointer") + } + redisConfig = *v + default: + return nil, fmt.Errorf("invalid redis config") + } + return newRedisStore(ctx, redisConfig, logger)framework/vectorstore/redis_test.go (1)
175-237: Unit test currently requires a live Redis; clarify or gate.newRedisStore pings Redis; “valid config” will fail without a running server. Either (a) mark this test as integration and Skip in short mode, or (b) refactor to validate config without connecting.
func TestRedisConfig_Validation(t *testing.T) { + if testing.Short() { + t.Skip("Skipping config-connection validation in short mode") + }If you prefer a pure unit test, extract a validateRedisConfig(config) error that doesn’t ping and test that instead.
framework/vectorstore/redis.go (5)
581-584: Use HSET instead of deprecated HMSet and pass fields variadically.HMSet is a compatibility alias; prefer HSet for clarity.
- if err := s.client.HMSet(ctx, key, fields).Err(); err != nil { + if err := s.client.HSet(ctx, key, fields).Err(); err != nil { return fmt.Errorf("failed to store semantic cache entry: %w", err) }
518-526: Add LIMIT to FT.SEARCH to align returned row count with KNN topK.Without LIMIT, RediSearch defaults can cap results independently of KNN. Append LIMIT 0 .
args := []interface{}{ "FT.SEARCH", namespace, fmt.Sprintf("%s=>[KNN %d @embedding $vec AS score]", hybridQuery, limit), "PARAMS", "2", "vec", queryBytes, "SORTBY", "score", "RETURN", "3", "id", "score", "*", "DIALECT", "2", + "LIMIT", "0", fmt.Sprint(limit), }
360-423: Implement missing operators (ContainsAny/ContainsAll) or reject explicitly.The VectorStore API defines ContainsAny/ContainsAll but matchesQueries doesn’t handle them; current default path devolves to string equality, which is wrong for these.
Minimal approach: explicitly return false with a TODO for these operators to avoid silent misbehavior. Alternatively, implement them for []string and comma-delimited string values.
271-277: Return nil cursor when no more pages.Returning a non-nil pointer to an empty string forces callers to check string content. Prefer nil to signal completion.
- // Set next cursor if there are more results - if nextCursor == "" && cursorValue != 0 { - nextCursor = strconv.FormatUint(cursorValue, 10) - } - - return allResults, &nextCursor, nil + // Set next cursor if there are more results + if nextCursor == "" && cursorValue != 0 { + v := strconv.FormatUint(cursorValue, 10) + return allResults, &v, nil + } + return allResults, nil, nil
795-884: Be defensive in FT.SEARCH result parsing.Relying on map[interface{}]interface{} with total_results/results may not hold across Redis/RediSearch versions and clients (many return []interface{} rows). Consider supporting the array-shaped response as a fallback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
framework/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
framework/go.mod(2 hunks)framework/vectorstore/redis.go(1 hunks)framework/vectorstore/redis_test.go(1 hunks)framework/vectorstore/store.go(3 hunks)framework/vectorstore/test_helpers..go(1 hunks)framework/vectorstore/weaviate_test.go(0 hunks)plugins/semanticcache/test_helpers.go(2 hunks)
💤 Files with no reviewable changes (1)
- framework/vectorstore/weaviate_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- framework/vectorstore/test_helpers..go
- framework/go.mod
- plugins/semanticcache/test_helpers.go
🧰 Additional context used
🧬 Code graph analysis (3)
framework/vectorstore/store.go (1)
framework/vectorstore/redis.go (1)
RedisConfig(17-38)
framework/vectorstore/redis_test.go (3)
framework/vectorstore/redis.go (2)
RedisStore(41-45)RedisConfig(17-38)core/schemas/logger.go (2)
Logger(28-55)LogLevelInfo(11-11)framework/vectorstore/store.go (14)
Config(92-96)VectorStoreProperties(63-66)VectorStorePropertyTypeString(71-71)VectorStorePropertyTypeInteger(72-72)VectorStorePropertyTypeBoolean(73-73)Query(20-24)QueryOperatorEqual(29-29)QueryOperatorGreaterThan(31-31)QueryOperatorLike(35-35)QueryOperatorLessThan(32-32)QueryOperatorGreaterThanOrEqual(33-33)VectorStore(78-89)VectorStoreTypeRedis(16-16)NewVectorStore(138-168)
framework/vectorstore/redis.go (3)
core/schemas/logger.go (1)
Logger(28-55)framework/vectorstore/store.go (16)
VectorStoreProperties(63-66)SearchResult(43-47)Query(20-24)QueryOperatorIsNull(38-38)QueryOperatorEqual(29-29)QueryOperatorNotEqual(30-30)QueryOperatorLike(35-35)QueryOperatorIsNotNull(39-39)QueryOperatorGreaterThan(31-31)QueryOperatorGreaterThanOrEqual(33-33)QueryOperatorLessThan(32-32)QueryOperatorLessThanOrEqual(34-34)QueryOperator(26-26)DeleteResult(50-54)DeleteStatusError(60-60)DeleteStatusSuccess(59-59)plugins/semanticcache/main.go (1)
VectorStoreProperties(150-187)
🔇 Additional comments (3)
framework/vectorstore/store.go (2)
16-16: LGTM: Added Redis store type constant.Constant definition is clear and consistent with existing types.
124-129: LGTM: UnmarshalJSON supports Redis config.Happy with the pattern mirroring Weaviate. Error messages are precise.
framework/vectorstore/redis_test.go (1)
571-579: Relax flakiness risk acknowledgment.Assertion targets only the top hit with a reasonable delta. Looks good.
TejasGhatte
force-pushed
the
09-02-feat_redis_as_vector_store
branch
from
e625742 to
04cca26
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
framework/vectorstore/redis_test.go (1)
144-149: Proper test resilience for missing RediSearch.The updated error handling correctly skips tests when RediSearch is unavailable rather than continuing with cascading failures. This makes the tests more robust in different deployment environments.
framework/vectorstore/redis.go (3)
93-97: Consider using Redis schema types for better performance.All metadata fields are currently stored as TEXT with NOSTEM, but this prevents efficient numeric operations. Consider detecting field types from the
VectorStorePropertiesand using appropriate Redis field types (NUMERIC for integers, etc.) to improve query performance.
426-434: Fix incorrect fallback for numeric comparison operators.When numeric parsing fails, the code falls back to string equality comparison even for inequality operators like GreaterThan/LessThan, which is incorrect. Numeric ordering operations should return false when values can't be parsed as numbers.
773-787: Close method should delegate to DeleteNamespace.The
Closemethod only drops the FT index but doesn't clean up the associated data, unlikeDeleteNamespace. For consistency and complete cleanup,Closeshould delegate toDeleteNamespaceor clearly document why it only performs partial cleanup.
🧹 Nitpick comments (1)
framework/vectorstore/redis.go (1)
25-27: Consider consolidating pool configuration fields.Both
PoolSizeandMaxActiveConnsfields serve similar purposes in connection pool management. According to the Redis client documentation,PoolSizeis the primary field for controlling pool size, whileMaxActiveConnsprovides additional limiting. Consider documenting the distinction or consolidating if they serve overlapping purposes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
framework/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
framework/go.mod(2 hunks)framework/vectorstore/redis.go(1 hunks)framework/vectorstore/redis_test.go(1 hunks)framework/vectorstore/store.go(3 hunks)framework/vectorstore/test_helpers..go(1 hunks)framework/vectorstore/weaviate_test.go(0 hunks)plugins/semanticcache/search.go(3 hunks)plugins/semanticcache/test_helpers.go(2 hunks)
💤 Files with no reviewable changes (1)
- framework/vectorstore/weaviate_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- framework/go.mod
- framework/vectorstore/store.go
🧰 Additional context used
🧬 Code graph analysis (4)
plugins/semanticcache/search.go (1)
plugins/semanticcache/main.go (1)
Plugin(128-134)
framework/vectorstore/redis.go (2)
core/schemas/logger.go (1)
Logger(28-55)framework/vectorstore/store.go (16)
VectorStoreProperties(63-66)SearchResult(43-47)Query(20-24)QueryOperatorIsNull(38-38)QueryOperatorEqual(29-29)QueryOperatorNotEqual(30-30)QueryOperatorLike(35-35)QueryOperatorIsNotNull(39-39)QueryOperator(26-26)QueryOperatorGreaterThan(31-31)QueryOperatorGreaterThanOrEqual(33-33)QueryOperatorLessThan(32-32)QueryOperatorLessThanOrEqual(34-34)DeleteResult(50-54)DeleteStatusError(60-60)DeleteStatusSuccess(59-59)
plugins/semanticcache/test_helpers.go (2)
framework/vectorstore/redis.go (1)
RedisConfig(17-38)framework/vectorstore/store.go (2)
VectorStoreTypeRedis(16-16)Config(92-96)
framework/vectorstore/redis_test.go (4)
framework/vectorstore/redis.go (2)
RedisStore(41-45)RedisConfig(17-38)core/schemas/logger.go (2)
Logger(28-55)LogLevelInfo(11-11)framework/vectorstore/store.go (14)
Config(92-96)VectorStoreProperties(63-66)VectorStorePropertyTypeString(71-71)VectorStorePropertyTypeInteger(72-72)VectorStorePropertyTypeBoolean(73-73)Query(20-24)QueryOperatorEqual(29-29)QueryOperatorGreaterThan(31-31)QueryOperatorLike(35-35)QueryOperatorLessThan(32-32)QueryOperatorGreaterThanOrEqual(33-33)VectorStore(78-89)VectorStoreTypeRedis(16-16)NewVectorStore(138-168)core/logger.go (1)
NewDefaultLogger(40-49)
🔇 Additional comments (11)
plugins/semanticcache/search.go (3)
205-210: LGTM! Improved stream_chunks parsing logic.The refactored parsing logic using the new
parseStreamChunkshelper method improves robustness by handling different data formats and properly validates the parsed result before determining streaming validity.
272-276: Good error handling improvement.The error message now clearly indicates that parsing failed rather than using the generic type assertion error, which makes debugging easier.
346-378: Well-designed helper function for stream data normalization.The
parseStreamChunksfunction effectively handles the different formats that stream data can arrive in from various vector store backends (Redis JSON strings, native arrays, etc.). The type switch covers all expected scenarios and provides clear error messages.framework/vectorstore/test_helpers..go (1)
1-47: Well-structured test helper utilities.The helper functions provide clean environment variable handling and embedding generation utilities that are essential for testing vector store functionality. The functions follow Go conventions with unexported names and proper error handling.
plugins/semanticcache/test_helpers.go (2)
45-88: Clean Redis configuration helper.The
getRedisConfigFromEnvfunction properly handles environment variable parsing with sensible defaults and error handling for invalid values. The timeout parsing usingtime.ParseDurationis particularly well done.
148-151: Proper migration to Redis backend.The switch from Weaviate to Redis in the test setup is correctly implemented, using the new Redis configuration helper and the appropriate vector store type constant.
framework/vectorstore/redis_test.go (3)
16-23: Good test constants definition.The constants provide clear default values and proper timeout settings for Redis testing. The dimension constant aligns with common embedding model outputs.
365-378: Fixed array indexing issue.The embedding array now properly matches the number of keys, preventing the out-of-range panic that was previously possible. Good fix!
578-580: Improved score assertion reliability.Only checking the top result's score for exact match (~1.0) is much more reliable than asserting all results have perfect scores, which was prone to flakiness.
framework/vectorstore/redis.go (2)
76-79: Good configurable embedding dimension.The implementation now properly uses the configured dimension from
RedisConfigwith a sensible fallback to 1536. This addresses the previous hard-coded dimension issue and supports different embedding model dimensions.
963-977: Redis client configuration looks correct.Based on the latest go-redis documentation, both
PoolSizeandMaxActiveConnsare valid fields inredis.Options. The configuration properly maps all the Redis config fields to the client options.
TejasGhatte
force-pushed
the
09-02-feat_redis_as_vector_store
branch
2 times, most recently
from
7644ea4 to
6f0396e
Compare
TejasGhatte
force-pushed
the
09-02-feat_redis_as_vector_store
branch
from
6f0396e to
6ce25c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
docs/architecture/framework/vector-store/redis.mdx (2)
273-287: Connection pool configuration example exceeds Redis client field limits.The Redis client's
redis.Optionsstruct doesn't have aMaxActiveConnsfield based on the code implementation. OnlyPoolSizeis used.config := vectorstore.RedisConfig{ Addr: "localhost:6379", PoolSize: 20, // Max socket connections - MaxActiveConns: 20, // Max active connections MinIdleConns: 5, // Min idle connections MaxIdleConns: 10, // Max idle connections ConnMaxLifetime: 30 * time.Minute, // Connection lifetime ConnMaxIdleTime: 5 * time.Minute, // Idle connection timeout DialTimeout: 5 * time.Second, // Connection timeout ReadTimeout: 3 * time.Second, // Read timeout WriteTimeout: 3 * time.Second, // Write timeout ContextTimeout: 10 * time.Second, // Operation timeout }
295-308: Remove MaxActiveConns from JSON configuration examples.The JSON configuration example still references
max_active_connswhich isn't mapped correctly in the Redis client.{ "vector_store": { "config": { "pool_size": 50, // Increase for high concurrency - "max_active_conns": 50, // Match pool_size "min_idle_conns": 10, // Keep connections warm "max_idle_conns": 20, // Allow some idle connections "conn_max_lifetime": "1h", // Refresh connections periodically "conn_max_idle_time": "10m" // Close idle connections } } }framework/vectorstore/redis.go (3)
781-785: LGTM! Close method properly delegates to DeleteNamespace.The Close method now correctly delegates to DeleteNamespace, ensuring both index and data cleanup as previously suggested.
17-38: Remove MaxActiveConns field or document it clearly.The
MaxActiveConnsfield inRedisConfigdoesn't map to any field inredis.Options. The Redis Go client only usesPoolSizefor connection pooling.You have two options:
Option 1: Remove the field entirely (recommended)
type RedisConfig struct { // Connection settings Addr string `json:"addr"` // Redis server address (host:port) - REQUIRED Username string `json:"username,omitempty"` // Username for Redis AUTH (optional) Password string `json:"password,omitempty"` // Password for Redis AUTH (optional) DB int `json:"db,omitempty"` // Redis database number (default: 0) // Connection pool and timeout settings (passed directly to Redis client) PoolSize int `json:"pool_size,omitempty"` // Maximum number of socket connections (optional) - MaxActiveConns int `json:"max_active_conns,omitempty"` // Maximum number of active connections (optional) MinIdleConns int `json:"min_idle_conns,omitempty"` // Minimum number of idle connections (optional) MaxIdleConns int `json:"max_idle_conns,omitempty"` // Maximum number of idle connections (optional)Option 2: Map both fields to PoolSize for backward compatibility
If you need to keep the field for backward compatibility, update the comment to clarify it's an alias for PoolSize.
961-975: Remove MaxActiveConns from Redis client options.The
redis.Optionsstruct doesn't have aMaxActiveConnsfield. This will cause a compilation error.client := redis.NewClient(&redis.Options{ Addr: config.Addr, Username: config.Username, Password: config.Password, DB: config.DB, PoolSize: config.PoolSize, - MaxActiveConns: config.MaxActiveConns, MinIdleConns: config.MinIdleConns, MaxIdleConns: config.MaxIdleConns, ConnMaxLifetime: config.ConnMaxLifetime, ConnMaxIdleTime: config.ConnMaxIdleTime, DialTimeout: config.DialTimeout, ReadTimeout: config.ReadTimeout, WriteTimeout: config.WriteTimeout, })framework/vectorstore/redis_test.go (4)
16-22: LGTM! Test constants are well-defined.The test constants including
RedisTestDimensionare properly defined, addressing the previous compilation issue.
143-148: LGTM! Proper test skipping when RediSearch is unavailable.The test now correctly uses
t.Skipfwhen namespace creation fails, avoiding cascading failures.
358-375: LGTM! Batch retrieval test properly handles embedding array.The test correctly creates an embeddings array with the same length as keys (including a nil entry for metadata-only items), fixing the out-of-range panic issue.
563-574: LGTM! Vector search score assertions are appropriately relaxed.The test now only asserts that the top result has a score close to 1.0, which is more realistic for similarity search results.
🧹 Nitpick comments (4)
docs/architecture/framework/vector-store/redis.mdx (1)
38-40: Consider using Go type aliases for clarity.Instead of importing both packages with the same name, consider using type aliases or renaming one of the imports for clarity.
-import ( - "context" - "github.com/maximhq/bifrost/framework/vectorstore" - "github.com/maximhq/bifrost/core/schemas" -) +import ( + "context" + bifrostVectorStore "github.com/maximhq/bifrost/framework/vectorstore" + "github.com/maximhq/bifrost/core/schemas" +)framework/vectorstore/redis.go (2)
417-430: Numeric comparison implementation has incomplete operator support.The
compareNumbersfunction is called for numeric operators but doesn't handleQueryOperatorContainsAnyandQueryOperatorContainsAllthat are defined in the interface. Consider documenting which operators are supported or implementing all of them.Add a comment documenting supported operators:
// matchesQueries checks if the properties match the given queries +// Supported operators: Equal, NotEqual, Like, IsNull, IsNotNull, +// GreaterThan, GreaterThanOrEqual, LessThan, LessThanOrEqual +// Unsupported operators will fall back to string equality comparison func (s *RedisStore) matchesQueries(properties map[string]interface{}, queries []Query) bool {
433-472: Consider simplifying numeric comparison logic.The
compareNumbersfunction has nested parsing with redundant comparison logic. The float parsing already handles integers, so the integer parsing fallback may be unnecessary.func (s *RedisStore) compareNumbers(propStr, queryStr string, operator QueryOperator) bool { // Try to parse as float64 first (handles both integers and floats) propVal, err1 := strconv.ParseFloat(propStr, 64) queryVal, err2 := strconv.ParseFloat(queryStr, 64) if err1 != nil || err2 != nil { - // If parsing as float fails, try as int64 - propInt, err1 := strconv.ParseInt(propStr, 10, 64) - queryInt, err2 := strconv.ParseInt(queryStr, 10, 64) - if err1 != nil || err2 != nil { - // If both fail, fall back to string comparison - return propStr == queryStr - } - // Use integer comparison - switch operator { - case QueryOperatorGreaterThan: - return propInt > queryInt - case QueryOperatorGreaterThanOrEqual: - return propInt >= queryInt - case QueryOperatorLessThan: - return propInt < queryInt - case QueryOperatorLessThanOrEqual: - return propInt <= queryInt - default: - return propInt == queryInt - } + // If numeric parsing fails, return false for numeric operators + return false } // Use float comparison switch operator {framework/vectorstore/redis_test.go (1)
830-836: Consider adding a dimension validation test.The test creates a RedisConfig with Dimension but doesn't verify that the dimension is properly used when creating the namespace/index.
Add a test to verify dimension handling:
t.Run("Dimension configuration", func(t *testing.T) { // Create a store with a specific dimension customConfig := RedisConfig{ Addr: getEnvWithDefault("REDIS_ADDR", DefaultTestAddr), Dimension: 768, // Different from default } customStore, err := newRedisStore(context.Background(), customConfig, logger) require.NoError(t, err) defer customStore.Close(context.Background(), "TestDimension") // Create namespace and verify it accepts embeddings of the configured dimension err = customStore.CreateNamespace(context.Background(), "TestDimension", map[string]VectorStoreProperties{}) require.NoError(t, err) // Add with correct dimension should succeed embedding768 := generateTestEmbedding(768) err = customStore.Add(context.Background(), "TestDimension", "test1", embedding768, nil) assert.NoError(t, err) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
framework/go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
docs/architecture/framework/vector-store/redis.mdx(1 hunks)framework/go.mod(2 hunks)framework/vectorstore/redis.go(1 hunks)framework/vectorstore/redis_test.go(1 hunks)framework/vectorstore/store.go(3 hunks)framework/vectorstore/test_helpers..go(1 hunks)framework/vectorstore/weaviate_test.go(0 hunks)plugins/semanticcache/search.go(3 hunks)plugins/semanticcache/test_helpers.go(2 hunks)
💤 Files with no reviewable changes (1)
- framework/vectorstore/weaviate_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- plugins/semanticcache/search.go
- framework/go.mod
- plugins/semanticcache/test_helpers.go
- framework/vectorstore/test_helpers..go
🧰 Additional context used
🧬 Code graph analysis (3)
framework/vectorstore/store.go (1)
framework/vectorstore/redis.go (1)
RedisConfig(17-38)
framework/vectorstore/redis_test.go (4)
framework/vectorstore/redis.go (2)
RedisStore(41-45)RedisConfig(17-38)core/schemas/logger.go (2)
Logger(28-55)LogLevelInfo(11-11)framework/vectorstore/store.go (14)
Config(92-96)VectorStoreProperties(63-66)VectorStorePropertyTypeString(71-71)VectorStorePropertyTypeInteger(72-72)VectorStorePropertyTypeBoolean(73-73)Query(20-24)QueryOperatorEqual(29-29)QueryOperatorGreaterThan(31-31)QueryOperatorLike(35-35)QueryOperatorLessThan(32-32)QueryOperatorGreaterThanOrEqual(33-33)VectorStore(78-89)VectorStoreTypeRedis(16-16)NewVectorStore(138-168)core/logger.go (1)
NewDefaultLogger(40-49)
framework/vectorstore/redis.go (3)
core/schemas/logger.go (1)
Logger(28-55)framework/vectorstore/store.go (17)
VectorStoreProperties(63-66)VectorStorePropertyTypeInteger(72-72)SearchResult(43-47)Query(20-24)QueryOperatorIsNull(38-38)QueryOperatorEqual(29-29)QueryOperatorNotEqual(30-30)QueryOperatorLike(35-35)QueryOperatorIsNotNull(39-39)QueryOperatorGreaterThan(31-31)QueryOperatorGreaterThanOrEqual(33-33)QueryOperatorLessThan(32-32)QueryOperatorLessThanOrEqual(34-34)QueryOperator(26-26)DeleteResult(50-54)DeleteStatusError(60-60)DeleteStatusSuccess(59-59)plugins/semanticcache/main.go (1)
VectorStoreProperties(150-187)
🔇 Additional comments (5)
framework/vectorstore/store.go (3)
16-16: LGTM! Clean integration of Redis as a new vector store type.The addition of
VectorStoreTypeRedisconstant follows the existing pattern and properly extends the vector store types enum.
124-129: LGTM! Redis config unmarshaling follows the established pattern.The Redis configuration unmarshaling is correctly implemented with proper error handling, consistent with the existing Weaviate pattern.
157-165: LGTM! Redis store factory integration is properly implemented.The Redis store creation follows the same validation and error handling pattern as Weaviate, maintaining consistency in the codebase.
framework/vectorstore/redis.go (2)
56-112: LGTM! Namespace creation with proper schema handling.The implementation correctly handles both string and integer field types when creating the Redis FT index, with sensible defaults for the embedding dimension.
590-614: TTL handling implementation looks good.The TTL implementation correctly extracts
expires_atfrom metadata and sets Redis expiration time. Good defensive programming with type checking.
|
Lets get |
TejasGhatte
force-pushed
the
09-02-feat_redis_as_vector_store
branch
from
6ce25c3 to
a85a794
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
plugins/semanticcache/main.go (4)
34-34: Expose VectorStoreNamespace but validate/sanitize itAdd lightweight validation (allowed chars) to avoid backend-specific name errors (e.g., Redis index/prefix constraints) and document accepted format.
You can validate during Init:
@@ type Config struct { @@ - VectorStoreNamespace string `json:"vector_store_namespace,omitempty"` // Namespace for vector store (optional) + VectorStoreNamespace string `json:"vector_store_namespace,omitempty"` // Namespace for vector store (optional; [A-Za-z0-9:_-])And in Init (see lines 260-262 comment below) perform a regex check before use.
53-53: Support a JSON alias 'namespace' for ergonomicsMany users expect "namespace" in config. Accept an alias to reduce friction.
type TempConfig struct { @@ - VectorStoreNamespace string `json:"vector_store_namespace,omitempty"` + VectorStoreNamespace string `json:"vector_store_namespace,omitempty"` + Namespace string `json:"namespace,omitempty"`
71-71: Prefer alias fallback when vector_store_namespace is unsetIf only "namespace" is provided, use it.
- c.VectorStoreNamespace = temp.VectorStoreNamespace + if temp.VectorStoreNamespace != "" { + c.VectorStoreNamespace = temp.VectorStoreNamespace + } else if temp.Namespace != "" { + c.VectorStoreNamespace = temp.Namespace + }
260-262: Trim and default namespace; optionally enforce formatSmall hardening to avoid accidental spaces and invalid names.
@@ func Init(ctx context.Context, config Config, logger schemas.Logger, store vectorstore.VectorStore) (schemas.Plugin, error) { - if config.VectorStoreNamespace == "" { - config.VectorStoreNamespace = DefaultVectorStoreNamespace - } + // Normalize before defaulting + config.VectorStoreNamespace = strings.TrimSpace(config.VectorStoreNamespace) + if config.VectorStoreNamespace == "" { + config.VectorStoreNamespace = DefaultVectorStoreNamespace + } + // Optional: enforce allowed chars + // if ok := regexp.MustCompile(`^[A-Za-z0-9:_-]+$`).MatchString(config.VectorStoreNamespace); !ok { + // return nil, fmt.Errorf("invalid vector_store_namespace: %q (allowed: [A-Za-z0-9:_-])", config.VectorStoreNamespace) + // }Add imports:
@@ -import ( +import ( "context" "encoding/json" "fmt" "strconv" "sync" "time" + "strings" + // "regexp" // if you enable format enforcement )framework/vectorstore/store.go (2)
124-129: Minor: normalize pointer vs value semantics across backendsHere
c.Configstores a valueRedisConfig, while Weaviate’s path later passes a pointer. Consider standardizing (either both values or both pointers) to reduce surprises and copies. Not urgent.
157-166: Make factory robust to programmatic (map) configsIf
Configis constructed in code (not via JSON),config.Configmay be amap[string]anyor*RedisConfig. Add a small normalization to support these without forcing callers to pre-cast.Apply:
case VectorStoreTypeRedis: if config.Config == nil { return nil, fmt.Errorf("redis config is required") } - redisConfig, ok := config.Config.(RedisConfig) - if !ok { - return nil, fmt.Errorf("invalid redis config") - } - return newRedisStore(ctx, redisConfig, logger) + switch v := config.Config.(type) { + case RedisConfig: + return newRedisStore(ctx, v, logger) + case *RedisConfig: + return newRedisStore(ctx, *v, logger) + case map[string]any, json.RawMessage: + b, _ := json.Marshal(v) + var rc RedisConfig + if err := json.Unmarshal(b, &rc); err != nil { + return nil, fmt.Errorf("invalid redis config: %w", err) + } + return newRedisStore(ctx, rc, logger) + default: + return nil, fmt.Errorf("invalid redis config") + }framework/vectorstore/redis_test.go (6)
34-34: Mark helper for clearer failure locationsAdd t.Helper() to NewRedisTestSetup.
func NewRedisTestSetup(t *testing.T) *RedisTestSetup { + t.Helper()
151-168: Cleanup should page through all results (avoid 1000-item cap)If >1000 objects accumulate, current cleanup leaves leftovers. Iterate with cursor.
func (ts *RedisTestSetup) cleanupTestData(t *testing.T) { - // Delete all objects in the test namespace - allTestKeys, _, err := ts.Store.GetAll(ts.ctx, TestNamespace, []Query{}, []string{}, nil, 1000) - if err != nil { - t.Logf("Warning: Failed to get all test keys: %v", err) - return - } - - for _, key := range allTestKeys { - err := ts.Store.Delete(ts.ctx, TestNamespace, key.ID) - if err != nil { - t.Logf("Warning: Failed to delete test key %s: %v", key.ID, err) - } - } + var cursor *string + for { + page, next, err := ts.Store.GetAll(ts.ctx, TestNamespace, nil, nil, cursor, 500) + if err != nil { + t.Logf("Warning: Failed to get test keys: %v", err) + break + } + for _, key := range page { + if err := ts.Store.Delete(ts.ctx, TestNamespace, key.ID); err != nil { + t.Logf("Warning: Failed to delete test key %s: %v", key.ID, err) + } + } + if next == nil { + break + } + cursor = next + }
385-388: Avoid assuming GetChunks result orderUnless the contract guarantees order, assert by ID.
- // Verify each result - for i, result := range results { - assert.Equal(t, keys[i], result.ID) - assert.Equal(t, metadata[i]["type"], result.Properties["type"]) - } + // Verify by ID (order-agnostic) + resByID := make(map[string]SearchResult, len(results)) + for _, r := range results { + resByID[r.ID] = r + } + for i, key := range keys { + r, ok := resByID[key] + require.True(t, ok, "missing result for key %s", key) + assert.Equal(t, metadata[i]["type"], r.Properties["type"]) + }
329-331: Reduce flakiness: replace fixed sleeps with EventuallyIndexing can be async; poll instead of sleeping.
require.Eventually(t, func() bool { _, err := setup.Store.GetChunk(setup.ctx, TestNamespace, testKey) return err == nil }, 5*time.Second, 50*time.Millisecond)
571-574: Assert top hit identity; relax score deltaValidates correctness without overfitting to exact similarity value.
require.NotEmpty(t, results) require.NotNil(t, results[0].Score) - assert.InDelta(t, 1.0, *results[0].Score, 1e-4) + assert.Equal(t, testDocs[0].key, results[0].ID) + assert.InDelta(t, 1.0, *results[0].Score, 1e-3)
596-600: Add minimal assertions for high-threshold searchEnsure at least the self-match returns.
results, err := setup.Store.GetNearest(setup.ctx, TestNamespace, queryEmbedding, nil, []string{"type", "category", "content"}, 0.99, 10) require.NoError(t, err) - // Should return fewer results due to high threshold - t.Logf("High threshold search returned %d results", len(results)) + require.NotEmpty(t, results) + assert.Equal(t, testDocs[0].key, results[0].ID) + t.Logf("High threshold search returned %d results", len(results))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
framework/go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
docs/architecture/framework/vector-store/redis.mdx(1 hunks)framework/go.mod(2 hunks)framework/vectorstore/redis.go(1 hunks)framework/vectorstore/redis_test.go(1 hunks)framework/vectorstore/store.go(3 hunks)framework/vectorstore/test_helpers..go(1 hunks)framework/vectorstore/weaviate_test.go(0 hunks)plugins/semanticcache/main.go(10 hunks)plugins/semanticcache/search.go(6 hunks)plugins/semanticcache/stream.go(1 hunks)plugins/semanticcache/test_helpers.go(2 hunks)plugins/semanticcache/utils.go(1 hunks)
💤 Files with no reviewable changes (1)
- framework/vectorstore/weaviate_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- framework/go.mod
- docs/architecture/framework/vector-store/redis.mdx
- plugins/semanticcache/test_helpers.go
- framework/vectorstore/redis.go
- plugins/semanticcache/search.go
- framework/vectorstore/test_helpers..go
🧰 Additional context used
🧬 Code graph analysis (3)
framework/vectorstore/store.go (1)
framework/vectorstore/redis.go (1)
RedisConfig(17-38)
plugins/semanticcache/main.go (1)
framework/vectorstore/store.go (1)
VectorStoreProperties(63-66)
framework/vectorstore/redis_test.go (4)
framework/vectorstore/redis.go (2)
RedisStore(41-45)RedisConfig(17-38)core/schemas/logger.go (2)
Logger(28-55)LogLevelInfo(11-11)framework/vectorstore/store.go (6)
Config(92-96)VectorStoreProperties(63-66)Query(20-24)VectorStore(78-89)VectorStoreTypeRedis(16-16)NewVectorStore(138-168)core/logger.go (1)
NewDefaultLogger(40-49)
🔇 Additional comments (8)
plugins/semanticcache/utils.go (1)
308-308: Switching to configurable namespace is correctUsing plugin.config.VectorStoreNamespace here aligns storage with the new config-driven namespace. No action needed.
plugins/semanticcache/stream.go (1)
123-123: Consistent namespace usage for streaming final AddGood alignment with the configurable namespace on the final store.Add call.
plugins/semanticcache/main.go (3)
307-307: Ensure CreateNamespace is idempotentIf the namespace already exists, this should not fail Init. Confirm RedisStore.CreateNamespace returns a typed ErrNamespaceExists (or equivalent) that you can ignore; otherwise, make the call idempotent.
Would you like me to wire a guard to ignore a known “already exists” error once you confirm the error type?
616-616: Cleanup DeleteAll scoped by plugin flag looks goodFiltering by from_bifrost_semantic_cache_plugin avoids cross-tenant deletion. LGTM.
34-34: VectorStoreClassName removal verified
Ran searches forVectorStoreClassName|vector_store_class_name|vectorStoreClassNameand found no matches;VectorStoreNamespaceis consistently referenced in theplugins/semanticcacheplugin.framework/vectorstore/store.go (1)
16-16: Redis store type wiring looks goodConstant addition is straightforward and consistent with existing types.
framework/vectorstore/redis_test.go (2)
827-849: Factory smoke test looks goodCovers the type path and Close. Fine as-is.
868-871: Verify Delete semantics for non-existent keys across stores
- Check that
RedisStore.Delete(framework/vectorstore/redis.go) andWeaviateStore.Delete(framework/vectorstore/weaviate.go) both return an error when the target ID doesn’t exist.- Align implementations or tests so callers see the same behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/semanticcache/stream.go (1)
89-95: Fix sort comparator: nil chunks are currently moved to the front (breaks ordering).The comparator returns true when i.Response is nil and j.Response is non‑nil, pushing nils to the front, contrary to the comment and expected behavior.
Apply this diff to push nils to the end deterministically:
- sort.SliceStable(accumulator.Chunks, func(i, j int) bool { - if accumulator.Chunks[i].Response == nil || accumulator.Chunks[j].Response == nil { - // Push nils to the end deterministically - return accumulator.Chunks[j].Response != nil - } - return accumulator.Chunks[i].Response.ExtraFields.ChunkIndex < accumulator.Chunks[j].Response.ExtraFields.ChunkIndex - }) + sort.SliceStable(accumulator.Chunks, func(i, j int) bool { + ri := accumulator.Chunks[i].Response + rj := accumulator.Chunks[j].Response + // Push nils to the end deterministically + if ri == nil && rj == nil { + return false + } + if ri == nil { + return false + } + if rj == nil { + return true + } + return ri.ExtraFields.ChunkIndex < rj.ExtraFields.ChunkIndex + })plugins/semanticcache/test_utils.go (1)
185-212: Function name vs. implementation mismatch (Weaviate used in “RedisCluster” setup).NewRedisClusterTestSetup initializes a Weaviate store, not Redis. Either rename the function or switch to VectorStoreTypeRedis and use getRedisConfigFromEnv().
Apply:
- store, err := vectorstore.NewVectorStore(ctx, &vectorstore.Config{ - Type: vectorstore.VectorStoreTypeWeaviate, - Config: getWeaviateConfigFromEnv(), - }, logger) + store, err := vectorstore.NewVectorStore(ctx, &vectorstore.Config{ + Type: vectorstore.VectorStoreTypeRedis, + Config: getRedisConfigFromEnv(), + }, logger)
♻️ Duplicate comments (1)
framework/vectorstore/redis.go (1)
872-881: Remove MaxActiveConns from redis.Options; map legacy MaxActiveConns → PoolSizego-redis/v9's redis.Options has no MaxActiveConns field. Replace the Options entry with a compatibility mapping that prefers config.PoolSize and falls back to config.MaxActiveConns, and update docs/examples.
- client := redis.NewClient(&redis.Options{ + // Optional compatibility mapping: prefer PoolSize, fall back to MaxActiveConns for legacy config + poolSize := config.PoolSize + if poolSize == 0 && config.MaxActiveConns > 0 { + poolSize = config.MaxActiveConns + } + client := redis.NewClient(&redis.Options{ Addr: config.Addr, Username: config.Username, Password: config.Password, DB: config.DB, Protocol: 3, // Explicitly use RESP3 protocol - PoolSize: config.PoolSize, - MaxActiveConns: config.MaxActiveConns, + PoolSize: poolSize, MinIdleConns: config.MinIdleConns, MaxIdleConns: config.MaxIdleConns, ConnMaxLifetime: config.ConnMaxLifetime, ConnMaxIdleTime: config.ConnMaxIdleTime, DialTimeout: config.DialTimeout, ReadTimeout: config.ReadTimeout, WriteTimeout: config.WriteTimeout, })Files to update: framework/vectorstore/redis.go (options literal around lines ~871–874; config struct field remains at ~line 31 if you want backward compatibility) and docs/architecture/framework/vector-store.mdx (remove or deprecate example uses of MaxActiveConns).
🧹 Nitpick comments (10)
plugins/semanticcache/stream.go (1)
115-121: Config‑driven namespace is good; guard empty, propagate expiry, and log the namespace.Validate namespace isn’t empty, include TTL/expiry in metadata (for stores without native TTL), and log ns for traceability. Also confirm Add is upsert‑safe for repeated requestIDs.
Apply this diff:
finalMetadata["stream_chunks"] = streamResponses + // Carry expiry info for stores that don't support native TTL + if accumulator.TTL > 0 { + finalMetadata["expires_at"] = time.Now().Add(accumulator.TTL).UTC().Format(time.RFC3339) + } // Store complete unified entry using original requestID - this is the final .Add() call - if err := plugin.store.Add(ctx, plugin.config.VectorStoreNamespace, requestID, accumulator.Embedding, finalMetadata); err != nil { + ns := plugin.config.VectorStoreNamespace + if ns == "" { + return fmt.Errorf("VectorStoreNamespace cannot be empty") + } + if err := plugin.store.Add(ctx, ns, requestID, accumulator.Embedding, finalMetadata); err != nil { return fmt.Errorf("failed to store complete streaming cache entry: %w", err) } - plugin.logger.Debug(fmt.Sprintf("%s Successfully cached complete stream with %d ordered chunks, ID: %s", PluginLoggerPrefix, len(streamResponses), requestID)) + plugin.logger.Debug(fmt.Sprintf("%s Successfully cached complete stream with %d ordered chunks, ns=%s, ID=%s", PluginLoggerPrefix, len(streamResponses), ns, requestID))Also applies to: 123-123, 127-127
plugins/semanticcache/test_utils.go (2)
45-88: Unused helper: consider moving or using getRedisConfigFromEnv.This function isn’t referenced in this package. Either wire it into Redis-backed tests or move it to framework/vectorstore/test_utils.go to avoid dead code.
314-325: Duplicate success log.“Response correctly served from cache” is logged twice. Remove one to reduce noise.
Apply:
- t.Log("✅ Response correctly served from cache") - } - - t.Log("✅ Response correctly served from cache") + t.Log("✅ Response correctly served from cache") + }framework/vectorstore/test_utils.go (3)
19-24: Simplify API: unnecessary error return.getEnvWithDefaultInt never returns a non-nil error on the default path. Consider returning just int and documenting that invalid values fall back to default.
30-36: Determinism for tests.generateTestEmbedding uses math/rand without a seed; results vary run-to-run. For stable tests, accept a *rand.Rand or seed locally with a fixed source.
Apply (one option):
-func generateTestEmbedding(dim int) []float32 { - embedding := make([]float32, dim) - for i := range embedding { - embedding[i] = rand.Float32()*2 - 1 - } - return embedding -} +func generateTestEmbedding(dim int) []float32 { + r := rand.New(rand.NewSource(1)) + embedding := make([]float32, dim) + for i := range embedding { + embedding[i] = r.Float32()*2 - 1 + } + return embedding +}
38-46: Clarify parameter semantics.Add a brief comment that similarity is in [0,1], where 1 means nearly identical (minimal noise) and 0 allows maximal noise.
framework/vectorstore/redis.go (4)
45-50: Add Redis Cluster support with UniversalClient.Using *redis.Client precludes Cluster. Switch to redis.UniversalClient and instantiate ClusterClient when multiple addresses are provided.
Apply:
type RedisStore struct { - client *redis.Client + client redis.UniversalClient config RedisConfig logger schemas.Logger } @@ -func newRedisStore(ctx context.Context, config RedisConfig, logger schemas.Logger) (*RedisStore, error) { +func newRedisStore(ctx context.Context, config RedisConfig, logger schemas.Logger) (*RedisStore, error) { if config.Addr == "" { return nil, fmt.Errorf("redis addr is required") } - - client := redis.NewClient(&redis.Options{ - Addr: config.Addr, - Username: config.Username, - Password: config.Password, - DB: config.DB, - Protocol: 3, - PoolSize: poolSize, - MinIdleConns: config.MinIdleConns, - MaxIdleConns: config.MaxIdleConns, - ConnMaxLifetime: config.ConnMaxLifetime, - ConnMaxIdleTime: config.ConnMaxIdleTime, - DialTimeout: config.DialTimeout, - ReadTimeout: config.ReadTimeout, - WriteTimeout: config.WriteTimeout, - }) + addrs := strings.Split(config.Addr, ",") + var client redis.UniversalClient + if len(addrs) > 1 { + client = redis.NewClusterClient(&redis.ClusterOptions{ + Addrs: addrs, + Username: config.Username, + Password: config.Password, + Protocol: 3, + PoolSize: poolSize, + MinIdleConns: config.MinIdleConns, + MaxIdleConns: config.MaxIdleConns, + ConnMaxLifetime: config.ConnMaxLifetime, + ConnMaxIdleTime: config.ConnMaxIdleTime, + DialTimeout: config.DialTimeout, + ReadTimeout: config.ReadTimeout, + WriteTimeout: config.WriteTimeout, + }) + } else { + client = redis.NewClient(&redis.Options{ + Addr: config.Addr, + Username: config.Username, + Password: config.Password, + DB: config.DB, + Protocol: 3, + PoolSize: poolSize, + MinIdleConns: config.MinIdleConns, + MaxIdleConns: config.MaxIdleConns, + ConnMaxLifetime: config.ConnMaxLifetime, + ConnMaxIdleTime: config.ConnMaxIdleTime, + DialTimeout: config.DialTimeout, + ReadTimeout: config.ReadTimeout, + WriteTimeout: config.WriteTimeout, + }) + }Also applies to: 859-897
111-145: Avoid returning raw binary embedding in GetChunk(s).HGetAll returns the binary “embedding” field; propagating it into Properties bloats responses and logs. Skip it unless explicitly requested.
Apply (both methods):
- for k, v := range fields { - searchResult.Properties[k] = v - } + for k, v := range fields { + if k == "embedding" { // omit large binary blob + continue + } + searchResult.Properties[k] = v + }Also applies to: 147-208
287-373: RESP3-only parsing: acceptable with Protocol:3, add a note.parseSearchResults assumes RESP3 map shape; since you pin Protocol:3, that’s fine. Add a brief comment here referencing the client Protocol setting.
814-843: Document intentional comma→OR behavior in TAG escaping.Since commas are rewritten to “|” to match any comma-separated token, add a comment explaining this nonstandard equality semantics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
framework/go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
docs/architecture/framework/vector-store.mdx(1 hunks)docs/architecture/framework/vector-store/weaviate.mdx(0 hunks)docs/docs.json(1 hunks)framework/go.mod(2 hunks)framework/vectorstore/redis.go(1 hunks)framework/vectorstore/redis_test.go(1 hunks)framework/vectorstore/store.go(3 hunks)framework/vectorstore/test_utils.go(1 hunks)framework/vectorstore/utils.go(1 hunks)framework/vectorstore/weaviate_test.go(0 hunks)plugins/semanticcache/main.go(10 hunks)plugins/semanticcache/search.go(8 hunks)plugins/semanticcache/stream.go(1 hunks)plugins/semanticcache/test_utils.go(6 hunks)plugins/semanticcache/utils.go(1 hunks)
💤 Files with no reviewable changes (2)
- framework/vectorstore/weaviate_test.go
- docs/architecture/framework/vector-store/weaviate.mdx
🚧 Files skipped from review as they are similar to previous changes (9)
- plugins/semanticcache/utils.go
- framework/go.mod
- docs/docs.json
- framework/vectorstore/utils.go
- plugins/semanticcache/main.go
- docs/architecture/framework/vector-store.mdx
- framework/vectorstore/store.go
- plugins/semanticcache/search.go
- framework/vectorstore/redis_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
plugins/semanticcache/test_utils.go (1)
framework/vectorstore/redis.go (1)
RedisConfig(22-43)
framework/vectorstore/redis.go (2)
core/schemas/logger.go (1)
Logger(28-55)framework/vectorstore/store.go (18)
VectorStoreProperties(63-66)VectorStorePropertyTypeInteger(72-72)SearchResult(43-47)Query(20-24)QueryOperatorEqual(29-29)QueryOperatorNotEqual(30-30)QueryOperatorLike(35-35)QueryOperatorGreaterThan(31-31)QueryOperatorGreaterThanOrEqual(33-33)QueryOperatorLessThan(32-32)QueryOperatorLessThanOrEqual(34-34)QueryOperatorIsNull(38-38)QueryOperatorIsNotNull(39-39)QueryOperatorContainsAny(36-36)QueryOperatorContainsAll(37-37)DeleteResult(50-54)DeleteStatusError(60-60)DeleteStatusSuccess(59-59)
🔇 Additional comments (7)
plugins/semanticcache/test_utils.go (1)
129-141: Switch to text-embedding-3-small looks good.Lower cost model for tests is appropriate and should reduce flakiness due to rate limits.
Also applies to: 193-205, 396-408, 419-431, 442-455
framework/vectorstore/test_utils.go (1)
26-28: LGTM.UUID generation is straightforward and appropriate for test IDs.
framework/vectorstore/redis.go (5)
71-109: Index creation and RediSearch presence checks look solid.Dimension validation and FT.INFO unknown-command handling are good safeguards.
210-285: Good: LIMIT, RETURN handling, and cursor pagination.The LIMIT clause and offset-based cursor resolve earlier paging issues and make DeleteAll robust.
466-573: KNN flow and similarity conversion look correct.Including AS score, sorting, thresholding, and returning similarity via 1-distance reads well.
586-624: Metadata encoding is safe for arrays/objects.JSON-encoding for []interface{} avoids CSV pitfalls and preserves structure for round-trips.
755-812: Consistent cleanup via Close → DeleteNamespace.Good consolidation; this prevents orphaned data or indexes.
TejasGhatte
force-pushed
the
09-02-feat_redis_as_vector_store
branch
from
f8f10ca to
6237854
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
plugins/semanticcache/main.go (1)
264-271: Add debug log for dimension defaulting.The namespace defaulting includes a debug log, but the dimension defaulting is missing one. Consider adding a debug log for consistency.
if config.Dimension == 0 { - logger.Debug(PluginLoggerPrefix + " Dimension is not set, using default of 0") + logger.Debug(PluginLoggerPrefix + " Dimension is not set, will be determined by vector store") config.Dimension = 0 }Note: Setting dimension to 0 might not be the best default. Consider either requiring this field or defaulting to a common value like 1536 for OpenAI embeddings.
framework/vectorstore/redis.go (2)
22-40: Remove or consolidate MaxActiveConns field.The
MaxActiveConnsfield duplicates the functionality ofPoolSize. The Redis Go client usesPoolSizeto control the maximum number of connections.// Connection pool and timeout settings (passed directly to Redis client) PoolSize int `json:"pool_size,omitempty"` // Maximum number of socket connections (optional) - MaxActiveConns int `json:"max_active_conns,omitempty"` // Maximum number of active connections (optional) MinIdleConns int `json:"min_idle_conns,omitempty"` // Minimum number of idle connections (optional)Also remove the corresponding field from line 863 in the Redis client options.
830-830: Document the intentional comma-to-OR conversion.The conversion of commas to OR operators in TAG queries is intentional for supporting comma-separated values. Consider adding a comment to clarify this design choice.
"~", "\\~", "`", "\\`", "\"", "\\\"", "'", "\\'", " ", "\\ ", "-", "\\-", + // Intentionally convert commas to OR for "contains any" semantics on comma-separated values ",", "|",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
framework/go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
docs/architecture/framework/vector-store.mdx(1 hunks)docs/architecture/framework/vector-store/weaviate.mdx(0 hunks)docs/docs.json(1 hunks)docs/features/semantic-caching.mdx(1 hunks)framework/go.mod(2 hunks)framework/vectorstore/redis.go(1 hunks)framework/vectorstore/redis_test.go(1 hunks)framework/vectorstore/store.go(4 hunks)framework/vectorstore/test_utils.go(1 hunks)framework/vectorstore/utils.go(1 hunks)framework/vectorstore/weaviate.go(2 hunks)framework/vectorstore/weaviate_test.go(1 hunks)plugins/semanticcache/main.go(10 hunks)plugins/semanticcache/search.go(8 hunks)plugins/semanticcache/stream.go(1 hunks)plugins/semanticcache/test_utils.go(6 hunks)plugins/semanticcache/utils.go(1 hunks)
💤 Files with no reviewable changes (1)
- docs/architecture/framework/vector-store/weaviate.mdx
🚧 Files skipped from review as they are similar to previous changes (8)
- plugins/semanticcache/utils.go
- framework/vectorstore/test_utils.go
- framework/go.mod
- docs/architecture/framework/vector-store.mdx
- docs/docs.json
- plugins/semanticcache/stream.go
- framework/vectorstore/utils.go
- plugins/semanticcache/test_utils.go
🧰 Additional context used
🧬 Code graph analysis (7)
framework/vectorstore/weaviate.go (2)
framework/vectorstore/store.go (1)
VectorStoreProperties(63-66)plugins/semanticcache/main.go (1)
VectorStoreProperties(157-194)
framework/vectorstore/store.go (2)
plugins/semanticcache/main.go (1)
Config(25-42)framework/vectorstore/redis.go (1)
RedisConfig(22-40)
plugins/semanticcache/main.go (1)
framework/vectorstore/store.go (1)
VectorStoreProperties(63-66)
framework/vectorstore/redis_test.go (4)
framework/vectorstore/redis.go (2)
RedisStore(43-47)RedisConfig(22-40)core/schemas/logger.go (1)
Logger(28-55)framework/vectorstore/store.go (12)
Config(92-96)VectorStoreProperties(63-66)VectorStorePropertyTypeString(71-71)VectorStorePropertyTypeInteger(72-72)VectorStorePropertyTypeBoolean(73-73)Query(20-24)QueryOperatorGreaterThan(31-31)QueryOperatorEqual(29-29)QueryOperatorLessThan(32-32)VectorStore(78-89)VectorStoreTypeRedis(16-16)NewVectorStore(138-168)core/logger.go (1)
NewDefaultLogger(40-49)
plugins/semanticcache/search.go (1)
plugins/semanticcache/main.go (1)
Plugin(134-140)
framework/vectorstore/weaviate_test.go (2)
plugins/semanticcache/test_utils.go (1)
NewTestSetup(115-132)framework/vectorstore/store.go (2)
VectorStoreProperties(63-66)VectorStorePropertyTypeString(71-71)
framework/vectorstore/redis.go (3)
core/schemas/logger.go (1)
Logger(28-55)framework/vectorstore/store.go (18)
VectorStoreProperties(63-66)VectorStorePropertyTypeInteger(72-72)SearchResult(43-47)Query(20-24)QueryOperatorEqual(29-29)QueryOperatorNotEqual(30-30)QueryOperatorLike(35-35)QueryOperatorGreaterThan(31-31)QueryOperatorGreaterThanOrEqual(33-33)QueryOperatorLessThan(32-32)QueryOperatorLessThanOrEqual(34-34)QueryOperatorIsNull(38-38)QueryOperatorIsNotNull(39-39)QueryOperatorContainsAny(36-36)QueryOperatorContainsAll(37-37)DeleteResult(50-54)DeleteStatusError(60-60)DeleteStatusSuccess(59-59)plugins/semanticcache/main.go (1)
VectorStoreProperties(157-194)
🔇 Additional comments (33)
docs/features/semantic-caching.mdx (1)
114-114: LGTM!The addition of the
Dimensionfield to the semantic cache configuration is well-integrated and properly documented. The example value of 1536 is appropriate for the OpenAI text-embedding-3-small model mentioned in the configuration.framework/vectorstore/weaviate.go (1)
451-508: LGTM! Dimension-aware namespace creation.The implementation correctly handles the dimension parameter for vector indexing. When dimension > 0, it properly configures the VectorIndexConfig with the specified dimension, allowing Weaviate to optimize the vector index for the expected embedding size.
framework/vectorstore/store.go (4)
16-16: LGTM! Redis vector store type constant added.The new constant follows the existing naming convention and correctly identifies the Redis store type.
79-79: LGTM! Interface updated to support dimension-aware namespaces.The addition of the
dimensionparameter toCreateNamespaceis a necessary change that enables both Redis and Weaviate backends to properly configure their vector indices with the correct embedding dimension.
124-129: LGTM! Redis configuration unmarshalling added.The JSON unmarshalling logic correctly handles the Redis configuration case, following the same pattern as Weaviate.
157-165: LGTM! Redis store factory integration.The Redis store creation logic properly validates the configuration and delegates to
newRedisStore, maintaining consistency with the Weaviate pattern.framework/vectorstore/weaviate_test.go (1)
742-814: LGTM! Comprehensive dimension handling test.The test thoroughly validates that namespaces can be recreated with different embedding dimensions without crashes. It properly tests:
- Creating a namespace with dimension 512 and adding/verifying data
- Deleting the namespace
- Recreating with dimension 1024 and verifying vector search functionality
This ensures dimension changes are handled gracefully.
plugins/semanticcache/main.go (6)
30-30: LGTM! Dimension field added to config.The addition of the
Dimensionfield enables proper configuration of vector store namespaces with the expected embedding dimension.
33-35: LGTM! VectorStoreNamespace field added for namespace management.The addition of
VectorStoreNamespaceallows users to configure custom namespaces for their vector stores, providing better isolation and organization of cache data.
145-148: LGTM! Constants updated for namespace management.The replacement of
VectorStoreClassNamewithDefaultVectorStoreNamespaceand addition ofCreateNamespaceTimeoutprovide better defaults and timeout management for namespace operations.
316-320: LGTM! Namespace creation with proper timeout.The namespace creation correctly uses the configured namespace name, dimension, and a dedicated timeout for this potentially long-running operation.
627-641: LGTM! Proper namespace cleanup with timeout.The cleanup properly deletes all cache entries and the namespace itself, with appropriate timeout handling. The documentation clearly states that the namespace is temporary and should not be used for external data.
671-674: LGTM! Cache clearing methods use proper timeout.Both
ClearCacheForKeyandClearCacheForRequestIDcorrectly use timeouts and the configured namespace for their operations.plugins/semanticcache/search.go (6)
8-8: LGTM! Added strconv import for parsing.The import is necessary for the new string-to-int parsing logic for expires_at values.
62-62: LGTM! Namespace configuration properly applied.Both
performDirectSearchandperformSemanticSearchnow correctly use the configured namespace fromplugin.config.VectorStoreNamespaceinstead of a hardcoded value.Also applies to: 139-139
170-177: LGTM! Robust expires_at parsing.The code now handles expires_at values that might be stored as strings (from Redis TAG fields), properly parsing them to int64 before comparing with the current time.
195-195: LGTM! Delete operation uses configured namespace.The expired entry deletion correctly uses the configured namespace.
214-218: Stream chunks parsing handles Redis storage format.The new approach using
parseStreamChunksproperly handles the Redis TAG storage format where arrays might be stored as JSON strings.
355-387: LGTM! Comprehensive stream chunks parsing.The
parseStreamChunksfunction handles multiple formats gracefully:
- Direct
[]interface{}(from Weaviate)[]stringarrays- JSON-encoded string arrays (from Redis)
This ensures compatibility across different vector store backends.
framework/vectorstore/redis_test.go (4)
143-147: LGTM! Proper test skipping for missing RediSearch.The test correctly skips when RediSearch is unavailable rather than continuing with cascading failures.
293-297: LGTM! Fixed embedding array bounds issue.The test now correctly provides three embeddings (including a nil for metadata-only) matching the three keys, preventing index out of bounds errors.
504-506: LGTM! Relaxed score assertion for vector search.The test now correctly asserts only that the top result has a score close to 1.0, which is more robust than asserting all results have perfect scores.
820-890: LGTM! Comprehensive namespace dimension handling test.The test thoroughly validates that Redis namespaces can be recreated with different dimensions, properly testing both 512 and 1024-dimensional embeddings.
framework/vectorstore/redis.go (10)
18-20: LGTM! Well-defined batch limit constant.The constant provides a reasonable default for batch operations and pagination.
58-60: LGTM! Good error handling for missing RediSearch.The code provides a clear, actionable error message when RediSearch is not available.
71-73: LGTM! Proper dimension validation.The code validates that dimension > 0 before creating the index, preventing FT.CREATE failures.
91-97: LGTM! Proper field type detection.The code correctly maps VectorStorePropertyTypeInteger to NUMERIC fields in Redis for proper numeric comparisons, while defaulting to TAG for text fields.
233-237: LGTM! Proper handling of limit=0 for "get all".The code correctly interprets limit=0 as "get all results" using MaxInt32, and applies proper limits in both GetAll and GetNearest methods.
Also applies to: 511-515
285-368: LGTM! RESP3 protocol parsing.The parseSearchResults function correctly handles RESP3 map format from FT.SEARCH. The code explicitly sets Protocol: 3 in the client options (line 861) ensuring consistent response format.
597-602: LGTM! Arrays preserved as JSON.The code correctly preserves array structures by JSON-encoding them, which is essential for round-trip compatibility with fields like
stream_chunks.
646-743: LGTM! Efficient cursor-based batch deletion.The DeleteAll implementation efficiently handles large datasets using cursor-based pagination and batch processing with pipelines.
798-801: LGTM! Close delegates to DeleteNamespace.The Close method now properly delegates to DeleteNamespace, ensuring consistent cleanup of both index and data.
861-861: LGTM! RESP3 protocol explicitly set.Setting Protocol: 3 ensures consistent RESP3 response format, which is required by the parseSearchResults implementation.
TejasGhatte
force-pushed
the
09-02-feat_redis_as_vector_store
branch
from
6237854 to
6f87f7c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/docs.json (1)
149-156: vector-store page exists; fix broken nav targets in docs/docs.jsonvector-store.mdx found; no stale weaviate references. The nav sanity check reports these missing files referenced from docs/docs.json:
- docs/Gateway.mdx
- docs/server.mdx
- docs/Use as Go SDK.mdx
- docs/code.mdx
- docs/Plugins.mdx
- docs/puzzle-piece.mdx
- docs/Core Architecture.mdx
- docs/sitemap.mdx
- docs/Framework.mdx
- docs/screwdriver-wrench.mdx
Add the missing files under docs/ or update docs/docs.json to point to the correct paths so the nav entries resolve.
♻️ Duplicate comments (1)
framework/vectorstore/redis_test.go (1)
142-147: Don’t skip when namespace already existsTreat “already exists” as success; only skip for real failures (e.g., missing RediSearch). This avoids cascading skips. (This was flagged earlier; appears still unresolved.)
-err := ts.Store.CreateNamespace(ts.ctx, TestNamespace, RedisTestDimension, properties) -if err != nil { - t.Skipf("RediSearch not available or schema creation failed for namespace %q: %v", TestNamespace, err) -} else { - t.Logf("Created test namespace: %s", TestNamespace) -} +err := ts.Store.CreateNamespace(ts.ctx, TestNamespace, RedisTestDimension, properties) +if err != nil { + if strings.Contains(strings.ToLower(err.Error()), "already exists") { + t.Logf("Namespace %q already exists; continuing", TestNamespace) + return + } + t.Skipf("RediSearch not available or schema creation failed for namespace %q: %v", TestNamespace, err) +} +t.Logf("Created test namespace: %s", TestNamespace)
🧹 Nitpick comments (10)
docs/docs.json (2)
149-156: Add redirects for moved/removed docs to avoid 404s.If the old Weaviate-specific page was public, add a redirect to preserve inbound links.
Proposed addition to docs/docs.json (root-level “redirects” key):
{ "redirects": [ { "from": "/architecture/framework/vector-store/weaviate", "to": "/architecture/framework/vector-store" } ] }If an existing “redirects” array is present, append to it instead.
149-156: Ensure the new page documents the Redisnamespaceparameter.A reviewer requested adding a
namespaceparam for RedisStore; confirm the unified page covers how to set and use it so users don’t mix indices across environments.If needed, I can draft a short “Namespaces” subsection for the page—want me to?
ui/lib/types/config.ts (1)
227-227: Dimension sentinel semantics need to be explicit (or make it optional).You’ve added
dimension: numberas required, while the UI defaults to0and allows sending0. If0means “unspecified/let backend infer,” please document this in the type to avoid misuse; if some stores (e.g., Redis) require a positive dimension, consider marking this field optional and letting defaults be handled backend-side.Apply this small inline doc to clarify intent:
export interface CacheConfig { provider: ModelProviderName; keys: ModelProviderKey[]; embedding_model: string; - dimension: number; + /** + * Vector dimension. + * 0 = let backend infer / ignore (where supported). + * Positive integer required for stores that mandate dimensions (e.g., Redis). + */ + dimension: number; // consider `?: number` if backend treats it as optional ttl_seconds: number;ui/app/config/views/pluginsForm.tsx (2)
21-21: Defaulting to 0 is fine, but call it out to reduce misconfiguration.If
0is a sentinel for “infer,” add a brief comment here (and/or helper text next to the input) so users don’t enable semantic cache with an invalid dimension for backends that require it.const defaultCacheConfig: CacheConfig = { provider: "openai" as ModelProviderName, keys: [], embedding_model: "text-embedding-3-small", - dimension: 0, + // 0 = backend infer/ignore. For Redis, set an explicit positive dimension. + dimension: 0, ttl_seconds: 300,
282-291: Align input constraints with the sentinel value and guard negatives.The input uses
min="1"but the code falls back to0. This mismatch can push an invalid value for stores that accept only positive dimensions. Either allow0explicitly or clamp to >=1. Given current semantics, prefer allowing0and guarding negatives.<div className="space-y-2"> <Label htmlFor="dimension">Dimension</Label> <Input id="dimension" type="number" - min="1" + min="0" + step="1" value={cacheConfig.dimension} - onChange={(e) => debouncedUpdateCacheConfig({ dimension: parseInt(e.target.value) || 0 })} + onChange={(e) => { + const parsed = parseInt(e.target.value, 10); + const dimension = Number.isFinite(parsed) ? Math.max(0, parsed) : 0; + debouncedUpdateCacheConfig({ dimension }); + }} /> + <p className="text-muted-foreground text-xs"> + Use 0 to let the backend infer the dimension (if supported). For Redis, set the exact dimension. + </p> </div>plugins/semanticcache/test_utils.go (1)
45-79: Unused Redis test helper — remove or move to shared test utilsgetRedisConfigFromEnv is package‑private and not referenced in this file. Either delete it or relocate/export it in a shared test utils module (e.g., framework/vectorstore/test_utils.go) to avoid dead code and duplication.
framework/vectorstore/weaviate_test.go (1)
742-814: Reduce flakiness: wait briefly after Add before readsWeaviate can be eventually consistent for fresh writes. Add a short sleep before GetChunk and GetNearest in this test.
@@ - err = setup.Store.Add(setup.ctx, testClassName, testKey512, embedding512, metadata) + err = setup.Store.Add(setup.ctx, testClassName, testKey512, embedding512, metadata) require.NoError(t, err) - // Verify it was added + // Small delay for consistency + time.Sleep(100 * time.Millisecond) + // Verify it was added @@ - err = setup.Store.Add(setup.ctx, testClassName, testKey1024, embedding1024, metadata1024) + err = setup.Store.Add(setup.ctx, testClassName, testKey1024, embedding1024, metadata1024) require.NoError(t, err) - // Verify new document exists + // Small delay for consistency + time.Sleep(100 * time.Millisecond) + // Verify new document existsframework/vectorstore/redis_test.go (3)
3-13: Import strings to detect “already exists” without skippingNeeded for the CreateNamespace error check below.
import ( "context" "os" "testing" "time" + "strings" bifrost "github.com/maximhq/bifrost/core"
317-321: Avoid relying on GetChunks result orderingOrder may not be guaranteed. Validate by ID map instead.
- for i, result := range results { - assert.Equal(t, keys[i], result.ID) - assert.Equal(t, metadata[i]["type"], result.Properties["type"]) - } + byID := map[string]map[string]interface{}{} + for i, k := range keys { + byID[k] = metadata[i] + } + for _, result := range results { + assert.Contains(t, byID, result.ID) + assert.Equal(t, byID[result.ID]["type"], result.Properties["type"]) + }
850-854: Reduce flakiness: add short sleeps after Add before readsAlign with other tests that wait briefly after writes.
@@ - // Verify it was added + // Small delay to ensure consistency + time.Sleep(100 * time.Millisecond) + // Verify it was added @@ - // Verify new document exists + // Small delay to ensure consistency + time.Sleep(100 * time.Millisecond) + // Verify new document existsAlso applies to: 874-877
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
framework/go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
docs/architecture/framework/vector-store.mdx(1 hunks)docs/architecture/framework/vector-store/weaviate.mdx(0 hunks)docs/docs.json(1 hunks)docs/features/semantic-caching.mdx(1 hunks)framework/go.mod(2 hunks)framework/vectorstore/redis.go(1 hunks)framework/vectorstore/redis_test.go(1 hunks)framework/vectorstore/store.go(4 hunks)framework/vectorstore/test_utils.go(1 hunks)framework/vectorstore/utils.go(1 hunks)framework/vectorstore/weaviate.go(2 hunks)framework/vectorstore/weaviate_test.go(1 hunks)plugins/semanticcache/main.go(10 hunks)plugins/semanticcache/search.go(8 hunks)plugins/semanticcache/stream.go(1 hunks)plugins/semanticcache/test_utils.go(6 hunks)plugins/semanticcache/utils.go(1 hunks)ui/app/config/views/pluginsForm.tsx(2 hunks)ui/lib/types/config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- docs/architecture/framework/vector-store/weaviate.mdx
🚧 Files skipped from review as they are similar to previous changes (12)
- framework/vectorstore/test_utils.go
- framework/vectorstore/store.go
- plugins/semanticcache/utils.go
- framework/vectorstore/utils.go
- plugins/semanticcache/main.go
- plugins/semanticcache/stream.go
- framework/go.mod
- docs/features/semantic-caching.mdx
- plugins/semanticcache/search.go
- framework/vectorstore/redis.go
- docs/architecture/framework/vector-store.mdx
- framework/vectorstore/weaviate.go
🧰 Additional context used
🧬 Code graph analysis (4)
framework/vectorstore/weaviate_test.go (2)
plugins/semanticcache/test_utils.go (1)
NewTestSetup(115-132)framework/vectorstore/store.go (2)
VectorStoreProperties(63-66)VectorStorePropertyTypeString(71-71)
ui/app/config/views/pluginsForm.tsx (2)
ui/components/ui/label.tsx (1)
Label(21-21)ui/components/ui/input.tsx (1)
Input(7-21)
plugins/semanticcache/test_utils.go (1)
framework/vectorstore/redis.go (1)
RedisConfig(22-40)
framework/vectorstore/redis_test.go (4)
framework/vectorstore/redis.go (2)
RedisStore(43-47)RedisConfig(22-40)core/schemas/logger.go (2)
Logger(28-55)LogLevelInfo(11-11)framework/vectorstore/store.go (12)
Config(92-96)VectorStoreProperties(63-66)VectorStorePropertyTypeString(71-71)VectorStorePropertyTypeInteger(72-72)VectorStorePropertyTypeBoolean(73-73)Query(20-24)QueryOperatorGreaterThan(31-31)QueryOperatorEqual(29-29)QueryOperatorLessThan(32-32)VectorStore(78-89)VectorStoreTypeRedis(16-16)NewVectorStore(138-168)core/logger.go (1)
NewDefaultLogger(40-49)
TejasGhatte
force-pushed
the
09-02-feat_redis_as_vector_store
branch
2 times, most recently
from
32580c0 to
637b4d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/app/config/views/pluginsForm.tsx (2)
271-280: Threshold handler blocks setting 0 due to “|| 0.8”. Clamp instead.
parseFloat(...) || 0.8treats0as falsy, preventing users from choosing a valid 0. UsevalueAsNumberand clamp to [0,1].- onChange={(e) => debouncedUpdateCacheConfig({ threshold: parseFloat(e.target.value) || 0.8 })} + onChange={(e) => { + const v = e.currentTarget.valueAsNumber; + debouncedUpdateCacheConfig({ + threshold: Number.isFinite(v) ? Math.min(1, Math.max(0, v)) : 0.8, + }); + }}
128-159: Debounced revert uses stalecacheConfigfrom closure; capture prev state.On backend error,
setCacheConfig(cacheConfig)may revert to a newer or unrelated state. CaptureprevConfigbefore optimistic update and revert to that.const debouncedUpdateCacheConfig = useCallback( (updates: Partial<CacheConfig>) => { - // Update local state immediately for responsive UI - const newConfig = { ...cacheConfig, ...updates }; + // Update local state immediately for responsive UI + const prevConfig = cacheConfig; + const newConfig = { ...prevConfig, ...updates }; setCacheConfig(newConfig); @@ - .catch((error) => { + .catch((error) => { toast.error("Failed to update cache configuration"); // Revert on error - setCacheConfig(cacheConfig); + setCacheConfig(prevConfig); });
♻️ Duplicate comments (2)
framework/vectorstore/redis.go (2)
29-36: Compile error: redis.Options has no MaxActiveConns/MaxIdleConns.Drop unsupported fields here to match go-redis v9.
type RedisConfig struct { @@ - PoolSize int `json:"pool_size,omitempty"` // Maximum number of socket connections (optional) - MaxActiveConns int `json:"max_active_conns,omitempty"` // Maximum number of active connections (optional) - MinIdleConns int `json:"min_idle_conns,omitempty"` // Minimum number of idle connections (optional) - MaxIdleConns int `json:"max_idle_conns,omitempty"` // Maximum number of idle connections (optional) + PoolSize int `json:"pool_size,omitempty"` // Maximum number of socket connections (optional) + MinIdleConns int `json:"min_idle_conns,omitempty"` // Minimum number of idle connections (optional)
856-871: Compile error: remove unsupported Options fields.Match go-redis v9 Options by removing MaxActiveConns/MaxIdleConns.
client := redis.NewClient(&redis.Options{ Addr: config.Addr, Username: config.Username, Password: config.Password, DB: config.DB, Protocol: 3, // Explicitly use RESP3 protocol - PoolSize: config.PoolSize, - MaxActiveConns: config.MaxActiveConns, - MinIdleConns: config.MinIdleConns, - MaxIdleConns: config.MaxIdleConns, + PoolSize: config.PoolSize, + MinIdleConns: config.MinIdleConns, ConnMaxLifetime: config.ConnMaxLifetime, ConnMaxIdleTime: config.ConnMaxIdleTime, DialTimeout: config.DialTimeout, ReadTimeout: config.ReadTimeout, WriteTimeout: config.WriteTimeout, })
🧹 Nitpick comments (6)
ui/app/config/views/pluginsForm.tsx (1)
282-291: Dimension input: allow 0=auto (if intended), clamp and avoid parseInt pitfalls.Fix UX and validation by permitting 0, adding step, and using
valueAsNumberwith clamping and helper text.<div className="space-y-2"> <Label htmlFor="dimension">Dimension</Label> <Input id="dimension" type="number" - min="1" - value={cacheConfig.dimension} - onChange={(e) => debouncedUpdateCacheConfig({ dimension: parseInt(e.target.value) || 0 })} + min="0" + step="1" + value={cacheConfig.dimension} + onChange={(e) => { + const v = e.currentTarget.valueAsNumber; + debouncedUpdateCacheConfig({ + dimension: Number.isFinite(v) ? Math.max(0, Math.floor(v)) : 0, + }); + }} /> + <p className="text-muted-foreground text-xs">0 uses the embedding model's default dimension.</p> </div>plugins/semanticcache/main.go (2)
45-46: Docstring claims “1hr”, but time.ParseDuration doesn’t accept it.Either change the example to “1h” or add custom parsing for “hr”.
-// It supports TTL parsing from both string durations ("1m", "1hr") and numeric seconds for configurable cache behavior. +// It supports TTL parsing from both string durations ("1m", "1h") and numeric seconds for configurable cache behavior.
302-314: Avoid shadowing the imported package name “bifrost”.Minor readability nit; rename the local variable.
- bifrost, err := bifrost.Init(ctx, schemas.BifrostConfig{ + bfClient, err := bifrost.Init(ctx, schemas.BifrostConfig{ Logger: logger, Account: &PluginAccount{ provider: config.Provider, keys: config.Keys, }, }) if err != nil { return nil, fmt.Errorf("failed to initialize bifrost for semantic cache: %w", err) } - - plugin.client = bifrost + plugin.client = bfClientframework/vectorstore/test_utils.go (1)
30-46: Seed randomness to reduce test flakiness.Deterministic embeddings make assertions stable across runs.
+func init() { + rand.Seed(1) +}framework/vectorstore/redis_test.go (1)
16-22: Use a unique test namespace per run to avoid collisions.Parallel runs or leaked data can interfere with fixed “TestRedis”.
-const ( - RedisTestTimeout = 30 * time.Second - TestNamespace = "TestRedis" +var ( + RedisTestTimeout = 30 * time.Second + TestNamespace = "TestRedis_" + generateUUID()framework/vectorstore/redis.go (1)
284-369: RESP3-only parsing: okay since Protocol=3 is set. Add a brief comment.Clarify the coupling to avoid future regressions.
func (s *RedisStore) parseSearchResults(result interface{}, selectFields []string) ([]SearchResult, error) { - // FT.SEARCH returns a map with results array + // RESP3: FT.SEARCH returns a map with "results"/"extra_attributes". + // newRedisStore pins Protocol=3 to guarantee this shape.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
framework/go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
docs/architecture/framework/vector-store.mdx(1 hunks)docs/architecture/framework/vector-store/weaviate.mdx(0 hunks)docs/docs.json(1 hunks)docs/features/semantic-caching.mdx(1 hunks)framework/go.mod(2 hunks)framework/vectorstore/redis.go(1 hunks)framework/vectorstore/redis_test.go(1 hunks)framework/vectorstore/store.go(4 hunks)framework/vectorstore/test_utils.go(1 hunks)framework/vectorstore/utils.go(1 hunks)framework/vectorstore/weaviate.go(2 hunks)framework/vectorstore/weaviate_test.go(1 hunks)plugins/semanticcache/main.go(10 hunks)plugins/semanticcache/plugin_core_test.go(3 hunks)plugins/semanticcache/search.go(8 hunks)plugins/semanticcache/stream.go(1 hunks)plugins/semanticcache/test_utils.go(6 hunks)plugins/semanticcache/utils.go(1 hunks)ui/app/config/views/pluginsForm.tsx(2 hunks)ui/lib/types/config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- docs/architecture/framework/vector-store/weaviate.mdx
✅ Files skipped from review due to trivial changes (1)
- docs/architecture/framework/vector-store.mdx
🚧 Files skipped from review as they are similar to previous changes (11)
- plugins/semanticcache/stream.go
- ui/lib/types/config.ts
- framework/vectorstore/store.go
- docs/features/semantic-caching.mdx
- framework/go.mod
- docs/docs.json
- framework/vectorstore/utils.go
- plugins/semanticcache/search.go
- plugins/semanticcache/test_utils.go
- framework/vectorstore/weaviate_test.go
- plugins/semanticcache/utils.go
🧰 Additional context used
🧬 Code graph analysis (4)
framework/vectorstore/weaviate.go (2)
framework/vectorstore/store.go (1)
VectorStoreProperties(63-66)plugins/semanticcache/main.go (1)
VectorStoreProperties(157-194)
ui/app/config/views/pluginsForm.tsx (2)
ui/components/ui/label.tsx (1)
Label(21-21)ui/components/ui/input.tsx (1)
Input(7-21)
framework/vectorstore/redis.go (3)
core/schemas/logger.go (1)
Logger(28-55)framework/vectorstore/store.go (18)
VectorStoreProperties(63-66)VectorStorePropertyTypeInteger(72-72)SearchResult(43-47)Query(20-24)QueryOperatorEqual(29-29)QueryOperatorNotEqual(30-30)QueryOperatorLike(35-35)QueryOperatorGreaterThan(31-31)QueryOperatorGreaterThanOrEqual(33-33)QueryOperatorLessThan(32-32)QueryOperatorLessThanOrEqual(34-34)QueryOperatorIsNull(38-38)QueryOperatorIsNotNull(39-39)QueryOperatorContainsAny(36-36)QueryOperatorContainsAll(37-37)DeleteResult(50-54)DeleteStatusError(60-60)DeleteStatusSuccess(59-59)plugins/semanticcache/main.go (1)
VectorStoreProperties(157-194)
plugins/semanticcache/main.go (1)
framework/vectorstore/store.go (1)
VectorStoreProperties(63-66)
🔇 Additional comments (2)
plugins/semanticcache/plugin_core_test.go (1)
336-363: LGTM: model name updates are consistent.framework/vectorstore/weaviate.go (1)
451-499: Dimension-aware CreateNamespace looks good.Applying vectorDimensions only when >0 preserves backward compatibility.
If you’ve validated “vectorDimensions” against your current Weaviate version in CI, we’re good. Otherwise, confirm the exact key in the version you target.
TejasGhatte
force-pushed
the
09-02-feat_redis_as_vector_store
branch
2 times, most recently
from
16484ca to
3e8863d
Compare
TejasGhatte
force-pushed
the
09-02-feat_redis_as_vector_store
branch
from
3e8863d to
1b4a949
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugins/semanticcache/main.go (1)
623-637: Cleanup order is sane; destructive namespace deletion is intentional per product choiceDeleteAll (scoped by from_bifrost_semantic_cache_plugin) followed by DeleteNamespace is consistent with earlier discussion. Ensure docs clearly mark the namespace as plugin-owned/ephemeral, which you already mentioned in prior comments.
🧹 Nitpick comments (7)
plugins/semanticcache/main.go (2)
32-36: Dimension in Config: consider omitempty to reduce noisy serializationdimension is required for Redis but optional (auto-inferred) for Weaviate. Marking it omitempty avoids emitting 0 in JSON when not set.
- Dimension int `json:"dimension"` + Dimension int `json:"dimension,omitempty"`
312-315: CreateNamespace timeout + dimension threading — good; add one debug for DXWe’re calling CreateNamespace with a bounded context and passing dimension through — great. Add a debug log that prints the namespace and dimension to aid troubleshooting (esp. when Redis requires >0).
createCtx, cancel := context.WithTimeout(ctx, CreateNamespaceTimeout) defer cancel() +logger.Debug(fmt.Sprintf("%s Creating vector namespace %q (dimension=%d)", PluginLoggerPrefix, config.VectorStoreNamespace, config.Dimension)) if err := store.CreateNamespace(createCtx, config.VectorStoreNamespace, config.Dimension, VectorStoreProperties); err != nil {framework/vectorstore/redis_test.go (2)
142-148: Treat “already exists” as success instead of skipping the whole suiteCreateNamespace may return an “already exists” error; don’t skip in that case or subsequent tests will be silently dropped.
- err := ts.Store.CreateNamespace(ts.ctx, TestNamespace, RedisTestDimension, properties) - if err != nil { - t.Skipf("RediSearch not available or schema creation failed for namespace %q: %v", TestNamespace, err) - } else { - t.Logf("Created test namespace: %s", TestNamespace) - } + err := ts.Store.CreateNamespace(ts.ctx, TestNamespace, RedisTestDimension, properties) + if err != nil { + if strings.Contains(strings.ToLower(err.Error()), "already exists") { + t.Logf("Namespace %q already exists; continuing", TestNamespace) + } else { + t.Skipf("RediSearch not available or schema creation failed for namespace %q: %v", TestNamespace, err) + } + } else { + t.Logf("Created test namespace: %s", TestNamespace) + }Remember to import strings.
83-95: Tests leave the index around; optional: drop test namespace on CleanupYou already delete keys; optionally call DeleteNamespace to keep Redis clean across runs.
func (ts *RedisTestSetup) Cleanup(t *testing.T) { defer ts.cancel() if !testing.Short() { ts.cleanupTestData(t) + if err := ts.Store.DeleteNamespace(ts.ctx, TestNamespace); err != nil { + t.Logf("Warning: Failed to drop test namespace: %v", err) + } } if err := ts.Store.Close(ts.ctx, TestNamespace); err != nil { t.Logf("Warning: Failed to close store: %v", err) } }framework/vectorstore/redis.go (3)
463-516: KNN query honors LIMIT and returns score; small safeguard recommendedAdd an early namespace check to surface clearer errors and optionally validate vector length if you decide to persist per-namespace dimension later.
func (s *RedisStore) GetNearest(...){ ctx, cancel := withTimeout(ctx, s.config.ContextTimeout) defer cancel() + if strings.TrimSpace(namespace) == "" { + return nil, fmt.Errorf("namespace is required") + } // (Optionally) if you later track dimension: compare len(vector) with index dim and error early.
582-611: Add-time dimensionality check (optional, if you track index dim)If you store/know the index dimension, validate embedding length on Add to prevent KNN failures later.
if len(embedding) > 0 { + // if known, validate embedding dimensionality to match index + // e.g., if s.indexDims[namespace] > 0 && len(embedding) != s.indexDims[namespace] { return fmt.Errorf(...) } embeddingBytes := float32SliceToBytes(embedding) fields["embedding"] = embeddingBytes }
766-795: Comma-to-OR escaping is non-standard — add a brief commentSince commas are intentionally treated as OR for TAG queries, document it inline to avoid future confusion.
- ",", "|", + // Intentional: treat comma-delimited values as OR in TAG queries + ",", "|",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
framework/go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
docs/architecture/framework/vector-store.mdx(1 hunks)docs/architecture/framework/vector-store/weaviate.mdx(0 hunks)docs/docs.json(1 hunks)docs/features/semantic-caching.mdx(1 hunks)framework/go.mod(2 hunks)framework/vectorstore/redis.go(1 hunks)framework/vectorstore/redis_test.go(1 hunks)framework/vectorstore/store.go(4 hunks)framework/vectorstore/test_utils.go(1 hunks)framework/vectorstore/utils.go(1 hunks)framework/vectorstore/weaviate.go(2 hunks)framework/vectorstore/weaviate_test.go(1 hunks)plugins/semanticcache/main.go(10 hunks)plugins/semanticcache/plugin_core_test.go(3 hunks)plugins/semanticcache/search.go(8 hunks)plugins/semanticcache/stream.go(1 hunks)plugins/semanticcache/test_utils.go(6 hunks)plugins/semanticcache/utils.go(1 hunks)ui/app/config/views/pluginsForm.tsx(2 hunks)ui/lib/types/config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- docs/architecture/framework/vector-store/weaviate.mdx
🚧 Files skipped from review as they are similar to previous changes (14)
- docs/features/semantic-caching.mdx
- framework/vectorstore/test_utils.go
- framework/vectorstore/utils.go
- ui/lib/types/config.ts
- ui/app/config/views/pluginsForm.tsx
- plugins/semanticcache/plugin_core_test.go
- plugins/semanticcache/test_utils.go
- framework/vectorstore/weaviate.go
- docs/docs.json
- framework/go.mod
- framework/vectorstore/weaviate_test.go
- docs/architecture/framework/vector-store.mdx
- framework/vectorstore/store.go
- plugins/semanticcache/search.go
🧰 Additional context used
🧬 Code graph analysis (3)
framework/vectorstore/redis_test.go (4)
framework/vectorstore/redis.go (2)
RedisStore(43-47)RedisConfig(22-40)core/schemas/logger.go (2)
Logger(28-55)LogLevelInfo(11-11)framework/vectorstore/store.go (12)
Config(92-96)VectorStoreProperties(63-66)VectorStorePropertyTypeString(71-71)VectorStorePropertyTypeInteger(72-72)VectorStorePropertyTypeBoolean(73-73)Query(20-24)QueryOperatorGreaterThan(31-31)QueryOperatorEqual(29-29)QueryOperatorLessThan(32-32)VectorStore(78-89)VectorStoreTypeRedis(16-16)NewVectorStore(138-168)core/logger.go (1)
NewDefaultLogger(40-49)
plugins/semanticcache/main.go (1)
framework/vectorstore/store.go (1)
VectorStoreProperties(63-66)
framework/vectorstore/redis.go (2)
core/schemas/logger.go (1)
Logger(28-55)framework/vectorstore/store.go (18)
VectorStoreProperties(63-66)VectorStorePropertyTypeInteger(72-72)SearchResult(43-47)Query(20-24)QueryOperatorEqual(29-29)QueryOperatorNotEqual(30-30)QueryOperatorLike(35-35)QueryOperatorGreaterThan(31-31)QueryOperatorGreaterThanOrEqual(33-33)QueryOperatorLessThan(32-32)QueryOperatorLessThanOrEqual(34-34)QueryOperatorIsNull(38-38)QueryOperatorIsNotNull(39-39)QueryOperatorContainsAny(36-36)QueryOperatorContainsAll(37-37)DeleteResult(50-54)DeleteStatusError(60-60)DeleteStatusSuccess(59-59)
🔇 Additional comments (8)
plugins/semanticcache/utils.go (1)
308-308: Namespace switch to configured value — looks goodUsing plugin.config.VectorStoreNamespace here aligns with the new pluggable namespace behavior across the plugin.
plugins/semanticcache/stream.go (1)
123-123: Consistent use of configured namespace in streaming finalizationThe final Add now targets plugin.config.VectorStoreNamespace, matching single-response logic. Good consistency.
plugins/semanticcache/main.go (2)
264-267: Good defaulting for VectorStoreNamespaceDefaulting and logging help trace config. Thanks for the explicit debug.
667-670: Timeouts added for DeleteAll/Delete — niceThis prevents request-thread hangs on external calls.
Also applies to: 696-699
framework/vectorstore/redis.go (4)
71-86: Fail-fast on invalid dimension — goodRejecting DIM<=0 at FT.CREATE avoids opaque Redis errors.
225-281: GetAll: LIMIT/OFFSET + DIALECT 2 implemented correctlyHonors limit=0 as “all” via MaxInt32 and supports cursor-based paging. Solid.
284-369: RESP3-only parsing is acceptable given Protocol:3parseSearchResults expects RESP3 maps and newRedisStore pins Protocol:3. This coupling is fine; just ensure we don’t downgrade Protocol elsewhere.
818-833: Compilation blocker: redis.Options has no MaxActiveConns/MaxIdleConnsgo-redis v9 uses PoolSize and MinIdleConns; MaxActiveConns/MaxIdleConns aren’t valid fields and will fail to compile.
client := redis.NewClient(&redis.Options{ Addr: config.Addr, Username: config.Username, Password: config.Password, DB: config.DB, Protocol: 3, // Explicitly use RESP3 protocol PoolSize: config.PoolSize, - MaxActiveConns: config.MaxActiveConns, MinIdleConns: config.MinIdleConns, - MaxIdleConns: config.MaxIdleConns, ConnMaxLifetime: config.ConnMaxLifetime, ConnMaxIdleTime: config.ConnMaxIdleTime, DialTimeout: config.DialTimeout, ReadTimeout: config.ReadTimeout, WriteTimeout: config.WriteTimeout, })Optional follow-up: deprecate MaxActiveConns/MaxIdleConns in RedisConfig or map legacy MaxActiveConns to PoolSize when PoolSize==0.
⛔ Skipped due to learnings
Learnt from: TejasGhatte PR: maximhq/bifrost#450 File: docs/architecture/framework/vector-store/redis.mdx:59-63 Timestamp: 2025-09-12T14:02:20.389Z Learning: go-redis v9 (specifically v9.2.0+) supports MaxActiveConns field in redis.Options struct for connection pool management. This field is valid and should not be flagged as unsupported in go-redis configurations.
Merge activity
|
## Summary Add Redis vector store implementation to support Redis as a backend for vector storage and retrieval operations. ## Changes - Implemented a new `RedisStore` that implements the `VectorStore` interface - Added support for Redis with RediSearch module for vector similarity search - Implemented all required vector store operations (add, get, delete, search) - Added comprehensive test suite for Redis vector store - Updated the vector store factory to support Redis as a store type ## Type of change - [x] Feature - [ ] Bug fix - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (Next.js) - [ ] Docs ## How to test Requires a Redis instance with RediSearch module enabled: ```sh # Run Redis with RediSearch module docker run -p 6379:6379 redis/redis-stack # Set environment variables for tests export REDIS_ADDR=localhost:6379 export REDIS_USERNAME= export REDIS_PASSWORD= # Run tests go test ./framework/vectorstore -v ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues Adds Redis as an alternative to Weaviate for vector storage, providing more deployment options. ## Security considerations - Redis connection details (username/password) are handled securely - Proper error handling for all Redis operations - Timeouts implemented for all Redis operations to prevent hanging ## Checklist - [x] I added/updated tests where appropriate - [x] I verified builds succeed (Go) - [x] I verified the CI pipeline passes locally
3 participants
Summary
Add Redis vector store implementation to support Redis as a backend for vector storage and retrieval operations.
Changes
RedisStorethat implements theVectorStoreinterfaceType of change
Affected areas
How to test
Requires a Redis instance with RediSearch module enabled:
Breaking changes
Related issues
Adds Redis as an alternative to Weaviate for vector storage, providing more deployment options.
Security considerations
Checklist