Make partitioned parquet reading deterministic #4739
Conversation
This ensures that the firstEntryPath is the lexicographically first entry. Fixes deephaven#4738
devinrsmith
added
bug
| @@ -571,11 +572,13 @@ private static Table readTableInternal( | |||
| } | |||
| final Path firstEntryPath; | |||
| try (final DirectoryStream<Path> sourceStream = Files.newDirectoryStream(sourcePath)) { | |||
| final Iterator<Path> entryIterator = sourceStream.iterator(); | |||
| if (!entryIterator.hasNext()) { | |||
| // Lexicographical comparison | |||
There was a problem hiding this comment.
Two things to raise here:
- @malhotrashivam take note, this may be a conflict with your PR
- @devinrsmith / @malhotrashivam I think we might prefer to take the max, instead of the min. If we think lexicographic order matters, it's often the case that there is a sortable timestamp (e.g. YYYY-MM-DD-blah) in the name. Taking the most recent gives us a better guess at the intended schema.
There was a problem hiding this comment.
So, the actual inference step is separate from this outer layer (readPartitionedTableInferSchema). My goal here was to make the existing behavior more deterministic; in this case, I think taking the min entry more closely matches the intentions and probable behavior of the existing code.
There was a problem hiding this comment.
This doesn't make anything more deterministic, unless you read the DirectoryStream JavaDocs with no context. Directories have an order on every file system I'm aware of.
|
I'm actually doubting my change is complete looking back at it - it is only setting us up deterministically for ParquetKeyValuePartitionedLayout vs ParquetFlatPartitionedLayout, but not how those work internally. I'm going to take another pass at this soon. |
|
I think there is still value in merging this PR to make things more deterministic, but it doesn't actually fix the inference step. Will leave that for later. |
|
I think the change does what's it supposed to do. Do you think we should add a tiny test for this? |
|
Closed in favor of more deterministic inference in #4783 |
This ensures that the firstEntryPath is the lexicographically first entry.
Part of #4738