Skip to content

Conversation

@gopidesupavan
Copy link
Member

@gopidesupavan gopidesupavan commented Jul 16, 2025

related #53395


^ 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 airflow-core/newsfragments.

@gopidesupavan gopidesupavan requested review from jscheffl and potiuk July 16, 2025 19:57
@gopidesupavan
Copy link
Member Author

after checking jaydebeapi there might be some drivers doesnt support the autocommit , so i think it make sense to keep that false , my bad closing this one ..

@gopidesupavan gopidesupavan deleted the fix-jdbc-unreachable-code branch July 16, 2025 21:31
@jscheffl
Copy link
Contributor

As for the future---- I still I would consider a PR makes sense to (1) add a type:ignore to suppress the warnign and (2) also add a comment why this is needed -for the future you/me that wants to fix the code.

Can you also tell me how the code is executed that the first return is not hit? Will then a driver raise an exception which is catched and then the second return is kicking-in?

@gopidesupavan
Copy link
Member Author

As for the future---- I still I would consider a PR makes sense to (1) add a type:ignore to suppress the warnign and (2) also add a comment why this is needed -for the future you/me that wants to fix the code.

Can you also tell me how the code is executed that the first return is not hit? Will then a driver raise an exception which is catched and then the second return is kicking-in?

yeah will reopen this update type ignore.

we have this custom suppression logic with suppress_and_warn(jaydebeapi.Error, jpype.JException) its getting suppressed when error raises and then the false getting returned.

@gopidesupavan
Copy link
Member Author

an example it simulates that behaviours

import traceback
import warnings
from contextlib import contextmanager


@contextmanager
def suppress_and_warn(*exceptions: type[BaseException]):
    try:
        yield
    except exceptions as e:
        warnings.warn(
            f"Exception suppressed: {e}\n{traceback.format_exc()}",
            category=UserWarning,
            stacklevel=3,
        )

def get_automcommit():
    raise Exception("get_automcommit is not implemented")

def check():

    with suppress_and_warn(Exception):
        return get_automcommit()
    return False
result = check()

if not result:
    print("Returned .", result)

@gopidesupavan gopidesupavan restored the fix-jdbc-unreachable-code branch July 16, 2025 21:40
@gopidesupavan gopidesupavan reopened this Jul 16, 2025
@gopidesupavan gopidesupavan changed the title Fix jdbc provider unreachable code Add type ignore unreachable for get_autocommit in jdbc provider Jul 16, 2025
Copy link
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

Cool!

@gopidesupavan gopidesupavan merged commit dc63c25 into apache:main Jul 17, 2025
70 checks passed
@gopidesupavan gopidesupavan deleted the fix-jdbc-unreachable-code branch July 17, 2025 06:05
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.

2 participants