Skip to content

Conversation

@sekikn
Copy link
Contributor

@sekikn sekikn commented May 25, 2018

Make sure you have checked all steps below.

JIRA

  • My PR addresses the following Airflow JIRA issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"

Description

  • Here are some details about my PR, including screenshots of any UI changes:

For now PostgresHook.copy_expert supports
"COPY TO" but not "COPY FROM", because it
opens a file with write mode and doesn't
commit operations. This PR fixes it by
opening a file with read and write mode
and committing operations at last.

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Fixed tests.hooks.test_postgres_hook:TestPostgresHook.test_copy_expert to confirm that the open() function is called with read and write mode and operations are committed at last.
In addition, I confirmed that the fixed version worked with both COPY FROM and COPY TO with PostgreSQL 9.5.12.

Commits

  • My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.

Code Quality

  • Passes git diff upstream/master -u -- "*.py" | flake8 --diff

For now PostgresHook.copy_expert supports
"COPY TO" but not "COPY FROM", because it
opens a file with write mode and doesn't
commit operations. This PR fixes it by
opening a file with read and write mode
and committing operations at last.
@codecov-io
Copy link

Codecov Report

Merging #3421 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3421      +/-   ##
=========================================
+ Coverage    76.6%   76.6%   +<.01%     
=========================================
  Files         203     203              
  Lines       15026   15027       +1     
=========================================
+ Hits        11510   11511       +1     
  Misses       3516    3516
Impacted Files Coverage Δ
airflow/hooks/postgres_hook.py 92.59% <100%> (+0.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba84b6f...dabf1b9. Read the comment docs.

Copy link

@jgao54 jgao54 left a comment

Choose a reason for hiding this comment

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

lgtm thanks!

@asfgit asfgit merged commit dabf1b9 into apache:master May 25, 2018
asfgit pushed a commit that referenced this pull request May 25, 2018
@sekikn
Copy link
Contributor Author

sekikn commented May 26, 2018

Thanks for reviewing and committing @jgao, but really sorry, I've just noticed I had a mistake.
Opening a file with 'w+' mode truncates it first, so COPY FROM breaks the input file and loads no data. My test was incomplete.

I'll send an additional PR soon, so would you take a look? I'll add a new parameter to PostgresHook.copy_expert for specifying file open mode.
It might be possible to decide file opening mode by parsing sql, but I don't think it's a good idea, since naive implementation (using regex, for example) is flaky.

sekikn added a commit to sekikn/airflow that referenced this pull request Jun 2, 2018
The previous commit on this issue (apache#3421)
introduced a new bug on COPY FROM operation.
This PR fixes it by opening a file with 'r+'
mode instead of 'w+' to avoid truncating it.
asfgit pushed a commit that referenced this pull request Jun 3, 2018
The previous commit on this issue (#3421)
introduced a new bug on COPY FROM operation.
This PR fixes it by opening a file with 'r+'
mode instead of 'w+' to avoid truncating it.

Closes #3423 from sekikn/AIRFLOW-2525-2
aliceabe pushed a commit to aliceabe/incubator-airflow that referenced this pull request Jan 3, 2019
The previous commit on this issue (apache#3421)
introduced a new bug on COPY FROM operation.
This PR fixes it by opening a file with 'r+'
mode instead of 'w+' to avoid truncating it.

Closes apache#3423 from sekikn/AIRFLOW-2525-2
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.

4 participants