Skip to content

Keep default parsers when only some are overridden - #178

Merged
platypii merged 1 commit into
hyparam:masterfrom
MaxFreedomPollard:fix/merge-parsers
Sep 16, 2026
Merged

platypii merged 1 commit into
hyparam:masterfrom
MaxFreedomPollard:fix/merge-parsers

Conversation

@MaxFreedomPollard

Copy link
Copy Markdown
Contributor

Fixes #175. A partial parsers object throws instead of overriding only the parsers it names. await parquetReadObjects({ file, parsers: { stringFromBytes: () => 'custom' } }) on test/files/duckdb4442.parquet fails with TypeError: parser is not a function, and the same call on test/files/uuid.parquet fails with parsers.uuidFromBytes is not a function. Any column whose type needs a parser becomes unreadable the moment a custom parser is supplied for anything else.

readRowGroup builds columnDecoder at src/rowgroup.js line 28 on master: line 32 merges { ...DEFAULT_PARSERS, ...options.parsers } and line 33 spreads ...options over it, so the caller's raw parsers replaces the merged object and the eight parsers the caller did not name are gone. convert() then reads parsers.timestampFromNanoseconds (src/convert.js line 155) off the partial object for call_date, which is INT64 TIMESTAMP with unit NANOS, and gets undefined. No existing test passes a partial parsers object through a read, so nothing tripped it.

The fix moves that one line below ...options and ...chunk.columnMetadata, so the merge is computed last, matching how assembleAsync (src/rowgroup.js line 229), parquetMetadata (src/metadata.js line 91) and readColumnIndex (src/indexes.js line 17) already merge after their parameters are bound. The type change is not optional: parsers?: ParquetParsers at src/types.d.ts lines 24 and 44 demanded all nine, so without widening it npx tsc fails on the new test with TS2740, and widening it makes those three functions hold the merged set in a local, because a reassigned Partial parameter stays Partial. prefetchPageIndexes forwards parsers straight to readColumnIndex, so only its JSDoc at src/plan.js line 298 widened. On the published types, Partial<ParquetParsers> only breaks a consumer who reads the option back out as a full ParquetParsers; ColumnDecoder.parsers at src/types.d.ts line 533 stays the full set, and the fix is what now guarantees it gets all nine. Nothing else moves: columnDecoder is built in exactly one place and chunk.columnMetadata carries no parsers key.

Verification. The new test keeps default parsers for types a custom parser does not override in test/read.test.js reads test/files/duckdb4442.parquet with parsers: { stringFromBytes: () => 'custom' } and asserts call_type is 'custom' while call_date is still new Date('2011-10-06T22:21:49.580Z'), one assertion for the override landing and one for the default surviving. It fails on master with TypeError: parser is not a function at src/convert.js line 158 and passes with the change. npx vitest run gives 664 passed in 30 files, npm run coverage is green at 92.21% statements, and npm run lint and npx tsc are clean.

readRowGroup built columnDecoder with the merged parsers first and then
spread the raw options on top, so whenever a caller passed a parsers
object it overwrote the merge and every default parser was lost. Reading
a file with a date, timestamp, UUID, geometry or JSON column then threw
"parser is not a function". Moving the merge below the two spreads makes
a partial parsers object override only the named parsers.

The public parsers option is now Partial<ParquetParsers>, since a partial
object was always the intended input but the declared type demanded all
nine. The three functions that merge the defaults now keep the merged set
in a local, because a reassigned Partial parameter stays Partial for
TypeScript. prefetchPageIndexes only needed its JSDoc widened, since it
forwards parsers straight to readColumnIndex.

@platypii platypii left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this is a better api. Thanks @MaxFreedomPollard

@platypii
platypii merged commit 8981323 into hyparam:master Sep 16, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

自定义解析器传入部分解析器而非全部时 会覆盖所有默认解析器

2 participants