Skip to content

Conversation

@khalidmammadov
Copy link
Contributor

Part of #19891

  • Fixed type of a field in a sub-class as super class was changed
  • Applied tuple conversion to adhere to the type of the field: Sequence[str]

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
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.

)

template_ext: Iterable[str] = ('.txt',)
template_ext: Sequence[str] = ('.txt',)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should have been Collection instead of Sequence? Or does anything in Airflow requires these to be deterministically ordered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This field is defined in the base class as Sequence. It used to be Iterable and was changed here. I changed it to match base class and not sure if order here metters at all. BTW, it was suggested by you :) #20034 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Yep. I already implemented Sequence for ALL operators in #20571 :).

Copy link
Member

Choose a reason for hiding this comment

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

And #20608

Copy link
Member

@potiuk potiuk Jan 3, 2022

Choose a reason for hiding this comment

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

So better not change it again - unless we have a good reason to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for below set-tuple conversion is to remove any duplicates and hence set with union operator was used prior to conversion. It's similar to one in here:

template_fields: Sequence[str] = tuple(
set(QuboleOperator.template_fields) | set(SQLCheckOperator.template_fields)
)

Well, it also means that if we dont need dups then Set type would be more appropriate

Copy link
Member

Choose a reason for hiding this comment

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

If we choose another protocol - fine with me. That''s yet andother 280 files change :) .

Anyone happy to do 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.

I can change them to Set type if we are in an agreement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a caution I will also need to change the type of values it (template_fields) gets assigned to, which are mostly tuple and will become all set. Also documentation needs to be amended as there were no much emphasis on the type of these fields and list also were used: https://airflow.apache.org/docs/apache-airflow/stable/concepts/operators.html#jinja-templating
This means we also need to notify users about this change.
Alternatively, we can leave them as is and let users to use list, tuple etc. to provide simplicity of use and internally carry on removing duplicates as they (dups) dont make any sense. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I’d say let’s keep Sequence and do the mass-change later. That makes the commit history easier to reason with.

@kazanzhy
Copy link
Contributor

kazanzhy commented Jan 3, 2022

Looks good. Here the first try to fix Qubole
#20319

@potiuk potiuk merged commit 9f135bc into apache:main Jan 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants