-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
SnowflakeToSlackOperator: Use Slack API and send result as file #24660
Conversation
@Taragolis note that we also have PrestoToSlackOperator |
template_ext: Sequence[str] = ('.sql', '.jinja', '.j2') | ||
SUPPORTED_FILE_FORMATS = ( | ||
'csv', | ||
'parquet', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think sending parquet to slack is a reasonable use case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I you ask personally me, than answer very clear "No, I don't think sending parquet to slack is a reasonable"
But users want to do some strange things and ability to send parquet cost on developing side almost nothing. This is the reason why I keep it.
@Taragolis I've just submitted a PR that will deprecate Adding the addition of a file in a generic way sounds fantastic! but maybe it's worth waiting for a review & merge to my PR first, that way - everything that is using |
c40df15
to
56b07f3
Compare
Rebased to fix selective check problem from #24665 after it's merged. |
@alexkruc Nice! You could grab whatever you think is useful from this PR. FYI I've tested manually all 3 options:
And seems it works fine. However why this PR is draft - no unit tests for new implementation, but If i wouldn't create this changes as usual keep in my laptop for couple months. @eladkal I noticed this in very late stage, which actually show me additional way how to do it in generic way. However create as part of And also seems like Slack it self a bit abandoned, even recommended Slack Api doesn't have their own connection. |
We don't have to. Alex PR is about generalizing the current operator. This PR is about adding more functionality. My suggest is to wait for Alex PR to be merged, then you can rebase and apply your changes on the new added generic operator.
but it's not.. it's SqlToSlack. Airflow can't do transfer from in memory object as operators runs on different workers so you can't split transfer action to sql -> df, df -> slack. but lets keep these comments on #24663 as it's not related to this PR :)
The operator is in slack provider. you won't need sql provider (at least not by direct relations) and currently we don't have sql provider. The PR that attempts to add it wasn't merged and even if so it doesn't move dbapi hook yet. |
I mean create base operator which do not have final implementation how to get this dataframe, something like class BasePandasDataFrameToSlackOperator:
...
def get_pandas_df(self) -> 'DataFrame':
raise NotImplementedError("You need to implement this method first")
...
def execute(self, context: 'Context') -> None:
df = self.get_pandas_df() # Better to move after some Slack messages validation
if self.filename:
self.send_file(df=df)
else:
self.send_text(context=context, df=df)
self.log.debug('Finished sending Snowflake data to Slack')
And finally everyone who want to use as base need to just implements this one, example class SnowflakeToSlackOperator(BasePandasDataFrameToSlackOperator):
...
def get_pandas_df(self):
self._validate_sql()
self.log.info('Running SQL query: %s', self.sql)
return self.snowflake_hook.get_pandas_df(self.sql, parameters=self.parameters) |
The generic operator will handle this - if implemented right there will be no need for individual classes in each provider, again if you have comments on this lets please take them to #24663 |
Close for now. If additional enhance required new PR related to SqlToSlackOperator would created |
@Taragolis now that #24663 is merged you can revisit it. |
closes: #9145