Deferred non-blocking findings from the neutral review of PR #982 (perf/icebird-0.8.25-native-batches, head bf5ece2d6c111a5bccc384e86790740a796a607f). Both review rounds verdicted approve; every blocking-adjacent finding was fixed on the branch. The residuals below were classified non-blocking at triage and are deferred here rather than holding the merge.
All file:line references are against head bf5ece2d.
-
Vacuous dataType clause in schemasAreCompatible. src/core/query/union-source.js:335-341 compares field.dataType with isDeepStrictEqual, but icebird stamps every field { type: 'unknown' } (node_modules/icebird/src/sql/icebergDataSource.js), so the clause never discriminates today: two partitions whose same-named column has genuinely different physical types would still concatenate natively. Not a regression (the row path never checked types either, and the engine is value-dynamic), but the check should become real, or be annotated as latent, if icebird starts reporting concrete types.
-
Eager child prepareScan discarded on row fallback. src/core/query/union-source.js:408-425 prepares every child eagerly, then discards the prepared scans when nativeCompatible is false and rowFallbackPreparedScan takes over. icebird's prepareScan does per-request manifest and data-file pruning, so the fallback pays for a prune it never uses. Unreachable with icebird children at the current pins (residual identity always matches), so this is defensive code paying a defensive cost. Candidate: prepare lazily, or short-circuit compatibility before preparing all children.
-
Benchmark default skips the production budget wrapper. benchmarks/icebird-real-data.mjs:204 defaults --max-heap-mb to 0, and benchmarks/icebird-real-data.mjs:390 documents that 0 skips withHeapBudget entirely, so the headline table in the PR body measures the bare source stack rather than the production path. Follow-up: run and record a budgeted (--max-heap-mb > 0) comparison, and add a line to release notes wherever the bare numbers get quoted.
-
Per-deferred-read process.memoryUsage() cost is unmeasured on the budgeted path. budgetedBatch (src/core/query/sql.js:290-303) calls guard.check once per deferred column read, and guard.check samples process.memoryUsage() on every call (src/core/query/sql.js:549-556, via confirmGrowth at src/core/query/sql.js:538-547). On Linux that reads /proc/self/statm, so a wide projection over many batches adds a fixed per-read cost to the path this PR exists to speed up. Combines with item 3: the shipped benchmark never exercises this cost. Follow-up: confirm with a budgeted benchmark run that it stays in the noise, or stride the deferred-read checks the way the row loop already does (src/core/query/sql.js:169).
For completeness, one further round-1 observation was judged settled rather than deferred: the fail-closed throw for a governable prepared-only source (src/core/query/sql.js local-only gate) is the documented LLP 0294 trade-off and needs no follow-up.
Backlink: PR #982.
Deferred non-blocking findings from the neutral review of PR #982 (
perf/icebird-0.8.25-native-batches, headbf5ece2d6c111a5bccc384e86790740a796a607f). Both review rounds verdicted approve; every blocking-adjacent finding was fixed on the branch. The residuals below were classified non-blocking at triage and are deferred here rather than holding the merge.All file:line references are against head
bf5ece2d.Vacuous dataType clause in
schemasAreCompatible.src/core/query/union-source.js:335-341comparesfield.dataTypewithisDeepStrictEqual, but icebird stamps every field{ type: 'unknown' }(node_modules/icebird/src/sql/icebergDataSource.js), so the clause never discriminates today: two partitions whose same-named column has genuinely different physical types would still concatenate natively. Not a regression (the row path never checked types either, and the engine is value-dynamic), but the check should become real, or be annotated as latent, if icebird starts reporting concrete types.Eager child
prepareScandiscarded on row fallback.src/core/query/union-source.js:408-425prepares every child eagerly, then discards the prepared scans whennativeCompatibleis false androwFallbackPreparedScantakes over. icebird'sprepareScandoes per-request manifest and data-file pruning, so the fallback pays for a prune it never uses. Unreachable with icebird children at the current pins (residual identity always matches), so this is defensive code paying a defensive cost. Candidate: prepare lazily, or short-circuit compatibility before preparing all children.Benchmark default skips the production budget wrapper.
benchmarks/icebird-real-data.mjs:204defaults--max-heap-mbto 0, andbenchmarks/icebird-real-data.mjs:390documents that 0 skipswithHeapBudgetentirely, so the headline table in the PR body measures the bare source stack rather than the production path. Follow-up: run and record a budgeted (--max-heap-mb > 0) comparison, and add a line to release notes wherever the bare numbers get quoted.Per-deferred-read
process.memoryUsage()cost is unmeasured on the budgeted path.budgetedBatch(src/core/query/sql.js:290-303) callsguard.checkonce per deferred column read, andguard.checksamplesprocess.memoryUsage()on every call (src/core/query/sql.js:549-556, viaconfirmGrowthatsrc/core/query/sql.js:538-547). On Linux that reads/proc/self/statm, so a wide projection over many batches adds a fixed per-read cost to the path this PR exists to speed up. Combines with item 3: the shipped benchmark never exercises this cost. Follow-up: confirm with a budgeted benchmark run that it stays in the noise, or stride the deferred-read checks the way the row loop already does (src/core/query/sql.js:169).For completeness, one further round-1 observation was judged settled rather than deferred: the fail-closed
throwfor a governable prepared-only source (src/core/query/sql.jslocal-only gate) is the documented LLP 0294 trade-off and needs no follow-up.Backlink: PR #982.