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

Fix rendering the mapped parameters when using expand_kwargs method #32272

Merged
merged 9 commits into from
Aug 18, 2023

Conversation

hussein-awala
Copy link
Member

@hussein-awala hussein-awala commented Jun 29, 2023

closes: #32260

This PR fixes a bug in the mapped operator when using expand_kwargs method.

In the ListOfDictsExpandInput class, the resolve method doesn't filter out string values from the resolved_oids list as we do in the class DictOfListsExpandInput. Consequently, these strings are incorrectly regarded as resolved, even though they are not instances from MappedArgument or XComArg, and their values still match those provided by the user.

This PR removes the literal str values from the list resolved_oids to let the BaseOperator class resolve them.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@hussein-awala
Copy link
Member Author

I have tested this with a testing DAG and confirmed that it works as expected. However, before removing the WIP status, it should also be tested with a taskflow DAG to ensure its compatibility and stability.

@uranusjr
Copy link
Member

Tests disagree 🙂

_expand_mapped_kwargs performs some rendering, and the returned OIDs record those already-performed rendering and prevent double-rendering. This broke the mechanism.

@hussein-awala
Copy link
Member Author

_expand_mapped_kwargs performs some rendering, and the returned OIDs record those already-performed rendering and prevent double-rendering. This broke the mechanism.

@uranusjr According to the failed test, we should use the XCom data as it is without resolving it via Jinja. In this case, we cannot consider the reported issue as a bug, because the task result is shared using XCom, and not resolving this result is an expected behavior.

However, supporting passing templates via XCom especially for the mapped operator is a nice feature IMO, for that, I propose adding a new parameter template_in_template to the expand method to tell Airflow whether to resolve the resolved parameters or not.

WDYT?

@hussein-awala hussein-awala changed the title [WIP] Fix rendering the mapped parameters in the mapped operator Fix rendering the mapped parameters in the mapped operator Aug 8, 2023
@uranusjr
Copy link
Member

uranusjr commented Aug 9, 2023

Isn’t the original issue about passing a template as a string literal? That sounds like a reasonable thing, and doesn’t involve XCom.

@hussein-awala
Copy link
Member Author

@uranusjr, I discovered that the issue is just related to the expand_kwargs method. In the ListOfDictsExpandInput class, the resolve method doesn't filter out string values from the resolved_oids list as we do in the class DictOfListsExpandInput. Consequently, these strings are incorrectly regarded as resolved, even though they are not instances from MappedArgument or XComArg, and their values still match those provided by the user.

Could you take another look?

For the new feature "template_in_template" I proposed, I will open a new PR to discuss its idea.

@hussein-awala hussein-awala changed the title Fix rendering the mapped parameters in the mapped operator Fix rendering the mapped parameters when using expand_kwargs method Aug 9, 2023
@eladkal eladkal added this to the Airflow 2.7.1 milestone Aug 14, 2023
@eladkal eladkal added the type:bug-fix Changelog: Bug Fixes label Aug 14, 2023
@hussein-awala hussein-awala merged commit d1e6a5c into apache:main Aug 18, 2023
42 checks passed
ephraimbuddy pushed a commit that referenced this pull request Aug 28, 2023
…#32272)

* Fix rendering the mapped parameters in the mapped operator

Signed-off-by: Hussein Awala <[email protected]>

* add template_in_template arg to expand method to tell Airflow whether to resolve the xcom data or not

* fix dag serialization tests

* Revert "fix dag serialization tests"

This reverts commit 191351c.

* Revert "add template_in_template arg to expand method to tell Airflow whether to resolve the xcom data or not"

This reverts commit 14bd392.

* Fix ListOfDictsExpandInput resolve method

* remove _iter_parse_time_resolved_kwargs method

* remove unnecessary step

---------

Signed-off-by: Hussein Awala <[email protected]>
(cherry picked from commit d1e6a5c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Apparently the Jinja template does not work when using dynamic task mapping with SQLExecuteQueryOperator
3 participants