enhance druid connection parameters #35244

blcksrx · 2023-10-28T19:48:49Z

Enhancement on this PR:

Support all the other connection parameters that exists on the pydruid.connect in the get_conn method.
Connections documentation
Fix return values

Note:
The issue mentions about get_pandas_df method also. Since this is a DBAPIHook and the connection uses PEP8 DB API, there should be no problem to not override it from the DBAPIHook.
In addition, back in 2019 I wrote a test function for this specific method:

airflow/tests/providers/apache/druid/hooks/test_druid.py

Lines 292 to 305 in 3d7c8f7

    
           def test_get_pandas_df(self): 
        
               statement = "SQL" 
        
               column = "col" 
        
               result_sets = [("row1",), ("row2",)] 
        
               self.cur.description = [(column,)] 
        
               self.cur.fetchall.return_value = result_sets 
        
               df = self.db_hook().get_pandas_df(statement) 
        
               assert column == df.columns[0] 
        
               for i, item in enumerate(result_sets): 
        
                   assert item[0] == df.values.tolist()[i][0] 
        
               assert self.conn.close.call_count == 1 
        
               assert self.cur.close.call_count == 1 
        
               self.cur.execute.assert_called_once_with(statement)

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

docs/apache-airflow-providers-apache-druid/connections/druid.rst

docs/apache-airflow-providers-apache-druid/connections/index.rst

Taragolis · 2023-11-05T12:21:27Z

docs/apache-airflow-providers-apache-druid/connections/index.rst

+Apache Airflow implemented connection to Apache Druid in different ways that listed here:
+    * Connection that uses the druid ingestion type that makes it possible to submit index/ingestion jobs.
+      To use the ingestion job, choose ``Druid Ingest Connection``
+    * Connection that uses to Query data using SQL.
+      To Query data use ``Druid Broker Connection``
+


Looks kind of hard to read in additional to repeatable bullet lists

If the difference mentioned into the connections, maybe better remove section and keep only toctree? WDYT @eladkal @blcksrx

Since the page would be empty, I believe it it good to be for the context and reduce the risk of confusion

Taragolis · 2023-11-05T12:39:04Z

airflow/providers/apache/druid/hooks/druid.py

    default_conn_name = "druid_broker_default"
-    conn_type = "druid"
-    hook_name = "Druid"
+    conn_type = "druid_broker"


I assume that this might be a breaking changes. Because if you rename conn_type then previously created connection IDs will keep conn_type=druid as result Airflow UI have no idea how to deal with this connection type

And fallback to the first available Connection type into the list

So we need to write down information into the change log how to resolve this situation.

That makes sense. How about writing a alembic migration for that?

We do not have mechanism to run alembic when upgrading provider version.

I think noting it the changelog and making it a major release with breaking change should be enough.
cc @potiuk WDYT?

Or adding backwards compatibility to handle both cases - I think it should be possible.

The problem that DruidDbApiHook use DbApiHook and this might be the single place outside of the UI where conn_type also matter. And it could be hard, but not impossible to make it by backward compatible. Better just keep conn_type="druid" since it required almost zero effort.

Indeed the easiest way would be switch to druid. how about also adding a warning that the name would be change in the next major release?

We can decide on deprecating first or on cutting major release immediately.
We always prefer to deprecate first if we can.

Taragolis · 2023-11-05T12:39:40Z

airflow/utils/db.py

        Connection(
            conn_id="druid_broker_default",
-            conn_type="druid",
+            conn_type="druid_broker",


Unfortunetly this not affect already existed connection ID

Taragolis · 2023-11-05T12:39:46Z

airflow/utils/db.py

        Connection(
            conn_id="druid_ingest_default",
-            conn_type="druid",
+            conn_type="druid_ingest",


Unfortunetly this not affect already existed connection ID

eladkal · 2024-01-07T06:47:05Z

@blcksrx are you planning completing this PR?

blcksrx · 2024-01-07T11:32:38Z

Yeah, I do! Actually i asked a question but didn’t got any answers On Sun, Jan 7, 2024 at 07:47, Elad Kalif ***@***.***> wrote: @blcksrx are you planning completing this PR? —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

eladkal · 2024-01-07T11:39:22Z

Actually i asked a question but didn’t got any answers

Which question?

blcksrx · 2024-01-07T11:42:54Z

About warning or decision about deprecation on the Major release On Sun, Jan 7, 2024 at 12:39, Elad Kalif ***@***.***> wrote: Actually i asked a question but didn’t got any answers Which question? —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

eladkal · 2024-01-07T11:47:04Z

About warning or decision about deprecation on the Major release

I answered about it in #35244 (comment)
Is there a followup question?

blcksrx · 2024-01-07T16:44:34Z

I see, Im gonna fix it this week. On Sun, Jan 7, 2024 at 12:47, Elad Kalif ***@***.***> wrote: About warning or decision about deprecation on the Major release I answered about it in #35244 (comment) Is there a followup question? —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

github-actions · 2024-02-22T00:11:13Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

boring-cyborg bot added area:providers kind:documentation provider:apache-druid labels Oct 28, 2023

eladkal reviewed Oct 28, 2023

View reviewed changes

docs/apache-airflow-providers-apache-druid/connections/druid.rst Outdated Show resolved Hide resolved

eladkal mentioned this pull request Oct 29, 2023

Created druid.rst and added documentaton #35251

Closed

blcksrx force-pushed the druid branch from 7ff0377 to ede7bfe Compare October 29, 2023 21:06

blcksrx requested a review from eladkal October 29, 2023 21:09

blcksrx force-pushed the druid branch from ede7bfe to ea83307 Compare October 29, 2023 22:57

eladkal reviewed Oct 30, 2023

View reviewed changes

docs/apache-airflow-providers-apache-druid/connections/index.rst Outdated Show resolved Hide resolved

blcksrx force-pushed the druid branch from ea83307 to 841de88 Compare November 2, 2023 20:55

eladkal approved these changes Nov 2, 2023

View reviewed changes

blcksrx force-pushed the druid branch from 841de88 to b9e8153 Compare November 4, 2023 12:54

enhance druid connection parameters

7ccda26

blcksrx force-pushed the druid branch from b9e8153 to 7ccda26 Compare November 4, 2023 14:03

Taragolis reviewed Nov 5, 2023

View reviewed changes

eladkal mentioned this pull request Jan 7, 2024

Add missing connection types and documentation for community providers #28790

Closed

49 tasks

github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Feb 22, 2024

github-actions bot closed this Feb 27, 2024

	def test_get_pandas_df(self):
	statement = "SQL"
	column = "col"
	result_sets = [("row1",), ("row2",)]
	self.cur.description = [(column,)]
	self.cur.fetchall.return_value = result_sets
	df = self.db_hook().get_pandas_df(statement)

	assert column == df.columns[0]
	for i, item in enumerate(result_sets):
	assert item[0] == df.values.tolist()[i][0]
	assert self.conn.close.call_count == 1
	assert self.cur.close.call_count == 1
	self.cur.execute.assert_called_once_with(statement)

enhance druid connection parameters #35244

enhance druid connection parameters #35244

Uh oh!

Conversation

blcksrx commented Oct 28, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eladkal commented Jan 7, 2024

Uh oh!

blcksrx commented Jan 7, 2024 via email

Uh oh!

eladkal commented Jan 7, 2024

Uh oh!

blcksrx commented Jan 7, 2024 via email

Uh oh!

eladkal commented Jan 7, 2024

Uh oh!

blcksrx commented Jan 7, 2024 via email

Uh oh!

github-actions bot commented Feb 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants