diff --git a/src/utils/metadataIndex.ts b/src/utils/metadataIndex.ts index d07fa8d7..83d37379 100644 --- a/src/utils/metadataIndex.ts +++ b/src/utils/metadataIndex.ts @@ -961,9 +961,41 @@ export class MetadataIndexManager implements MetadataIndexProvider { } /** - * Get IDs for a range using chunked sparse index with zone maps and roaring bitmaps - * Now fully lazy-loaded via UnifiedCache (no local sparseIndices Map) - * Normalize min/max for timestamp bucketing before comparison + * Get IDs for a range using the legacy chunked sparse index (zone maps + + * roaring bitmaps). Lazy-loaded via UnifiedCache. + * + * ORDER IS NOT A KEY. This path compares NORMALIZED values, and + * {@link normalizeValue} carries an escape hatch that is order-destroying by + * design: a string over 100 characters is replaced by {@link hashValue}'s + * digest so it can be used as a filesystem-safe key. Feeding that digest to + * an ORDERING comparison — which is what a `gte` / `lt` / `between` does — + * ranks rows by hash. The result is not empty and not an error: it is a + * confidently ordered wrong answer, and it disagrees with the column-store + * path (`getIdsForRange` above), which compares raw values and is correct. + * + * Two changes hold the line here: + * + * 1. THE BOUNDS ARE NEVER HASHED. They are normalized with `allowHash = + * false`, so a long bound stays comparable instead of collapsing to a + * digest. This alone fixes the common shape — a long bound queried + * against ordinary short values, where the digest sorts below every + * letter and `gte` therefore matched the entire store. + * + * 2. A HASHED KEY IS REFUSED, NEVER GUESSED. The persisted keys are whatever + * the pre-7.20.0 writer normalized them to, so a field whose values ran + * long is stored hashed and its order is simply not recoverable from this + * index. Rather than compare digests, the query throws a typed + * `BrainyError('INVALID_QUERY')` naming the field, the bound and the cure. + * Loud beats wrong. + * + * KNOWN, NAMED DIVERGENCE. The persisted keys are also lower-cased and + * trimmed by `normalizeValue`, so this path's string ranges are + * CASE-INSENSITIVE where the column store's are not. That is a property of + * the bytes a pre-7.20.0 engine wrote, not of the comparison: the raw values + * are not in the index to compare. The bounds are normalized into the same + * case-folded space so the comparison is at least self-consistent, and the + * divergence disappears with the field itself once the column store adopts + * it. See the module note on `getIdsFromChunks` for the path's lifetime. */ private async getIdsFromChunksForRange( field: string, @@ -979,9 +1011,27 @@ export class MetadataIndexManager implements MetadataIndexProvider { } // Normalize min/max for consistent comparison with indexed values - // (indexed values are bucketed for timestamps, so we must bucket the query bounds too) - const normalizedMin = min !== undefined ? this.normalizeValue(min, field) : undefined - const normalizedMax = max !== undefined ? this.normalizeValue(max, field) : undefined + // (indexed values are bucketed for timestamps, so we must bucket the query + // bounds too) — but NEVER through the hash escape hatch, which would make + // the bound incomparable. See the doc comment above. + const normalizedMin = min !== undefined ? this.normalizeValue(min, field, false) : undefined + const normalizedMax = max !== undefined ? this.normalizeValue(max, field, false) : undefined + + // REFUSE BEFORE SELECTING. Chunk selection itself orders values: it tests + // the bounds against each chunk's zone-map min/max. If those are hashes the + // selection is already meaningless — and its failure mode is an EMPTY + // answer (no chunk appears to overlap), which is the quietest wrong answer + // of all. So the key space is checked here, before a single chunk is + // chosen, and again per key below for a chunk whose zone map happens to + // read clean. + for (const chunkId of sparseIndex.getAllChunkIds()) { + const zoneMap = sparseIndex.getChunk(chunkId)?.zoneMap + for (const bound of [zoneMap?.min, zoneMap?.max]) { + if (typeof bound === 'string' && MetadataIndexManager.isHashedValue(bound)) { + throw MetadataIndexManager.rangeOverHashedIndex(field) + } + } + } // Find candidate chunks using zone maps const candidateChunkIds = sparseIndex.findChunksForRange(normalizedMin, normalizedMax) @@ -996,6 +1046,13 @@ export class MetadataIndexManager implements MetadataIndexProvider { const chunk = await this.chunkManager.loadChunk(field, chunkId) if (chunk) { for (const [value, bitmap] of chunk.entries) { + // A hashed key carries no order. Refuse the range rather than rank by + // digest — the whole answer is unsound, so failing on the first one + // is the honest outcome. + if (MetadataIndexManager.isHashedValue(value)) { + throw MetadataIndexManager.rangeOverHashedIndex(field) + } + // Check if value is in range using numeric-aware comparison // (normalizeValue converts numbers to strings, so we must compare numerically) let inRange = true @@ -1024,6 +1081,25 @@ export class MetadataIndexManager implements MetadataIndexProvider { return this.idMapper.intsIterableToUuids(allIntIds) } + /** + * The refusal a range query gets when the legacy sparse index holds hashed + * keys for the field. Names the field and the cure; never a wrong answer. + */ + private static rangeOverHashedIndex(field: string): BrainyError { + return new BrainyError( + `Range query on field "${field}" cannot be served by the legacy sparse index: ` + + `its values were persisted as hashes (values over 100 characters are stored ` + + `hashed to stay within filesystem name limits), and a hash carries no order — ` + + `comparing them would return a confidently ordered wrong answer. ` + + `Equality (\`where: { ${field}: value }\`) still works on this index. ` + + `To range over this field, let the column store adopt it: run ` + + `brain.repairIndex({ rebuild: ['metadata'] }), which rebuilds the field into ` + + `the column store, where ranges compare raw values.`, + 'INVALID_QUERY', + false + ) + } + /** * Get roaring bitmap for a field-value pair without converting to UUIDs * This is used for fast multi-field intersection queries using hardware-accelerated bitmap AND @@ -1191,8 +1267,17 @@ export class MetadataIndexManager implements MetadataIndexProvider { * value-based detection (DuckDB-inspired). Analyzes actual data values, not names. * * NO FALLBACKS - Pure value-based detection only. + * + * @param value - The value to normalize. + * @param field - Optional field name (drives the per-field statistics strategy). + * @param allowHash - Whether the >100-character escape hatch may fire. TRUE + * everywhere a normalized value is used as a KEY (equality postings, chunk + * entries, filenames) — that is what the hash exists for. FALSE on the + * ORDER-comparing path: a hash is deliberately order-destroying, so a + * bound that hashes can only be compared as nonsense. See + * {@link isHashedValue} and `getIdsFromChunksForRange`. */ - private normalizeValue(value: any, field?: string): string { + private normalizeValue(value: any, field?: string, allowHash: boolean = true): string { if (value === null || value === undefined) return '__NULL__' if (typeof value === 'boolean') return value ? '__TRUE__' : '__FALSE__' @@ -1250,21 +1335,34 @@ export class MetadataIndexManager implements MetadataIndexProvider { // Default normalization if (typeof value === 'number') return value.toString() if (Array.isArray(value)) { - const joined = value.map(v => this.normalizeValue(v, field)).join(',') + const joined = value.map(v => this.normalizeValue(v, field, allowHash)).join(',') // Hash very long array values to avoid filesystem limits - if (joined.length > 100) { + if (allowHash && joined.length > 100) { return this.hashValue(joined) } return joined } const stringValue = String(value).toLowerCase().trim() // Hash very long string values to avoid filesystem limits - if (stringValue.length > 100) { + if (allowHash && stringValue.length > 100) { return this.hashValue(stringValue) } return stringValue } + /** + * Is this normalized value a HASH rather than the value itself? + * + * {@link hashValue} is an escape hatch for filesystem name limits, and it is + * deliberately order-destroying: two values whose hashes compare one way + * routinely compare the other way themselves. Anything that ORDERS normalized + * values has to know when it is holding one, because comparing hashes yields + * a confident, wrong answer rather than an error. + */ + private static isHashedValue(normalized: string): boolean { + return normalized.startsWith('__HASH_') + } + /** * Create a short hash for long values to avoid filesystem filename limits */ diff --git a/tests/unit/utils/metadataIndex-sparse-range-collation.test.ts b/tests/unit/utils/metadataIndex-sparse-range-collation.test.ts new file mode 100644 index 00000000..d6d00568 --- /dev/null +++ b/tests/unit/utils/metadataIndex-sparse-range-collation.test.ts @@ -0,0 +1,258 @@ +/** + * @module tests/unit/utils/metadataIndex-sparse-range-collation + * @description RANGE QUERIES ON THE LEGACY SPARSE INDEX — order, or a refusal. + * Never a confidently ordered wrong answer. + * + * THE TWO RANGE PATHS. `getIdsForRange` routes a `gte` / `lt` / `between` two + * ways. The column store compares RAW values and is correct. The legacy sparse + * chunk index — the pre-7.20.0 fallback, still read for workspaces that have + * not been rebuilt — compared `normalizeValue()` output, and `normalizeValue` + * carries an escape hatch that destroys order on purpose: a string over 100 + * characters is replaced by a short hash so it can serve as a filesystem-safe + * key. Ordering hashes ranks rows by digest. + * + * THE DEFECT, IN TWO SHAPES. + * + * (a) A LONG BOUND against ordinary values. `where: { title: { gte: } }` collapsed the BOUND to `__HASH_…`, whose + * leading underscores sort below every letter — so a bound that should + * have excluded everything matched the entire field instead. This is the + * shape that reaches a caller who never stored a long value at all. + * + * (b) LONG VALUES in the index. A field whose values ran long was persisted + * hashed, so its order is not recoverable from this index at all. The old + * code compared the digests anyway and returned a subset chosen by hash. + * + * THE LAW. Bounds are normalized WITHOUT the hash escape hatch, so a long + * bound stays comparable — (a) is simply fixed. Where the persisted KEY is a + * hash, the order does not exist to be computed, and the query throws a typed + * `BrainyError('INVALID_QUERY')` naming the field and the cure — (b) is + * refused by name. Loud beats wrong. + * + * THE FIXTURE is a genuine legacy index: it is written through the same + * `ChunkManager` / `SparseIndex` doors a pre-7.20.0 engine wrote through, with + * keys normalized exactly as that engine normalized them, into a field the + * column store does not serve. The chunk WRITE path was removed in 11be039, so + * this is the only way the shape the read path exists for can be built. + * + * NOT CLAIMED HERE. The persisted keys are also lower-cased and trimmed by + * `normalizeValue`, so this path's string ranges are case-INSENSITIVE where + * the column store's are not. The raw values are not in the index to compare — + * that divergence is a property of the bytes on disk and it ends when the + * column store adopts the field. It is named in `getIdsFromChunksForRange`'s + * doc comment rather than papered over. + */ +import { describe, it, expect, beforeEach } from 'vitest' +import { Brainy } from '../../../src/brainy' +import { NounType } from '../../../src/types/graphTypes' +import { SparseIndex, ChunkManager } from '../../../src/utils/metadataIndexChunking' +import { BrainyError } from '../../../src/errors/brainyError' + +/** The field the legacy index covers — deliberately never given to a row, so + * the column store never learns it and the sparse fallback is the only path. */ +const FIELD = 'legacyTitle' + +/** + * Write a legacy sparse index for `field` exactly as a pre-7.20.0 engine did: + * one chunk, keys normalized through the index's own `normalizeValue`, ids as + * roaring bitmaps, a zone map and a bloom filter over the chunk. + * + * @param brain - The live brain whose metadata index gains the legacy field. + * @param field - Field name to index. + * @param valueToIds - Raw value → the entity ids that carried it. + */ +async function writeLegacySparseIndex( + brain: any, + field: string, + valueToIds: Array<[string, string[]]> +): Promise { + const index = brain.metadataIndex + const chunkManager: ChunkManager = index.chunkManager + const sparseIndex = new SparseIndex(field) + + // The keys a pre-7.20.0 writer persisted: normalizeValue output, hash escape + // hatch and all. This is what makes the fixture the real shape. + const chunk = await chunkManager.createChunk(field) + for (const [value, ids] of valueToIds) { + const key = index.normalizeValue(value, field) + for (const id of ids) await chunkManager.addToChunk(chunk, key, id) + } + await chunkManager.saveChunk(chunk) + + sparseIndex.registerChunk( + { + chunkId: chunk.chunkId, + field, + valueCount: chunk.entries.size, + idCount: Array.from(chunk.entries.values()).reduce((s: number, b: any) => s + b.size, 0), + zoneMap: (chunkManager as any).calculateZoneMap(chunk), + lastUpdated: Date.now(), + splitThreshold: 80, + mergeThreshold: 20 + }, + chunkManager.createBloomFilter(chunk) + ) + + await index.saveSparseIndex(field, sparseIndex) +} + +/** A deterministic string of `n` characters starting with `lead`. */ +function longString(lead: string, n: number): string { + return lead + 'x'.repeat(n - lead.length) +} + +describe('legacy sparse index: range queries order values, or refuse', () => { + let brain: Brainy + let index: any + let ids: string[] + + beforeEach(async () => { + brain = new Brainy({ requireSubtype: false, storage: { type: 'memory' } }) + await brain.init() + index = (brain as any).metadataIndex + + // Rows exist (so the id mapper can resolve them) but carry NO `legacyTitle` + // — the column store must not serve the field the pins query. + ids = [] + for (let i = 0; i < 3; i++) { + const id = `row-${i}` + await brain.add({ id, data: `row ${i}`, type: NounType.Thing, metadata: { lane: 'a' }, vector: [] }) + ids.push(id) + } + expect(index.columnStore.hasField(FIELD)).toBe(false) + }) + + describe('(a) a long BOUND against ordinary short values', () => { + // 'apple' < 'mango' < 'zebra', and every bound below is compared against + // these three raw keys. + beforeEach(async () => { + await writeLegacySparseIndex(brain, FIELD, [ + ['apple', [ids[0]]], + ['mango', [ids[1]]], + ['zebra', [ids[2]]] + ]) + }) + + it('the fixture: the values are stored raw, the long bound is what hashes', () => { + expect(index.normalizeValue('apple', FIELD)).toBe('apple') + // The bound is what the old code collapsed — and a digest sorts below + // every letter, which is exactly why `gte` matched everything. + const bound = longString('zzz', 120) + expect(index.normalizeValue(bound, FIELD)).toMatch(/^__HASH_/) + expect(index.normalizeValue(bound, FIELD) < 'apple').toBe(true) + }) + + it('gte a bound above every value matches NOTHING (it used to match all)', async () => { + const bound = longString('zzz', 120) + const matched = await index.getIdsForRange(FIELD, bound, undefined, true, true) + expect(matched).toEqual([]) + }) + + it('lte a bound above every value matches EVERY value', async () => { + const bound = longString('zzz', 120) + const matched = await index.getIdsForRange(FIELD, undefined, bound, true, true) + expect(matched).toHaveLength(3) + }) + + it('gte a long bound below every value matches every value', async () => { + const bound = longString('aaa', 120) + const matched = await index.getIdsForRange(FIELD, bound, undefined, true, true) + expect(matched).toHaveLength(3) + }) + + it('a long bound orders BETWEEN the values, not below all of them', async () => { + // 'mmm…' sits between 'mango' and 'zebra'. + const bound = longString('mmm', 120) + const matched = await index.getIdsForRange(FIELD, bound, undefined, true, true) + expect(matched).toHaveLength(1) + }) + + it('short bounds are unchanged — the ordinary case still orders correctly', async () => { + expect(await index.getIdsForRange(FIELD, 'b', undefined, true, true)).toHaveLength(2) + expect(await index.getIdsForRange(FIELD, undefined, 'n', true, true)).toHaveLength(2) + expect(await index.getIdsForRange(FIELD, 'b', 'n', true, true)).toHaveLength(1) + // Strict bounds stay strict. + expect(await index.getIdsForRange(FIELD, 'mango', undefined, false, true)).toHaveLength(1) + expect(await index.getIdsForRange(FIELD, 'mango', undefined, true, true)).toHaveLength(2) + }) + }) + + describe('(b) long VALUES — the index holds hashes, so the range is refused', () => { + beforeEach(async () => { + await writeLegacySparseIndex(brain, FIELD, [ + [longString('alpha', 140), [ids[0]]], + [longString('mike', 140), [ids[1]]], + [longString('zulu', 140), [ids[2]]] + ]) + }) + + it('the fixture: the persisted keys really are hashes', async () => { + const chunk = await index.chunkManager.loadChunk(FIELD, 0) + const keys = Array.from(chunk.entries.keys()) as string[] + expect(keys).toHaveLength(3) + for (const k of keys) expect(k).toMatch(/^__HASH_/) + // And their digest order is NOT their value order — the wrong answer the + // old code returned was wrong, not merely arbitrary. + const digestOrder = [...keys].sort() + const valueOrder = [ + index.normalizeValue(longString('alpha', 140), FIELD), + index.normalizeValue(longString('mike', 140), FIELD), + index.normalizeValue(longString('zulu', 140), FIELD) + ] + expect(digestOrder).not.toEqual(valueOrder) + }) + + it('a range over the hashed field throws a typed refusal naming the field', async () => { + await expect( + index.getIdsForRange(FIELD, longString('mike', 140), undefined, true, true) + ).rejects.toThrow(BrainyError) + + const err = await index + .getIdsForRange(FIELD, longString('mike', 140), undefined, true, true) + .catch((e: any) => e) + expect(err).toBeInstanceOf(BrainyError) + expect(err.type).toBe('INVALID_QUERY') + expect(err.message).toContain(FIELD) + expect(err.message).toContain('hash') + // The cure is named, not left to the caller to guess. + expect(err.message).toContain('repairIndex') + }) + + it('every range shape refuses — gte, lte and between alike', async () => { + const lo = longString('alpha', 140) + const hi = longString('zulu', 140) + for (const [min, max] of [ + [lo, undefined], + [undefined, hi], + [lo, hi] + ] as Array<[any, any]>) { + const err = await index.getIdsForRange(FIELD, min, max, true, true).catch((e: any) => e) + expect(err).toBeInstanceOf(BrainyError) + expect(err.type).toBe('INVALID_QUERY') + } + }) + + it('EQUALITY still works on the hashed field — only ordering is refused', async () => { + const matched = await index.getIds(FIELD, longString('mike', 140)) + expect(matched).toHaveLength(1) + }) + }) + + describe('numeric ranges on the legacy path are untouched', () => { + beforeEach(async () => { + await writeLegacySparseIndex(brain, FIELD, [ + ['5', [ids[0]]], + ['50', [ids[1]]], + ['500', [ids[2]]] + ]) + }) + + it('numbers still compare numerically, not lexicographically', async () => { + // The whole point of compareNormalizedValues: "50" < "500" numerically + // even though "500" < "50" would hold as strings by prefix. + expect(await index.getIdsForRange(FIELD, 10, undefined, true, true)).toHaveLength(2) + expect(await index.getIdsForRange(FIELD, undefined, 100, true, true)).toHaveLength(2) + expect(await index.getIdsForRange(FIELD, 10, 100, true, true)).toHaveLength(1) + }) + }) +})