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

[luigi.contrib.hive] HiveTableTarget inherits HivePartitionTarget #2872

Merged
merged 2 commits into from
Jan 28, 2020

Conversation

drowoseque
Copy link
Contributor

Description

luigi.contrib.hive.HiveTableTarget inherits HivePartitionTarget

Motivation and Context

  • HiveTableTarget is a particular case of HivePartitionTarget with no partition
  • methods in HiveTableTarget are copy-pasted from HivePartitionTarget

Have you tested this? If so, how?

Unit-tests are updated

@@ -469,34 +469,6 @@ def run_job(self, job, tracking_url_callback=None):
return luigi.contrib.hadoop.run_and_track_hadoop_job(arglist, job.set_tracking_url)


class HiveTableTarget(luigi.Target):
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 moved this class declaration down to make new parent visible to it

@drowoseque drowoseque force-pushed the hive-table-iherit-from-partition branch from afffb5c to 702a4e7 Compare December 24, 2019 03:48
@drowoseque
Copy link
Contributor Author

@honnix
@dlstadther
PTAL

Copy link
Collaborator

@dlstadther dlstadther left a comment

Choose a reason for hiding this comment

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

i think this makes sense to me. i've never been a hive luigi user, but the surface looks fine to me. while it was mostly copy/paste, could you improve the note left below?

luigi/contrib/hive.py Outdated Show resolved Hide resolved
@drowoseque
Copy link
Contributor Author

@Tarrasch @honnix
please up

@honnix
Copy link
Member

honnix commented Jan 28, 2020

@drowoseque Sorry for being late. I saw this PR has been approved. Anything else you need from me apart from merging it?

@drowoseque
Copy link
Contributor Author

@drowoseque Sorry for being late. I saw this PR has been approved. Anything else you need from me apart from merging it?

@honnix
if one approve is enough then i need nothing from you except merging :)

@honnix
Copy link
Member

honnix commented Jan 28, 2020

Yeah one is usually enough. I will take a look just to get informed and then merge it. Thanks for the PR.

@honnix honnix merged commit a0bedf6 into spotify:master Jan 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants