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

[APIC-273] Add Table Columns Parameter #379

Merged
merged 12 commits into from
Apr 6, 2020
Merged

[APIC-273] Add Table Columns Parameter #379

merged 12 commits into from
Apr 6, 2020

Conversation

uttercm
Copy link
Contributor

@uttercm uttercm commented Apr 2, 2020

JIRA Ticket:
APIC-273

Summary:
Adding the table_columns parameter to io methods.

Testing:

  • Updated specs
  • Performed manual tests

Manual Tests

  • import to a pre-existing table with non-drop mode and table columns not specified Job 70574228
  • import to pre-existing table with existing_table_rows=drop and table columns not specified Job 70573579
  • import to pre-existing table with existing_table_rows=drop and table columns specified Job 70572994
  • import to non-existent table with table columns specified Job 70455477
  • multiple file import using civis_file_to_table Job 70579238

Notes:

Additional Info:

@elsander
Copy link
Contributor

elsander commented Apr 2, 2020

Tagging @salilgupta1 since Team Sparkle owns the API clients now.

@elsander elsander removed their assignment Apr 2, 2020
@elsander elsander requested review from salilgupta1 and removed request for elsander April 2, 2020 17:52
@uttercm
Copy link
Contributor Author

uttercm commented Apr 2, 2020

Tagging @salilgupta1 since Team Sparkle owns the API clients now.

Oh sorry, I forgot that switch happened.

Copy link
Contributor

@zacharydfriedlander zacharydfriedlander left a comment

Choose a reason for hiding this comment

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

A few thoughts:

civis/io/_tables.py Outdated Show resolved Hide resolved
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
civis/io/_tables.py Show resolved Hide resolved
Copy link
Contributor

@zacharydfriedlander zacharydfriedlander left a comment

Choose a reason for hiding this comment

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

These changes look good, but I want to double check a few cases before we move forward with an approval (pending approval as well from maintainers):

  • import to a pre-existing table with non-drop mode and table columns not specified - this should work fine.
  • import to pre-existing table with existing_table_rows=drop and table columns not specified - this should cause the PreprocessCsv to detect columns.
  • import to pre-existing table with existing_table_rows=drop and table columns specified - this should cause the PreprocessCsv not to detect columns.
  • import to non-existent table with table columns specified - this should also cause the PreprocessCsv not to detect columns.

We should also double check importing more than one file (with the same schema) with table) with civis_file_to_table directly to make sure that is working as expected.

@uttercm
Copy link
Contributor Author

uttercm commented Apr 3, 2020

manual tests ran added to description of the PR

civis/io/_tables.py Outdated Show resolved Hide resolved
@mheilman
Copy link
Contributor

mheilman commented Apr 3, 2020

I'm cool with this. I defer to @zacharydfriedlander for final approval

@mheilman
Copy link
Contributor

mheilman commented Apr 3, 2020

also, thanks!

civis/io/_tables.py Outdated Show resolved Hide resolved
Copy link
Contributor

@zacharydfriedlander zacharydfriedlander left a comment

Choose a reason for hiding this comment

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

LGTM!

@uttercm
Copy link
Contributor Author

uttercm commented Apr 3, 2020

sorry figured we should update the changelog

Copy link
Contributor

@mheilman mheilman left a comment

Choose a reason for hiding this comment

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

LGTM

@uttercm uttercm merged commit 0ce29f8 into civisanalytics:master Apr 6, 2020
@uttercm uttercm deleted the apic-273-table-columns-param branch April 6, 2020 12:50
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.

None yet

6 participants