fix: correct orderBy sort for timestamp fields via centralized field resolver
Muse reported that find({ orderBy: 'createdAt' }) silently returned the
wrong order: getFieldValueForEntity read noun.metadata[field] for
timestamp fields, but getNoun() destructures standard fields to the top
level, so the read always returned undefined and the sort collapsed to
insertion order.
The fix introduces a single source of truth for reading fields off an
entity. STANDARD_ENTITY_FIELDS + resolveEntityField() in coreTypes.ts
encode the "standard fields top-level, custom fields nested" contract
in one place. getFieldValueForEntity now uses the helper and routes
through a named BUCKETED_INDEX_FIELDS set instead of a hardcoded
timestamp if-chain — filtered sort on createdAt/updatedAt now works.
Unfiltered orderBy is explicitly rejected with a clear error pointing
callers at the right pattern. A scalable, unfiltered-sort-capable
time-ordered segment index is tracked as a separate follow-up.
This commit is contained in:
parent
086d90d01c
commit
be6c4dc182
4 changed files with 343 additions and 29 deletions
|
|
@ -2166,6 +2166,21 @@ export class Brainy<T = any> implements BrainyInterface<T> {
|
|||
const limit = params.limit || 20
|
||||
const offset = params.offset || 0
|
||||
|
||||
// orderBy without any filter is not supported — it would require
|
||||
// O(N) work across every entity in storage. Consumers must supply
|
||||
// a filter (`type`, `where`, or `excludeVFS: true`) so the sort
|
||||
// runs over a bounded, roaring-bitmap-filtered set. The long-term
|
||||
// fix is a dedicated time-ordered segment index (Track 2).
|
||||
if (params.orderBy) {
|
||||
throw new Error(
|
||||
`find({ orderBy: '${params.orderBy}' }) requires a filter. ` +
|
||||
`Add 'type', 'where', or 'excludeVFS: true' so the sort runs ` +
|
||||
`over a bounded set. Unfiltered sort over all entities is not ` +
|
||||
`scalable with the current index and is tracked as a dedicated ` +
|
||||
`time-ordered segment index (Track 2).`
|
||||
)
|
||||
}
|
||||
|
||||
// ExcludeVFS helper - exclude VFS infrastructure entities
|
||||
// VFS files/folders have vfsType set, extracted entities do NOT
|
||||
let filter: any = {}
|
||||
|
|
|
|||
|
|
@ -233,6 +233,58 @@ export interface HNSWNounWithMetadata {
|
|||
metadata?: Record<string, unknown>
|
||||
}
|
||||
|
||||
/**
|
||||
* Standard top-level fields on HNSWNounWithMetadata.
|
||||
*
|
||||
* Single source of truth for the entity shape contract: fields listed here
|
||||
* live at the top level of the entity; any other field is a custom user
|
||||
* field and lives in `entity.metadata`.
|
||||
*
|
||||
* Keep this set in lockstep with the HNSWNounWithMetadata interface above.
|
||||
* Adding a new top-level field to the interface? Add it here too, or
|
||||
* `resolveEntityField` will look for it in the wrong place.
|
||||
*/
|
||||
export const STANDARD_ENTITY_FIELDS: ReadonlySet<string> = new Set([
|
||||
'id',
|
||||
'vector',
|
||||
'connections',
|
||||
'level',
|
||||
'type',
|
||||
'confidence',
|
||||
'weight',
|
||||
'createdAt',
|
||||
'updatedAt',
|
||||
'service',
|
||||
'createdBy',
|
||||
'data'
|
||||
])
|
||||
|
||||
/**
|
||||
* Resolve a field value off an entity by name.
|
||||
*
|
||||
* Encodes the HNSWNounWithMetadata shape contract in one place: standard
|
||||
* fields live at the top level, custom user fields live in `entity.metadata`.
|
||||
* Use this helper anywhere code needs to read a field by name (sorting,
|
||||
* filtering, aggregation) instead of reaching into the entity directly.
|
||||
*
|
||||
* @param entity - The entity to read from
|
||||
* @param field - The field name to resolve
|
||||
* @returns The field value, or undefined if not present
|
||||
*
|
||||
* @example
|
||||
* resolveEntityField(noun, 'createdAt') // reads noun.createdAt (top-level)
|
||||
* resolveEntityField(noun, 'customTag') // reads noun.metadata?.customTag
|
||||
*/
|
||||
export function resolveEntityField(
|
||||
entity: HNSWNounWithMetadata,
|
||||
field: string
|
||||
): unknown {
|
||||
if (STANDARD_ENTITY_FIELDS.has(field)) {
|
||||
return (entity as unknown as Record<string, unknown>)[field]
|
||||
}
|
||||
return entity.metadata?.[field]
|
||||
}
|
||||
|
||||
/**
|
||||
* Combined verb structure for transport/API boundaries
|
||||
*
|
||||
|
|
|
|||
|
|
@ -4,7 +4,7 @@
|
|||
* Automatically updates indexes when data changes
|
||||
*/
|
||||
|
||||
import { StorageAdapter } from '../coreTypes.js'
|
||||
import { StorageAdapter, resolveEntityField } from '../coreTypes.js'
|
||||
import { MetadataIndexCache, MetadataIndexCacheConfig } from './metadataIndexCache.js'
|
||||
import { prodLog } from './logger.js'
|
||||
import { getGlobalCache, UnifiedCache } from './unifiedCache.js'
|
||||
|
|
@ -28,6 +28,21 @@ import { EntityIdMapper } from './entityIdMapper.js'
|
|||
import { RoaringBitmap32, roaringLibraryInitialize } from './roaring/index.js'
|
||||
import { FieldTypeInference, FieldType } from './fieldTypeInference.js'
|
||||
|
||||
/**
|
||||
* Fields whose values are stored in the sparse index as BUCKETED values
|
||||
* (rounded to a coarser granularity to keep the index compact). Sorting
|
||||
* and any precision-sensitive comparison on these fields must bypass the
|
||||
* index and read the actual value directly from entity storage.
|
||||
*
|
||||
* Currently only timestamps are bucketed — they round to 1-minute windows
|
||||
* via `Math.floor(ts / 60000) * 60000` in the chunking layer. If any new
|
||||
* bucketed field is added (e.g. a compressed float), add it here too.
|
||||
*/
|
||||
const BUCKETED_INDEX_FIELDS: ReadonlySet<string> = new Set([
|
||||
'createdAt',
|
||||
'updatedAt'
|
||||
])
|
||||
|
||||
export interface MetadataIndexEntry {
|
||||
field: string
|
||||
value: string | number | boolean
|
||||
|
|
@ -2216,6 +2231,12 @@ export class MetadataIndexManager {
|
|||
order: 'asc' | 'desc' = 'asc'
|
||||
): Promise<string[]> {
|
||||
// 1. Get filtered IDs using existing roaring bitmap implementation (fast!)
|
||||
//
|
||||
// NOTE: This method REQUIRES a non-empty filter. Unfiltered sort over all
|
||||
// entities is not supported here because it would require O(N) storage reads
|
||||
// on bucketed fields (timestamps), which does not scale. The proper solution
|
||||
// is a dedicated time-ordered segment index — tracked as Track 2.
|
||||
// Callers should enforce a filter (type / where / excludeVFS) before calling.
|
||||
const filteredIds = await this.getIdsForFilter(filter)
|
||||
|
||||
if (filteredIds.length === 0) {
|
||||
|
|
@ -2254,17 +2275,24 @@ export class MetadataIndexManager {
|
|||
/**
|
||||
* Get field value for a specific entity (helper for sorted queries)
|
||||
*
|
||||
* **IMPORTANT**: For timestamp fields (createdAt, updatedAt), this loads
|
||||
* the ACTUAL value from entity metadata, NOT the bucketed index value.
|
||||
* This is required because timestamp bucketing (1-minute precision) loses
|
||||
* precision needed for accurate sorting.
|
||||
* Three-path lookup:
|
||||
*
|
||||
* For non-timestamp fields, loads from the chunked sparse index without
|
||||
* loading the full entity. This is critical for production-scale sorting.
|
||||
* 1. **Bucketed fields** (timestamps) — the sparse index stores values
|
||||
* rounded to 1-minute buckets to keep the index compact for range
|
||||
* queries. That bucketing loses precision, so sorting must read the
|
||||
* actual value directly from entity storage.
|
||||
*
|
||||
* 2. **Custom fields with no sparse index** — VFS fields like `modified`
|
||||
* and `accessed`, plus any user custom field whose sparse index was
|
||||
* never built. Resolved from entity storage via `resolveEntityField`,
|
||||
* which knows the top-level-vs-metadata shape contract.
|
||||
*
|
||||
* 3. **Indexed fields** — strings, enums, and low-cardinality ints live
|
||||
* in the sparse roaring index. O(chunks) lookup, typically 1-10 chunks.
|
||||
*
|
||||
* **Performance**:
|
||||
* - Timestamp fields: O(1) metadata load from storage (cached)
|
||||
* - Other fields: O(chunks) roaring bitmap lookup (typically 1-10 chunks)
|
||||
* - Paths 1 & 2: O(1) entity load from storage (cached)
|
||||
* - Path 3: O(chunks) roaring bitmap lookup
|
||||
*
|
||||
* @param entityId - Entity UUID to get field value for
|
||||
* @param field - Field name to retrieve (e.g., 'createdAt', 'title')
|
||||
|
|
@ -2273,43 +2301,39 @@ export class MetadataIndexManager {
|
|||
* @public (called from brainy.ts for sorted queries)
|
||||
*/
|
||||
async getFieldValueForEntity(entityId: string, field: string): Promise<any> {
|
||||
// For timestamp fields, load ACTUAL value from entity metadata
|
||||
// (index has bucketed values which lose precision for sorting)
|
||||
if (field === 'createdAt' || field === 'updatedAt' || field === 'accessed' || field === 'modified') {
|
||||
try {
|
||||
const noun = await this.storage.getNoun(entityId)
|
||||
if (noun && noun.metadata) {
|
||||
return noun.metadata[field]
|
||||
}
|
||||
} catch (err) {
|
||||
// If metadata load fails, fall back to index (bucketed value)
|
||||
console.warn(`[MetadataIndex] Failed to load ${field} from metadata for ${entityId}, using bucketed value`)
|
||||
}
|
||||
// Path 1: Bucketed fields need the actual value from storage.
|
||||
if (BUCKETED_INDEX_FIELDS.has(field)) {
|
||||
const noun = await this.storage.getNoun(entityId)
|
||||
return noun ? resolveEntityField(noun, field) : undefined
|
||||
}
|
||||
|
||||
// For non-timestamp fields, use the sparse index (no bucketing issues)
|
||||
// Path 3 precondition: entity must be in the id mapper for bitmap lookup.
|
||||
const intId = this.idMapper.getInt(entityId)
|
||||
if (intId === undefined) {
|
||||
return undefined
|
||||
}
|
||||
|
||||
// Load sparse index for this field (cached via UnifiedCache)
|
||||
// Load sparse index for this field (cached via UnifiedCache).
|
||||
const sparseIndex = await this.loadSparseIndex(field)
|
||||
|
||||
// Path 2: No sparse index exists — fall back to entity storage.
|
||||
// Covers VFS custom fields (modified, accessed) and user fields not
|
||||
// yet indexed. resolveEntityField handles the shape contract.
|
||||
if (!sparseIndex) {
|
||||
return undefined
|
||||
const noun = await this.storage.getNoun(entityId)
|
||||
return noun ? resolveEntityField(noun, field) : undefined
|
||||
}
|
||||
|
||||
// Search through chunks to find which value this entity has
|
||||
// Typically 1-10 chunks per field, so this is fast
|
||||
// Path 3: Search sparse index chunks for this entity's value.
|
||||
// Typically 1-10 chunks per field, so this is fast.
|
||||
for (const chunkId of sparseIndex.getAllChunkIds()) {
|
||||
const chunk = await this.chunkManager.loadChunk(field, chunkId)
|
||||
if (!chunk) continue
|
||||
|
||||
// Check each value's roaring bitmap for our entity ID
|
||||
// Roaring bitmap .has() is O(1) with SIMD optimization
|
||||
// Check each value's roaring bitmap for our entity ID.
|
||||
// Roaring bitmap .has() is O(1) with SIMD optimization.
|
||||
for (const [value, bitmap] of chunk.entries) {
|
||||
if (bitmap.has(intId)) {
|
||||
// Found it! Denormalize the value (no bucketing for non-timestamps)
|
||||
return this.denormalizeValue(value, field)
|
||||
}
|
||||
}
|
||||
|
|
|
|||
223
tests/integration/orderby-sort-bug.test.ts
Normal file
223
tests/integration/orderby-sort-bug.test.ts
Normal file
|
|
@ -0,0 +1,223 @@
|
|||
/**
|
||||
* @module orderby-sort-bug
|
||||
* @description Regression tests for the `find({ orderBy: ... })` sort bug.
|
||||
*
|
||||
* Bug: `getFieldValueForEntity` read `noun.metadata[field]` for timestamp
|
||||
* fields, but `getNoun()` destructures standard fields to the top level.
|
||||
* Every entity returned `undefined` for the sort key, stable sort preserved
|
||||
* insertion order, and `order: 'desc'` behaved like `order: 'asc'`.
|
||||
*
|
||||
* Fix: centralized `resolveEntityField` helper + `BUCKETED_INDEX_FIELDS`
|
||||
* set in coreTypes.ts, used by getFieldValueForEntity.
|
||||
*
|
||||
* Reported by Muse team 2026-04-09 (handoff action BR-ORDERBY-TS).
|
||||
*
|
||||
* NOTE: These tests cover FILTERED sort, which is the only supported path.
|
||||
* Unfiltered `find({ orderBy })` is explicitly rejected until the dedicated
|
||||
* time-ordered segment index ships (Track 2).
|
||||
*/
|
||||
|
||||
import { describe, it, expect, beforeEach, afterEach } from 'vitest'
|
||||
import { Brainy } from '../../src/brainy'
|
||||
import {
|
||||
resolveEntityField,
|
||||
STANDARD_ENTITY_FIELDS,
|
||||
type HNSWNounWithMetadata
|
||||
} from '../../src/coreTypes'
|
||||
import { NounType } from '../../src/types/graphTypes'
|
||||
|
||||
describe('find({ orderBy }) sort bug regression', () => {
|
||||
let brain: Brainy<any>
|
||||
|
||||
beforeEach(async () => {
|
||||
brain = new Brainy({ storage: { type: 'memory' }, silent: true })
|
||||
await brain.init()
|
||||
})
|
||||
|
||||
afterEach(async () => {
|
||||
await brain.close()
|
||||
})
|
||||
|
||||
/**
|
||||
* Muse's real use case: filtered sort of chat sessions.
|
||||
* Before the fix, this returned the OLDEST entity instead of the newest
|
||||
* because getFieldValueForEntity was reading createdAt from the wrong
|
||||
* location on the entity.
|
||||
*/
|
||||
it('orderBy createdAt desc with filter returns newest matching entity', async () => {
|
||||
// Add 4 entities 20ms apart so createdAt values are distinct.
|
||||
const id1 = await brain.add({ data: 'first', type: NounType.Concept })
|
||||
await new Promise((r) => setTimeout(r, 20))
|
||||
await brain.add({ data: 'second', type: NounType.Concept })
|
||||
await new Promise((r) => setTimeout(r, 20))
|
||||
await brain.add({ data: 'third', type: NounType.Concept })
|
||||
await new Promise((r) => setTimeout(r, 20))
|
||||
const id4 = await brain.add({ data: 'fourth', type: NounType.Concept })
|
||||
|
||||
const results = await brain.find({
|
||||
type: NounType.Concept,
|
||||
orderBy: 'createdAt',
|
||||
order: 'desc',
|
||||
limit: 1
|
||||
})
|
||||
|
||||
expect(results).toHaveLength(1)
|
||||
// Must be the last-inserted entity, not the first.
|
||||
expect(results[0].id).toBe(id4)
|
||||
expect(results[0].id).not.toBe(id1)
|
||||
})
|
||||
|
||||
it('orderBy createdAt asc with filter returns oldest matching entity', async () => {
|
||||
const id1 = await brain.add({ data: 'first', type: NounType.Concept })
|
||||
await new Promise((r) => setTimeout(r, 20))
|
||||
await brain.add({ data: 'second', type: NounType.Concept })
|
||||
await new Promise((r) => setTimeout(r, 20))
|
||||
await brain.add({ data: 'third', type: NounType.Concept })
|
||||
|
||||
const results = await brain.find({
|
||||
type: NounType.Concept,
|
||||
orderBy: 'createdAt',
|
||||
order: 'asc',
|
||||
limit: 1
|
||||
})
|
||||
|
||||
expect(results).toHaveLength(1)
|
||||
expect(results[0].id).toBe(id1)
|
||||
})
|
||||
|
||||
it('orderBy createdAt desc with filter returns all matching entities in newest-first order', async () => {
|
||||
const ids: string[] = []
|
||||
for (let i = 0; i < 5; i++) {
|
||||
ids.push(await brain.add({ data: `item-${i}`, type: NounType.Concept }))
|
||||
await new Promise((r) => setTimeout(r, 20))
|
||||
}
|
||||
|
||||
const results = await brain.find({
|
||||
type: NounType.Concept,
|
||||
orderBy: 'createdAt',
|
||||
order: 'desc'
|
||||
})
|
||||
|
||||
expect(results).toHaveLength(5)
|
||||
expect(results.map((r) => r.id)).toEqual([...ids].reverse())
|
||||
})
|
||||
|
||||
it('orderBy updatedAt desc with filter returns most-recently-updated matching entity', async () => {
|
||||
const id1 = await brain.add({ data: 'first', type: NounType.Concept })
|
||||
await new Promise((r) => setTimeout(r, 20))
|
||||
await brain.add({ data: 'second', type: NounType.Concept })
|
||||
await new Promise((r) => setTimeout(r, 20))
|
||||
await brain.add({ data: 'third', type: NounType.Concept })
|
||||
|
||||
// Touch id1 so it becomes the most-recently-updated.
|
||||
await new Promise((r) => setTimeout(r, 20))
|
||||
await brain.update({ id: id1, data: 'first-updated' })
|
||||
|
||||
const results = await brain.find({
|
||||
type: NounType.Concept,
|
||||
orderBy: 'updatedAt',
|
||||
order: 'desc',
|
||||
limit: 1
|
||||
})
|
||||
|
||||
expect(results).toHaveLength(1)
|
||||
expect(results[0].id).toBe(id1)
|
||||
})
|
||||
|
||||
/**
|
||||
* Unfiltered sort is explicitly rejected with a clear error pointing
|
||||
* callers at the right pattern. This guards against silent N-scans
|
||||
* on production workloads.
|
||||
*/
|
||||
it('orderBy without any filter throws a helpful error', async () => {
|
||||
await brain.add({ data: 'anything', type: NounType.Concept })
|
||||
|
||||
await expect(
|
||||
brain.find({ orderBy: 'createdAt', order: 'desc', limit: 1 })
|
||||
).rejects.toThrow(/requires a filter/)
|
||||
})
|
||||
})
|
||||
|
||||
describe('resolveEntityField helper', () => {
|
||||
const entity: HNSWNounWithMetadata = {
|
||||
id: 'abc',
|
||||
vector: [0.1, 0.2],
|
||||
connections: new Map(),
|
||||
level: 0,
|
||||
type: NounType.Concept,
|
||||
createdAt: 1700000000000,
|
||||
updatedAt: 1700000060000,
|
||||
confidence: 0.9,
|
||||
weight: 1,
|
||||
service: 'test',
|
||||
data: { title: 'Hello' },
|
||||
metadata: {
|
||||
customTag: 'green',
|
||||
priority: 5,
|
||||
modified: 1700000120000 // VFS custom field, lives in metadata
|
||||
}
|
||||
}
|
||||
|
||||
it('reads standard fields from top level', () => {
|
||||
expect(resolveEntityField(entity, 'createdAt')).toBe(1700000000000)
|
||||
expect(resolveEntityField(entity, 'updatedAt')).toBe(1700000060000)
|
||||
expect(resolveEntityField(entity, 'type')).toBe(NounType.Concept)
|
||||
expect(resolveEntityField(entity, 'confidence')).toBe(0.9)
|
||||
expect(resolveEntityField(entity, 'weight')).toBe(1)
|
||||
expect(resolveEntityField(entity, 'service')).toBe('test')
|
||||
expect(resolveEntityField(entity, 'id')).toBe('abc')
|
||||
})
|
||||
|
||||
it('reads custom fields from metadata', () => {
|
||||
expect(resolveEntityField(entity, 'customTag')).toBe('green')
|
||||
expect(resolveEntityField(entity, 'priority')).toBe(5)
|
||||
})
|
||||
|
||||
it('reads VFS custom fields (modified, accessed) from metadata', () => {
|
||||
// VFS stores `modified` as a custom field, not top-level.
|
||||
expect(resolveEntityField(entity, 'modified')).toBe(1700000120000)
|
||||
})
|
||||
|
||||
it('returns undefined for unknown fields', () => {
|
||||
expect(resolveEntityField(entity, 'nonexistent')).toBeUndefined()
|
||||
})
|
||||
|
||||
it('returns undefined for custom fields when metadata is absent', () => {
|
||||
const noMetadata: HNSWNounWithMetadata = { ...entity, metadata: undefined }
|
||||
expect(resolveEntityField(noMetadata, 'customTag')).toBeUndefined()
|
||||
})
|
||||
|
||||
it('does not look in metadata for standard fields', () => {
|
||||
// If a standard field is absent top-level, resolver returns undefined
|
||||
// rather than silently falling through to metadata. This prevents
|
||||
// misuse from masking bugs.
|
||||
const withShadowedField: HNSWNounWithMetadata = {
|
||||
...entity,
|
||||
// @ts-expect-error intentionally clobbering for the test
|
||||
createdAt: undefined,
|
||||
metadata: { createdAt: 9999 }
|
||||
}
|
||||
expect(resolveEntityField(withShadowedField, 'createdAt')).toBeUndefined()
|
||||
})
|
||||
|
||||
it('STANDARD_ENTITY_FIELDS covers every declared top-level field', () => {
|
||||
// Guards against the resolver and the interface drifting out of sync.
|
||||
const expected = [
|
||||
'id',
|
||||
'vector',
|
||||
'connections',
|
||||
'level',
|
||||
'type',
|
||||
'confidence',
|
||||
'weight',
|
||||
'createdAt',
|
||||
'updatedAt',
|
||||
'service',
|
||||
'createdBy',
|
||||
'data'
|
||||
]
|
||||
for (const field of expected) {
|
||||
expect(STANDARD_ENTITY_FIELDS.has(field)).toBe(true)
|
||||
}
|
||||
})
|
||||
})
|
||||
Loading…
Add table
Add a link
Reference in a new issue