Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CIVP-24576] FIX relax SQL type checking in civis_file_to_table #439

Merged
merged 30 commits into from
Dec 13, 2021

Conversation

jacksonlee-civis
Copy link
Member

This pull request relaxes SQL type checking in civis.io.civis_file_to_table, by casting to VARCHAR when type inconsistency is detected for a given column and at least one input file has VARCHAR.

  • (For Civis employees only) Reference to a relevant ticket in the pull request title
  • Changelog entry added to CHANGELOG.md at the repo's root level
  • Description of change in the pull request description
  • If applicable, unit tests have been added and/or updated
  • The CircleCI builds have all passed

@jacksonlee-civis jacksonlee-civis added this to the v1.16.0 milestone Dec 2, 2021
Copy link
Member Author

@jacksonlee-civis jacksonlee-civis left a comment

Choose a reason for hiding this comment

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

@skomanduri Tagging you for review, since there's a small chance that this PR requires internal Platform knowledge that you'd know better than anyone else

civis/io/_tables.py Outdated Show resolved Hide resolved
civis/tests/test_io.py Show resolved Hide resolved
@skomanduri skomanduri self-assigned this Dec 9, 2021
skomanduri
skomanduri previously approved these changes Dec 9, 2021
Copy link
Contributor

@skomanduri skomanduri left a comment

Choose a reason for hiding this comment

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

This is very comprehensive!!! Just have some minor style suggestions and then one question about the inconsistent-headers logic.

civis/io/_tables.py Outdated Show resolved Hide resolved
civis/io/_tables.py Outdated Show resolved Hide resolved
civis/io/_tables.py Show resolved Hide resolved
@skomanduri skomanduri removed their assignment Dec 9, 2021
Copy link
Contributor

@skomanduri skomanduri left a comment

Choose a reason for hiding this comment

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

@jacksonlee-civis Got it. Thanks for explaining your process. I was able to do some verification myself using the same methodology and script you provided and I couldn't get this to break with the cases I tried, so I think this is good to go! I just found one small issue where the column name might not get pulled from the file but might be auto-generated instead. Feel free to code it differently if you want.

civis/io/_tables.py Outdated Show resolved Hide resolved
...rather than throwing an error for the first problematic
file found, and thereby masking potential subsequent, similar
errors and slowing down the entire work/debugging process
@jacksonlee-civis
Copy link
Member Author

@skomanduri Thank you for catching the column name issue -- this PR is ready for another pass!

Copy link
Contributor

@skomanduri skomanduri left a comment

Choose a reason for hiding this comment

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

LGTM

@jacksonlee-civis jacksonlee-civis merged commit 9980eb8 into master Dec 13, 2021
@jacksonlee-civis jacksonlee-civis deleted the civp-24576-civis-file-to-table branch December 13, 2021 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants