Skip to content

gtfsdb: compute batch size via SQLite limit and fail fast on invalid …#528

Merged
aaronbrethorst merged 1 commit intoOneBusAway:mainfrom
Adityatorgal17:followup/safe-batch-size
Mar 1, 2026
Merged

gtfsdb: compute batch size via SQLite limit and fail fast on invalid …#528
aaronbrethorst merged 1 commit intoOneBusAway:mainfrom
Adityatorgal17:followup/safe-batch-size

Conversation

@Adityatorgal17
Copy link
Copy Markdown
Contributor

Summary

Follow-up to #517 addressing review feedback.

Changes

  • Remove BulkInsertBatchSize field and DefaultBulkInsertBatchSize constant
  • Compute batch size via SafeBatchSize using SQLite variable limit (32766)
  • Add stopTimeFieldsPerRow=10 and shapeFieldsPerRow=5 constants for clarity
  • Panic on fieldsPerRow <= 0 (programmer error)
  • Update stale comments in bulkInsertShapes
  • Update TestSafeBatchSize to assert panic for invalid input

Notes

fieldsPerRow is a compile-time constant at call sites, so invalid values
represent programmer error and should fail fast.

Follow-up to #517.

Copy link
Copy Markdown
Member

@aaronbrethorst aaronbrethorst left a comment

Choose a reason for hiding this comment

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

Hey Aditya, this is a really clean follow-up. Removing the dead BulkInsertBatchSize/DefaultBulkInsertBatchSize code, switching to a panic for what can only ever be a programmer error, and extracting the magic numbers into named constants — each change makes the code clearer and more honest about its invariants. I verified that stopTimeFieldsPerRow = 10 and shapeFieldsPerRow = 5 both match their respective INSERT column lists, placeholder counts, and args-per-row exactly. The updated batch size comment in bulkInsertShapes is also accurate (50 * 6553 = ~327k).

There's nothing left to change — this PR is ready to merge. Nice work keeping the follow-up focused and tidy.

@aaronbrethorst aaronbrethorst merged commit f96a6a0 into OneBusAway:main Mar 1, 2026
4 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