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

Feature/trino provider timezone #35963

Merged
merged 3 commits into from
Dec 1, 2023

Conversation

romsharon98
Copy link
Contributor

This PR continue this PR: #35147
With cherry picking @duyet


^ 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.

@romsharon98 romsharon98 force-pushed the feature/trino-provider-timezone branch from d880799 to fa33eb3 Compare November 29, 2023 21:04
Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

Looks good

Copy link
Contributor

@pgagnon pgagnon left a comment

Choose a reason for hiding this comment

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

lgtm. I have a mini nitpick regarding the documentation (add a colon after "Example") but I wouldn't delay merging the PR to address this.

docs/apache-airflow-providers-trino/connections.rst Outdated Show resolved Hide resolved
@@ -156,6 +156,7 @@ def get_conn(self) -> Connection:
verify=_boolify(extra.get("verify", True)),
session_properties=extra.get("session_properties") or None,
client_tags=extra.get("client_tags") or None,
timezone=extra.get("timezone") or None,
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: May I know if we need the or None here? I thought if "timezone" is not presented, it will return None by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that we have that in other parameters as well.
@romsharon98 can you check it and if needed handle it in a followup PR?

@eladkal eladkal merged commit 1c6bbe2 into apache:main Dec 1, 2023
47 checks passed
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.

6 participants