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

load CSV data via COPY command #20

Merged
merged 4 commits into from Aug 12, 2021

Conversation

talerngpong
Copy link
Contributor

@talerngpong talerngpong commented Aug 6, 2021

Purposed changes:

  • use init.sql script during spinning container up via docker compose up -d
  • use copy SQL command to load CSV data to declared table. This will serve a use case of Import CSV data with a single command

@talerngpong talerngpong marked this pull request as draft August 6, 2021 08:57
@talerngpong talerngpong marked this pull request as ready for review August 6, 2021 09:31
@talerngpong
Copy link
Contributor Author

talerngpong commented Aug 6, 2021

For AMOUNT field, I think we should remove improve it (in upstream CSV) by removing middle commas in order to make it complaint with numeric type.

current: "21,905,000"
complaint version: "21905000"

@vtno
Copy link
Member

vtno commented Aug 8, 2021

@talerngpong Thank you for your contribution! This does the same thing as my PR here #19 and it also handled:

For AMOUNT field, I think we should remove improve it (in upstream CSV) by removing middle commas in order to make it complaint with numeric type.

It might be good if you can have a look at that PR and help review it. 😃

@@ -9,3 +9,6 @@ services:
- POSTGRES_PASSWORD=kaogeek
ports:
- "5432:5432"
volumes:
- ./init.sql:/docker-entrypoint-initdb.d/init.sql
Copy link
Member

@vtno vtno Aug 8, 2021

Choose a reason for hiding this comment

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

I think this solution is cleaner than my script that manually drop existing and create a new DB. It might be good if you can rebase on top of my latest changes and apply this change to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebased.

I am not sure which change I need to apply. Then I comes with questions.

  1. From my guess, does it mean that I need to remove your previous solution (like import_data and run_sql shell scripts)?
  2. This PR aims improvement of how we insert data to Postgres (by using COPY command). Would it be good whether other thing else could be fulfilled in your PR instead? (by lemme review your PR) :)

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking of just:

Suggested change
- ./init.sql:/docker-entrypoint-initdb.d/init.sql
- ./create_report_table.sql:/docker-entrypoint-initdb.d/init.sql

Does this make sense?

@vtno vtno mentioned this pull request Aug 8, 2021
3 tasks
@@ -9,3 +9,6 @@ services:
- POSTGRES_PASSWORD=kaogeek
ports:
- "5432:5432"
volumes:
- ./create_report_table.sql:/docker-entrypoint-initdb.d/init.sql
- ../output_example_v2.csv:/usr/src/output_example_v2.csv
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this as the thing we import is from here.

@vtno vtno merged commit 0e28be1 into kaogeek:csv-to-db Aug 12, 2021
@vtno
Copy link
Member

vtno commented Aug 12, 2021

@talerngpong Merged! Thanks again for your contribution.

@talerngpong
Copy link
Contributor Author

@vtno Thank you as well. Other changes you interest depend on your branch csv-to-db after this :).

@vtno
Copy link
Member

vtno commented Aug 12, 2021

@talerngpong Feel free to open another PR to master to add new features. 🙏

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

2 participants