From 6f8b17b9d4bb6660ce8e1dbc48159fe62bf6662c Mon Sep 17 00:00:00 2001 From: Kenny Daniel Date: Wed, 7 Oct 2026 22:42:02 -0700 Subject: [PATCH] Fix Unicode pruning and update dependencies --- package.json | 8 ++--- src/prune.js | 30 +++++++++++++---- test/prune.bounds.test.js | 17 ++++++++-- test/prune.test.js | 5 +-- test/sql/unicodePruning.test.js | 60 +++++++++++++++++++++++++++++++++ 5 files changed, 106 insertions(+), 14 deletions(-) create mode 100644 test/sql/unicodePruning.test.js diff --git a/package.json b/package.json index 45df359..fb81c8d 100644 --- a/package.json +++ b/package.json @@ -53,9 +53,9 @@ "test": "vitest run" }, "dependencies": { - "hyparquet": "1.31.2", + "hyparquet": "1.31.3", "hyparquet-compressors": "1.1.2", - "hyparquet-writer": "0.16.10", + "hyparquet-writer": "0.16.11", "squirreling": "0.16.8" }, "peerDependencies": { @@ -67,12 +67,12 @@ } }, "devDependencies": { - "@aws-sdk/credential-providers": "3.1146.0", + "@aws-sdk/credential-providers": "3.1147.0", "@eslint/js": "10.0.1", "@types/node": "26.6.4", "@vitest/coverage-v8": "5.0.3", "eslint": "10.12.0", - "eslint-plugin-jsdoc": "65.1.0", + "eslint-plugin-jsdoc": "65.2.0", "globals": "17.13.0", "typescript": "7.0.2", "vitest": "5.0.3" diff --git a/src/prune.js b/src/prune.js index 4912ff6..f62b493 100644 --- a/src/prune.js +++ b/src/prune.js @@ -1,6 +1,6 @@ import { typeName } from './schema.js' import { applyTransform, transformResultType } from './write/transform.js' -import { compare, compareStringsCodePoint, deserializeValue } from './write/serde.js' +import { compare, deserializeValue } from './write/serde.js' /** * Partition-level scan pruning. Given a hyparquet query filter (keyed by @@ -81,6 +81,7 @@ export function manifestMightMatch(filter, manifest, schema, metadata) { */ function summaryMightMatch(op, value, summary, transform, sourceType) { if (!summary) return true + if (unsafeStringRange(op, value, sourceType)) return true const kind = transformKind(transform) if (kind === 'other') return true /** @type {IcebergType} */ @@ -464,10 +465,9 @@ function equals(a, b) { /** * Order two partition values. Returns -1/0/1, or undefined when the values are - * not safely orderable. Strings order by code point, which matches Iceberg's - * UTF-8 byte order (JS `<` compares UTF-16 code units and disagrees, which is - * why they were previously not ordered here). Booleans are intentionally not - * ordered, so range predicates on them never prune. + * not safely orderable. These are actual partition values, not statistical + * bounds, so strings use the same UTF-16 order as SQL and row filtering. + * Booleans are intentionally not ordered, so range predicates never prune. * * @param {any} a * @param {any} b @@ -482,7 +482,7 @@ function compareOrder(a, b) { if (aDate && bDate) return sign(a.getTime() - b.getTime()) if (typeof a === 'bigint' && typeof b === 'bigint') return a < b ? -1 : a > b ? 1 : 0 - if (typeof a === 'string' && typeof b === 'string') return compareStringsCodePoint(a, b) + if (typeof a === 'string' && typeof b === 'string') return a < b ? -1 : a > b ? 1 : 0 const na = numericOf(a) const nb = numericOf(b) @@ -618,6 +618,7 @@ function isOrderableForBounds(type) { * @returns {boolean} */ function boundsOpMightMatch(op, value, lo, hi, type) { + if (unsafeStringRange(op, value, type)) return true switch (op) { case '$lt': { // need some x < value; smallest is lo. Skip if lo >= value. @@ -654,6 +655,23 @@ function boundsOpMightMatch(op, value, lo, hi, type) { } } +/** + * Iceberg string bounds use UTF-8 order, while query comparisons use UTF-16. + * A non-ASCII range literal can order differently relative to values inside + * those bounds, even when neither endpoint contains a surrogate pair. Keep + * such ranges, as hyparquet does for Parquet statistics. ASCII range literals + * order consistently in both encodings; equality and IN still use UTF-8 bounds. + * + * @param {string} op + * @param {any} value + * @param {IcebergType} type + * @returns {boolean} + */ +function unsafeStringRange(op, value, type) { + return typeName(type) === 'string' && ['$lt', '$lte', '$gt', '$gte'].includes(op) && + (typeof value !== 'string' || /[\u0080-\uffff]/.test(value)) +} + /** * Whether `value` could lie within [lo, hi] (open sides allowed). Keep on any * undecidable comparison. diff --git a/test/prune.bounds.test.js b/test/prune.bounds.test.js index 0860ad5..b1df0b6 100644 --- a/test/prune.bounds.test.js +++ b/test/prune.bounds.test.js @@ -228,16 +228,29 @@ describe('fileMightMatch — string bounds (byte-ordered family)', () => { expect(fileMightMatch({ name: { $eq: 'aaaa' } }, t, schema)).toBe(true) }) - it('orders by code point (UTF-8 byte order), not UTF-16 code units', () => { + it('uses UTF-8 bounds for equality but keeps uncertain Unicode ranges', () => { // U+E000 sorts below U+10000 in code point/UTF-8 order but above its // surrogate pair in UTF-16 unit order. A file bounded at ['a', U+E000] // cannot contain U+10000; UTF-16 ordering would wrongly keep it. const u = entry({ 2: { min: 'a', max: '\uE000', type: 'string' } }) expect(fileMightMatch({ name: { $eq: '\u{10000}' } }, u, schema)).toBe(false) - expect(fileMightMatch({ name: { $gt: '\uE000' } }, u, schema)).toBe(false) + expect(fileMightMatch({ name: { $gt: '\uE000' } }, u, schema)).toBe(true) expect(fileMightMatch({ name: { $eq: '\uE000' } }, u, schema)).toBe(true) }) + it('keeps matching Unicode ranges inside mixed UTF-8 bounds', () => { + // U+E000 matches > U+10000 in SQL, even though the UTF-8 upper bound + // is below U+10000. The reverse ordering also affects < predicates. + const bmp = entry({ 2: { min: 'a', max: '\uFFFF', type: 'string' } }) + const supplementary = entry({ 2: { min: '\u{10000}', max: '😀', type: 'string' } }) + expect(fileMightMatch({ name: { $gt: '\u{10000}' } }, bmp, schema)).toBe(true) + expect(fileMightMatch({ name: { $gte: '\u{10000}' } }, bmp, schema)).toBe(true) + expect(fileMightMatch({ name: { $lt: '\uE000' } }, supplementary, schema)).toBe(true) + expect(fileMightMatch({ name: { $lte: '\uE000' } }, supplementary, schema)).toBe(true) + expect(fileMightMatch({ name: { $lt: 'a' } }, supplementary, schema)).toBe(false) + expect(fileMightMatch({ name: { $eq: '\u{10000}' } }, bmp, schema)).toBe(false) + }) + it('a mismatched literal type keeps the file', () => { expect(fileMightMatch({ name: { $eq: 42 } }, e, schema)).toBe(true) expect(fileMightMatch({ name: { $gt: 42 } }, e, schema)).toBe(true) diff --git a/test/prune.test.js b/test/prune.test.js index b9cab89..2157ae3 100644 --- a/test/prune.test.js +++ b/test/prune.test.js @@ -63,16 +63,17 @@ describe('partitionMightMatch — identity', () => { expect(partitionMightMatch({ id: { $nin: [1n] } }, entry({ id: 5 }), schema, m)).toBe(true) }) - it('prunes string identity on equality and ranges (code-point order)', () => { + it('prunes string identity on equality and ranges using query ordering', () => { const sm = meta([{ 'source-id': 2, 'field-id': 1000, name: 'name', transform: 'identity' }]) expect(partitionMightMatch({ name: { $eq: 'a' } }, entry({ name: 'a' }), schema, sm)).toBe(true) expect(partitionMightMatch({ name: { $eq: 'a' } }, entry({ name: 'b' }), schema, sm)).toBe(false) - // Strings order by code point (UTF-8 byte order), so ranges prune too. // Identity is exact: every row equals the partition value, so $gt 'a' // on a file whose value IS 'a' proves no row can match. expect(partitionMightMatch({ name: { $gt: 'a' } }, entry({ name: 'a' }), schema, sm)).toBe(false) expect(partitionMightMatch({ name: { $gt: 'a' } }, entry({ name: 'b' }), schema, sm)).toBe(true) expect(partitionMightMatch({ name: { $lt: 'a' } }, entry({ name: 'b' }), schema, sm)).toBe(false) + expect(partitionMightMatch({ name: { $gt: '\u{10000}' } }, entry({ name: '\uE000' }), schema, sm)).toBe(true) + expect(partitionMightMatch({ name: { $lt: '\uE000' } }, entry({ name: '\u{10000}' }), schema, sm)).toBe(true) }) it('prunes a null partition value only on equality with a concrete literal', () => { diff --git a/test/sql/unicodePruning.test.js b/test/sql/unicodePruning.test.js new file mode 100644 index 0000000..b2f9b92 --- /dev/null +++ b/test/sql/unicodePruning.test.js @@ -0,0 +1,60 @@ +import { collect, executeSql } from 'squirreling' +import { describe, expect, it } from 'vitest' +import { fileCatalog } from '../../src/catalog/file.js' +import { icebergDataSource } from '../../src/sql/icebergDataSource.js' +import { icebergQuery } from '../../src/sql/icebergQuery.js' +import { icebergAppend, icebergCreateTable, icebergRewriteManifests } from '../../src/write/write.js' +import { memResolver } from '../helpers.js' + +/** @import {Schema} from '../../src/types.js' */ + +/** @type {Schema} */ +const schema = { + type: 'struct', 'schema-id': 0, + fields: [{ id: 1, name: 'name', type: 'string', required: true }], +} + +// UTF-8 orders supplementary characters above the BMP; JavaScript places +// their surrogate pairs below U+E000. Prefixes exercise truncate projection. +const values = ['a', 'z', '\uD7FF', '\uE000', '\uFFFF', '\u{10000}', '😀', 'a\uE000', 'a😀'] +const records = values.map(name => ({ name })) + +describe('Unicode pruning matches SQL string comparisons', () => { + it.each(['unpartitioned', 'identity', 'truncate[1]'])('preserves filtered rows and counts with %s partitions', async transform => { + const { resolver, lister } = memResolver() + const catalog = fileCatalog({ resolver, lister, conditionalCommits: true }) + const tableUrl = `http://test/unicode-${transform}` + await icebergCreateTable({ + catalog, tableUrl, schema, + partitionSpec: { + 'spec-id': 0, + fields: transform === 'unpartitioned' ? [] : [{ 'source-id': 1, 'field-id': 1000, name: 'part', transform }], + }, + properties: { 'commit.manifest-merge.enabled': 'false' }, + }) + // Exercise both singleton and mixed Unicode bounds in data files, + // then combine the manifests to exercise mixed partition summaries too. + await icebergAppend({ catalog, tableUrl, records: [records[3]] }) + const before = await icebergAppend({ catalog, tableUrl, records: records.filter((_, i) => i !== 3) }) + const after = await icebergRewriteManifests({ catalog, tableUrl }) + for (const metadata of [before, after]) { + const source = await icebergDataSource({ tableUrl, metadata, resolver }) + for (const value of values) { + for (const op of ['<', '<=', '>', '>=', '=', 'IN']) { + const literal = `'${value}'` + const where = `WHERE name ${op} ${op === 'IN' ? `(${literal})` : literal}` + for (const select of ['name', 'COUNT(*) AS n']) { + const query = `SELECT ${select} FROM t ${where}` + const expected = await collect(await executeSql({ query, tables: { t: records } })) + const legacy = { columns: source.columns, scan: source.scan, scanColumn: source.scanColumn } + for (const t of [source, legacy]) { + const actual = await collect(await icebergQuery({ query, tables: { t } })) + expect(actual.sort((a, b) => String(a.name).localeCompare(String(b.name))), query) + .toEqual(expected.sort((a, b) => String(a.name).localeCompare(String(b.name)))) + } + } + } + } + } + }) +})