Skip to content

[AIRFLOW-XXXX] Remove unnecessary docstring in AWSAthenaOperator#9517

Merged
turbaszek merged 1 commit intoapache:masterfrom
coopergillan:AIRFLOW-XXXX-remove-now-inaccurate-doc-string
Jul 8, 2020
Merged

[AIRFLOW-XXXX] Remove unnecessary docstring in AWSAthenaOperator#9517
turbaszek merged 1 commit intoapache:masterfrom
coopergillan:AIRFLOW-XXXX-remove-now-inaccurate-doc-string

Conversation

@coopergillan
Copy link
Contributor

@coopergillan coopergillan commented Jun 25, 2020

EDIT - here is the updated PR description: query_execution_id is returned by execute, which means that the default behavior is that it will be xcom_pushed. While this can be overridden since this is a subclass of BaseOperator and this is an argument that is accepted, it need not be mentioned in the docstring here since it is referring to default behavior.

Old PR description for transparency:
Since query_execution_id is now returned by execute, it will be xcom
pushed by default, meaning do_xcom_push is no longer needed and should
not be mentioned in the docstring.


Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Unit tests coverage for changes (not needed for documentation changes)
  • Target Github ISSUE in description if exists
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

@boring-cyborg boring-cyborg bot added the provider:amazon AWS/Amazon - related issues label Jun 25, 2020
@jhtimmins
Copy link
Contributor

This change looks good. The earliest history I see for this file is in November 2019, at which point the Athena operator did not make use of do_xcom_push. It's not immediately clear if it ever did use the variable, but it isn't now.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is still valid because:

# If the task returns a result, push an XCom containing it
if task_copy.do_xcom_push and result is not None:
self.xcom_push(key=XCOM_RETURN_KEY, value=result)

Copy link
Member

Choose a reason for hiding this comment

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

If operator's execute method returns a result it doesn't mean that it will be always saved in XCom table. This will happen only if operator is used with do_xcom_push=True

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, that makes sense. This behavior seems to correspond to the super classes and not to this specific class, hence why I don't think it needs to be mentioned in the docstring. Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the setting in BaseOperator on master. Since operator docstrings usually do not mention super class functionality, I think this could be removed. The behavior should be evident by looking at the super class and understanding that query_execution_id is the value in question that will or will not be xcom_pushed.

Copy link
Member

Choose a reason for hiding this comment

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

Since operator docstrings usually do not mention super class functionality, I think this could be removed.

With that I can agree. My point was that this part of the PR description is not true:

Since query_execution_id is now returned by execute, it will be xcom
pushed by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted! Just updated the PR description and title to reflect the full context. Thanks for pointing that out!

@coopergillan coopergillan force-pushed the AIRFLOW-XXXX-remove-now-inaccurate-doc-string branch from 100d43b to 570f315 Compare July 7, 2020 15:49
@coopergillan
Copy link
Contributor Author

@turbaszek - just rebased and cleaned up the wording a bit. Apologies that I didn't see your approval first. Please let me know if it still stands.

Since query_execution_id is returned by execute, it will be xcom_pushed
by default. Since do_xcom_push is a setting in the super class,
BaseOperator, it need not be mentioned in the docstring.
@coopergillan coopergillan force-pushed the AIRFLOW-XXXX-remove-now-inaccurate-doc-string branch from 570f315 to 4dfc25b Compare July 8, 2020 02:29
@coopergillan coopergillan changed the title [AIRFLOW-XXXX] Remove now-inaccurate docstring [AIRFLOW-XXXX] Remove unnecessary docstring in AWSAthenaOperator Jul 8, 2020
@turbaszek turbaszek merged commit ecce1ac into apache:master Jul 8, 2020
@coopergillan coopergillan deleted the AIRFLOW-XXXX-remove-now-inaccurate-doc-string branch July 24, 2020 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

provider:amazon AWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments