Skip to content

BDMS-468: Preserve legacy empty strings in NMA transfer pipelines#390

Merged
jirhiker merged 2 commits into
stagingfrom
BDMS-468-Preserve-empty-string-values-from-the-legacy-database-during-migration-to-the-target-system-ensuring-they-are-not-converted-or-assigned-as-nulls
Jan 16, 2026
Merged

BDMS-468: Preserve legacy empty strings in NMA transfer pipelines#390
jirhiker merged 2 commits into
stagingfrom
BDMS-468-Preserve-empty-string-values-from-the-legacy-database-during-migration-to-the-target-system-ensuring-they-are-not-converted-or-assigned-as-nulls

Conversation

@jirhiker

Copy link
Copy Markdown
Member

Why

This PR addresses the following problem / context:

  • Preserve empty-string values from legacy sources during migration for specific NMA datasets.
  • Ensure blank strings are not coerced into NULLs for legacy string fields.

How

Implementation summary - the following was changed / added / removed:

  • Read targeted legacy CSVs with keep_default_na=False to keep empty strings.
  • Added blank‑safe numeric/date parsing to map empty strings to NULL only for non-string fields.
  • Added unit tests to prove "" is preserved for string fields in the specified transfers.

Notes

Any special considerations, workarounds, or follow-up work to note?

  • Full test suite: source .venv/bin/activate && set -a && source .env && set +a && pytest

Copilot AI review requested due to automatic review settings January 15, 2026 19:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR modifies NMA transfer pipelines to preserve empty strings from legacy CSV sources instead of converting them to NULL values. The changes ensure that empty-string values in string fields are retained during migration while blank strings in numeric/date fields are still correctly converted to NULL.

Changes:

  • Added keep_default_na=False parameter to CSV reading across multiple transfer modules
  • Implemented is_blank() helper functions and blank-aware numeric/date parsing to differentiate between empty strings (preserved for string fields) and blanks that should become NULL for non-string fields
  • Added comprehensive unit tests to verify empty string preservation for all affected transfer modules

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
transfers/weather_data.py Added keep_default_na=False to CSV reading
transfers/surface_water_data.py Added keep_default_na=False and blank-safe float/date parsing
transfers/radionuclides.py Added keep_default_na=False and blank-safe numeric/date parsing
transfers/minor_trace_chemistry_transfer.py Added keep_default_na=False and blank-safe float/date parsing
transfers/major_chemistry.py Added keep_default_na=False and blank-safe numeric/date parsing
transfers/hydraulicsdata.py Added keep_default_na=False and blank-safe numeric parsing with new as_float helper
transfers/chemistry_sampleinfo.py Added keep_default_na=False and blank-safe date parsing
tests/test_transfer_preserve_empty_strings.py New test file verifying empty string preservation across all transfer modules

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e96c19694c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread transfers/major_chemistry.py

@chasetmartin chasetmartin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Makes sense to me for all of these nma 1:1 transfers.

@jirhiker jirhiker merged commit ae2ac85 into staging Jan 16, 2026
6 checks passed
@TylerAdamMartinez TylerAdamMartinez deleted the BDMS-468-Preserve-empty-string-values-from-the-legacy-database-during-migration-to-the-target-system-ensuring-they-are-not-converted-or-assigned-as-nulls branch February 26, 2026 18:26
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.

3 participants