Skip to content

[AIRFLOW-4935] Add method in the bigquery hook to list tables in a dataset#5566

Merged
mik-laj merged 1 commit intoapache:masterfrom
benjamingrenier:AIRFLOW-4935
Dec 8, 2019
Merged

[AIRFLOW-4935] Add method in the bigquery hook to list tables in a dataset#5566
mik-laj merged 1 commit intoapache:masterfrom
benjamingrenier:AIRFLOW-4935

Conversation

@benjamingrenier
Copy link
Contributor

Method get_dataset_tables_list added in the hook, permits to use a table prefix too.

@ryanyuan
Copy link
Contributor

Could you write some unit tests for it?

@mik-laj mik-laj added the provider:google Google (including GCP) related issues label Jul 22, 2019
@zzlbuaa
Copy link

zzlbuaa commented Jul 24, 2019

Hi, this hook method is a useful feature for our use case. I'm wondering are you still working on this PR?
cc @criccomini @whynick1

@benjamingrenier
Copy link
Contributor Author

benjamingrenier commented Jul 24, 2019 via email

@zzlbuaa
Copy link

zzlbuaa commented Aug 5, 2019

@benjamingrenier any updates on this?

@benjamingrenier
Copy link
Contributor Author

benjamingrenier commented Aug 5, 2019 via email

@mik-laj
Copy link
Member

mik-laj commented Aug 12, 2019

Are you planning to continue working on this change? This week, I would like to deal with PR reviews related to GCP. I would be happy if you would respond to all comments

@benjamingrenier
Copy link
Contributor Author

benjamingrenier commented Aug 12, 2019

Yes I will work on it this week. I try to resolve my docker errors to run unit-tests locally.

@mik-laj
Copy link
Member

mik-laj commented Aug 12, 2019

Recently introduced a new development environment. have you tried them? we plan to create a new extension to improve GCP development.

@mik-laj
Copy link
Member

mik-laj commented Aug 14, 2019

Can you do rebase, please?

@benjamingrenier
Copy link
Contributor Author

Can you do rebase, please?

It's done. Ok for you ?

@mik-laj
Copy link
Member

mik-laj commented Aug 20, 2019

Something is wrong with this PR. Too many commits.

I'm on vacation now🇫🇷. I'll look at this on Friday.

@benjamingrenier
Copy link
Contributor Author

I think it's all right.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.log.info(dataset_tables_list)

I haven't seen it before, but it seems to me that there will be too much information in the logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will fix it soon. I prepare a commit with more unit tests too.

Copy link
Member

Choose a reason for hiding this comment

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

Can you please remove the log before merge?

@mik-laj
Copy link
Member

mik-laj commented Sep 5, 2019

Hi.

I made a change in the base class - GoogleCloudBaseHook. Your PR may need to be changed. Could you do rebase?

Cheers

Reference:
#5907

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

@benjamingrenier We are in the middle of moving hooks and operators from contrib to core. Is it possible to rebase (add more unittestss?) quickly so that we can merge the change before we move it?

Copy link
Member

Choose a reason for hiding this comment

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

Can you please remove the log before merge?

@benjamingrenier
Copy link
Contributor Author

@benjamingrenier We are in the middle of moving hooks and operators from contrib to core. Is it possible to rebase (add more unittestss?) quickly so that we can merge the change before we move it?

Rebase done and unittests added

@potiuk
Copy link
Member

potiuk commented Sep 21, 2019

Hey @benjamingrenier -> I must ask for another rebase: (.

@benjamingrenier
Copy link
Contributor Author

benjamingrenier commented Sep 21, 2019 via email

@codecov-io
Copy link

codecov-io commented Sep 22, 2019

Codecov Report

Merging #5566 into master will increase coverage by 0.13%.
The diff coverage is 94.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5566      +/-   ##
==========================================
+ Coverage   79.93%   80.06%   +0.13%     
==========================================
  Files         608      608              
  Lines       35030    35048      +18     
==========================================
+ Hits        28000    28062      +62     
+ Misses       7030     6986      -44
Impacted Files Coverage Δ
airflow/gcp/hooks/bigquery.py 70.85% <94.44%> (+0.64%) ⬆️
airflow/models/taskinstance.py 93.72% <0%> (+0.5%) ⬆️
airflow/jobs/scheduler_job.py 74.28% <0%> (+0.9%) ⬆️
airflow/utils/dag_processing.py 58.98% <0%> (+2.54%) ⬆️
airflow/executors/__init__.py 67.34% <0%> (+4.08%) ⬆️
airflow/utils/sqlalchemy.py 93.22% <0%> (+6.77%) ⬆️
airflow/utils/log/colored_log.py 93.18% <0%> (+11.36%) ⬆️
airflow/executors/sequential_executor.py 100% <0%> (+52.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d19a41...e1d8ecd. Read the comment docs.

@whynick1
Copy link
Contributor

I am also interested in this PR, any update? 😄
@potiuk @mik-laj @ryanyuan

@benjamingrenier
Copy link
Contributor Author

Someone can do the review ?

@mik-laj
Copy link
Member

mik-laj commented Nov 30, 2019

I added this PR to the queue next week. I will try to review it as soon as possible.

@mik-laj mik-laj merged commit 4561978 into apache:master Dec 8, 2019
galuszkak pushed a commit to FlyrInc/apache-airflow that referenced this pull request Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

provider:google Google (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants