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

Changed conf property from str to dict in SparkSqlOperator #42835

Conversation

duhizjame
Copy link
Contributor

Changes the conf property from str to dict in SparkSqlOperator
closes: #40507

The previous PR was closed due to inactivity, I had time this week to fix it

@romsharon98
Copy link
Contributor

@duhizjame the changes looks good.
because it's a breaking change we need to bump the version of the provider (major change) and also add a breaking change caption in the change log.
after this we can merge this change.

@romsharon98
Copy link
Contributor

also what do you think about supporting both str and dict and only parse it differently behind the scene

@duhizjame
Copy link
Contributor Author

@romsharon98 maybe its better that way, so we don't break everyone on versions <= 4.8.2, I will add it to the PR tonight

regarding version changes:
from the contribution docs I cant find who is doing the the version changes; after adding str, we bump only minor version and add a changelog; should I do this or someone from the contributors is doing this after approval?

thanks for the feedback! :)

@romsharon98
Copy link
Contributor

@romsharon98 maybe its better that way, so we don't break everyone on versions <= 4.8.2, I will add it to the PR tonight

regarding version changes:

from the contribution docs I cant find who is doing the the version changes; after adding str, we bump only minor version and add a changelog; should I do this or someone from the contributors is doing this after approval?

thanks for the feedback! :)

I think that for features we need to change nothing belongs to the provider.yml, I think we have a bot that add the documentation according to the PR title/commit and the version will update by the release manager when we releasing the package.
I am not sure 100%, probably @eladkal will now better 😃

@duhizjame
Copy link
Contributor Author

I have no idea why the pipeline fails to build images, I haven't used breeze to dev/test

@romsharon98
Copy link
Contributor

I have no idea why the pipeline fails to build images, I haven't used breeze to dev/test

You have conflicts that need to be resolved.
After you resolve it we will merge.

@duhizjame duhizjame force-pushed the SparkSqlOperator_uses_string_for_conf_instead_of_dict branch from 91e1fbf to 27f29e8 Compare October 21, 2024 23:55
@romsharon98 romsharon98 merged commit 509428b into apache:main Oct 22, 2024
56 checks passed
Copy link

boring-cyborg bot commented Oct 22, 2024

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

harjeevanmaan pushed a commit to harjeevanmaan/airflow that referenced this pull request Oct 23, 2024
PaulKobow7536 pushed a commit to PaulKobow7536/airflow that referenced this pull request Oct 24, 2024
ellisms pushed a commit to ellisms/airflow that referenced this pull request Nov 13, 2024
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.

SparkSqlOperator and SparkSubmitOperator are using different types for configurations
2 participants