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

Add sample DAG, test ExasolToS3Operator, and update documentation (#22632) #47478

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jieyao-MilestoneHub
Copy link
Contributor

@jieyao-MilestoneHub jieyao-MilestoneHub commented Mar 7, 2025

Summary

Adds documentation for ExasolToS3Operator to enhance clarity and usability.

Changes Introduced

  • New Documentation: exasol_to_s3.rst (Setup guide & example usage)

Related Issues

related: #22632

Copy link
Contributor

@vincbeck vincbeck left a comment

Choose a reason for hiding this comment

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

Those files are empty

@jieyao-MilestoneHub jieyao-MilestoneHub force-pushed the feature-exasol-sample-dag branch from afc543f to a6e6146 Compare March 8, 2025 06:58
@jieyao-MilestoneHub
Copy link
Contributor Author

Hey @vincbeck, I’d love to get your thoughts on this.

I’ve tested the connection setup, and this PR mainly focuses on documentation. Adding a test file could help validate the setup better.

While working on example_exasol_to_s3.py, I noticed exasol_to_s3 could be refactored for clearer separation of concerns, making testing easier. Thinking of including this in the PR—does that sound good to you?

I’ll adjust the docs as needed once we decide. Looking forward to your thoughts! 🚀

@vincbeck
Copy link
Contributor

Hey @vincbeck, I’d love to get your thoughts on this.

I’ve tested the connection setup, and this PR mainly focuses on documentation. Adding a test file could help validate the setup better.

While working on example_exasol_to_s3.py, I noticed exasol_to_s3 could be refactored for clearer separation of concerns, making testing easier. Thinking of including this in the PR—does that sound good to you?

I’ll adjust the docs as needed once we decide. Looking forward to your thoughts! 🚀

That sounds perfect to me :)

@jieyao-MilestoneHub
Copy link
Contributor Author

hanks for your feedback, @vincbeck! 😊

Glad to hear you’re on board with the changes. I’ll add the test file and refactor exasol_to_s3 for better separation of concerns and easier testing.

I’ll also update the docs accordingly—let me know if you have any suggestions! 🚀

jiao added 3 commits March 11, 2025 15:32
This commit adds a detailed Exasol connection setup guide for Apache Airflow, outlining the configuration steps, prerequisites, and troubleshooting tips. The document is structured in reStructuredText (.rst) format for better readability and integration with documentation tools.

Requires review to ensure clarity and completeness.
- Optimized `ExasolToS3Operator` to separate Exasol and S3 operations.
- Improved unit tests for `ExasolExportOperator`, `S3UploadOperator`, and `ExasolToS3Operator`.
- Added `example_exasol_to_s3` DAG to validate Exasol connection.
- Created `exasol_connection.rst` with setup instructions, avoiding credit card requirement.
@jieyao-MilestoneHub jieyao-MilestoneHub force-pushed the feature-exasol-sample-dag branch from a6e6146 to b75b646 Compare March 11, 2025 07:32
@jieyao-MilestoneHub
Copy link
Contributor Author

Hey @vincbeck, thanks for your input!

Just wanted to confirm—if this kind of introduction (how to test the Exasol connection without needing a credit card) is what you were expecting. If so, do you have any thoughts on where it would be best placed?

Let me know what you think! 🚀

tags=["exasol", "test"],
) as dag:

test_exasol_connection = ExasolExportOperator(
Copy link
Contributor

Choose a reason for hiding this comment

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

test_exasol_connection does not describe well what this operator does

Comment on lines +21 to +24
This DAG uses the ExasolToS3Operator to execute a simple SQL query on Exasol
and simulate an S3 upload. The main purpose is to verify that the Exasol connection
settings (exasol_default) are working correctly. S3 connection is simulated via dummy
parameters (aws_default, dummy bucket and key).
Copy link
Contributor

Choose a reason for hiding this comment

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

An example DAG is a DAG that should work out of the box (if you have valid credentials of course). I would rephrase this comment and just mention what this DAG is doing. This DAG should work for someone who has exasol and AWS permissions.

@@ -0,0 +1,152 @@
.. Licensed to the Apache Software Foundation (ASF) under one
Copy link
Contributor

Choose a reason for hiding this comment

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

There should no doc in example_dags folder. This kind of documentation should go in providers/exasol/docs

@@ -0,0 +1,174 @@
.. _exasol_airflow_setup:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not duplicate documentation. Plus, this documentation is related to Exasol, it has nothing to do with Amazon. As I mentioned in my previous comment, this should go in Exasol documentation

@@ -31,29 +32,165 @@
from airflow.utils.context import Context


class ExasolExportOperator(BaseOperator):
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have ExasolToS3Operator, are you sure we need this operator as well?

raise


class S3UploadOperator(BaseOperator):
Copy link
Contributor

Choose a reason for hiding this comment

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

S3CreateObjectOperator already exists

def execute(self, context: Context):
exasol_hook = ExasolHook(exasol_conn_id=self.exasol_conn_id)
s3_hook = S3Hook(aws_conn_id=self.aws_conn_id)
def execute(self, context: Context) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why modifying the implementation? Did you notice anything wrong with the previous implementation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:amazon AWS/Amazon - related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants