Fix/decoder iterator resources - #162
Merged
Merged
Conversation
`JdbcDecoder[Option[T]]` read the column via the strict `JdbcDecoder[T]` and only then asked `rs.wasNull()`. For every reference type that is too late: the strict decoders for String, BigDecimal, Array[Byte] and Instant inspect `wasNull` themselves and throw, so the wrapper inherited the throw and a nullable VARCHAR mapped to `Option[String]` failed on its first NULL row with "Use Option[String] for nullable columns" - advice the caller had already taken. LocalDate, LocalDateTime and UUID had the mirror problem: `getObject` returns null, so a non-Option column decoded to a null rather than reporting anything. Reading the column twice is not an option - JDBC only guarantees one left-to-right read per column, and `JdbcRowDecoder` relies on that. So the null policy moves onto the typeclass instead: `decode` stays strict and `decodeOption` is the same single read with the opposite answer for NULL. The built-ins are now all built by one `strict` combinator that reads once, checks `wasNull`, and only then converts - which is also what stops `BigDecimal(_)` and `Timestamp#toInstant` NPE-ing on the sentinel a NULL read returns. `decodeOption` has a default, so a user-supplied decoder keeps working and still gets `Option` support; it needs overriding only when `decode` itself consults `wasNull`. Primitives are strict now too. `getInt` hands back 0 for a NULL, so a non-Option Int column silently decoded NULLs to zero - a summed column quietly short by however many rows were NULL. It now says which column and what to write instead, consistent with what the String decoder already claimed to do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`hasNext` compared a counter against the range's last row, while `sheetIterator` skips rows POI reports as absent. An entirely blank row inside the range - a visual separator between groups of data, which is ordinary in real spreadsheets - therefore made `hasNext` promise more rows than the iterator had, and `next()` fell off the end with a bare `java.util.NoSuchElementException: next on empty iterator`. On a sheet spanning A1:B6 with one absent row, four rows came back and then it threw. Pinned ranges are the documented recommendation for Excel, so this sat on the recommended path. `hasNext` now asks the row iterator. The range still bounds iteration, because `sheetIterator` is built from `firstRow + 1 to lastRow`, and skipping blank rows keeps reading consistent with compile time inference, which walks the range the same way. `currentRowIndex` is now read off the row rather than counted, so the row number in a decode error names the row Excel shows; counting drifted as soon as a blank row was skipped. The parsed range is memoised too - it was being re-parsed from its string form on every `hasNext` and every row. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`CSV.resource(...)` opened a `scala.io.Source`, kept its line iterator and
dropped the `Source` itself, so nothing ever closed it; `JsonTable` did the
same with its `InputStream`. The handles came back only when the JVM
noticed the objects were unreachable, which is driven by heap pressure
rather than by reading - measured here, 20k reads of a small CSV stacked up
a high water mark of ~5,200 open descriptors before any were returned,
which is already past a 1024 `ulimit -n`. On Windows the file stays locked
for just as long. `parquet` and `db` already got this right, so the two
oldest readers were the odd ones out.
Both iterators now own an optional `AutoCloseable` and close it when the
rows run out, so the ordinary shapes - `.toSeq`, `foreach`, a forced
`LazyList` - need nothing. Both are `AutoCloseable` too, for stopping early:
Using(CSV.absolutePath("big.csv"))(_.take(10).toList)
The same measurement after this change stays flat at the baseline for a
drained read and for `Using`. Abandoning a partially read iterator without
closing it still accumulates, and cannot not: `.take(10)` returns a *new*
iterator with no way to tell the original it is finished. That is what
`close` is for, and the docs now say so.
`next()` refuses to read past a close rather than returning whatever the
reader had buffered - a test caught that, not inspection.
Also closed three handles that were only released on the happy path: the
column and dense-array builders dropped theirs whenever `parseLine` met a
malformed line, `fromTyped` stranded one on each of its three header
validation failures, and the macro's own handle on the CSV was never closed
at all - one descriptor per expansion, for the lifetime of a build daemon.
ExcelWorkbookCache had a related bug of its own: when a cached workbook
failed its liveness check the replacement was never put back in the cache,
so every later call opened a fresh workbook - each holding a file handle -
and cached none of them. `compute` now does the lookup and the insert in
one step, which also closes the check-then-act race where two threads each
opened a workbook and the loser's was leaked, despite the method's
documented thread safety.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Quafadas
force-pushed
the
fix/decoder-iterator-resources
branch
from
October 1, 2026 07:27
d686874 to
2202829
Compare
The `finally` added in the previous commit started below the header line, so the two ways reading a header can fail still stranded the compiler's handle: `HeaderOptions.Auto` takes `buffered.head`, which throws on an empty CSV, and `CSVParser.parseLine` can throw on a malformed one. Those are precisely the compiles someone repeats while still getting the file right, so it was the wrong half to leave outside. The `try` now opens immediately after `Source.fromFile`. Measured, by counting handles on the CSV across the compiler's processes while expanding 40 `TypeInferrer.FirstN(5)` call sites: 40 concurrently open before the fix - one per call site, all alive at once - and 0 after. FirstN is the case that needs this most, since inference stops after five lines and leaves the reader parked mid-file with nothing to exhaust it; but FromAllRows depended on it too, because `scala.io.Source` does not close itself when its lines run out. An empty CSV still fails the build, as it should, and now holds no handle while doing so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.