Repository navigation
Conversation
|
Review changes with Changed Files |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open issues
What changed in this PR
Fixes writeKeyValuePartitionedTable crashing on empty inputs by making empty key-value partitioned writes a no-op and tightening internal preconditions.
Changes:
- Add early return in
writeKeyValuePartitionedTableImplwhen there are no destinations/partitions to write. - Add
Require.gtZero(destinations.length)inwriteTablesImplto preventdestinations[0]access on empty arrays. - Add/extend tests and Javadocs documenting the empty-write behavior and directory-creation semantics.
| File | Description |
|---|---|
| extensions/parquet/table/src/test/java/io/deephaven/parquet/table/ParquetTableReadWriteTest.java | Adds regression tests covering empty key-value partitioned writes, metadata-file behavior, and overwriting semantics. |
| extensions/parquet/table/src/main/java/io/deephaven/parquet/table/ParquetTools.java | Implements no-op behavior for empty destination sets, documents empty-write contract, and adds a defensive precondition in writeTablesImpl. |
Review effort: Lite
No docs changes detected for 67f6a43 |
Test coverage of changes vs
|
| File | Lines | Branches | Changed lines | |
|---|---|---|---|---|
| mod | ParquetTools |
422/445 (94%) | 174/193 (90%) | 3/3 (100%) |
All new and modified files
0 new, 1 modified — lines 422/445 (94%), branches 174/193 (90%)
All new and modified lines
3 executable lines added or changed — 3/3 (100%) covered
Gaps
Every executable line the branch changed runs under the existing tests: the early return for a partitioned table with no constituents, with and without metadata files and over an existing directory, and the new Require.gtZero precondition in writeTablesImpl. The only path not taken is the precondition's failure branch, a defensive guard that no public entry point can reach: the key-value write now returns before calling writeTablesImpl with zero destinations, and the public writeTables already rejects an empty array up front.
Measured at 67f6a43e3 against main, over test and testOutOfBand in :extensions-parquet-table.
ParquetTools.writeKeyValuePartitionedTablethrew anArrayIndexOutOfBoundsExceptionwith no useful message when the source table had no rows, or the partitioned table had no constituents.writeKeyValuePartitionedTableImplbuilds one destination per constituent, an empty table yields no constituents, andwriteTablesImplpicks its channels provider fromdestinations[0]. The pushdown fuzzer (DH-23557, finding 4) found the crash.Changes
writeKeyValuePartitionedTableImplreturns before callingwriteTablesImplwhen there are no destinations. Nothing is written, not even the metadata files requested bygenerateMetadataFiles, and the destination directory is not created.writeTablesImplnow requires at least one destination (Require.gtZero), so a zero-length destination array can no longer reachdestinations[0]through any caller.writeKeyValuePartitionedTableoverloads documents the empty case, and the "Non-existing directories are created" promise now reads "when there is data to write".Why a no-op rather than an exception
An empty table is a normal state: a day with no rows in a pipeline that partitions by day, or a
PartitionedTableafter an upstream filter. Throwing would force every caller to add an emptiness check, and a caller that forgets would fail on the first quiet day. An empty write over a populated directory leaves the existing data intact.Not in this PR
Reading the empty result back still needs a
TableDefinitionin the read instructions, because the key-value layout records the schema only in the data files. DH-23638 follows up for the metadata-files case by writing schema-only_metadataand_common_metadatafiles.