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.

Sign up for GitHub

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

Jump to bottom

[DF-292] Add builders: TableBuilder, ColumnBuilder, SerDeInfoBuilder and StorageDescriptorBuilder #15

Merged
merged 18 commits into from
Dec 1, 2020
Merged

[DF-292] Add builders: TableBuilder, ColumnBuilder, SerDeInfoBuilder and StorageDescriptorBuilder #15

merged 18 commits into from
Dec 1, 2020

Conversation

LucasMMota
Copy link
Contributor

@LucasMMota LucasMMota commented Nov 26, 2020 •
edited
Loading

Why? 📖

We are adding builders to isolate the thrift files logic from lib's clients.

What? 🔧

  • add builders: TableBuilder, ColumnBuilder, SerDeInfoBuilder, StorageDescriptorBuilder

Type of change 🗄️

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How everything was tested? 📏

Unit tests + test builders with API

Checklist 📝

  • I have added labels to distinguish the type of pull request.
  • My code follows the style guidelines of this project (docstrings, type hinting and linter compliance);
  • I have performed a self-review of my own code;
  • I have made corresponding changes to the documentation;
  • I have added tests that prove my fix is effective or that my feature works;
  • I have made sure that new and existing unit tests pass locally with my changes;

@LucasMMota LucasMMota added the enhancement New feature or request label Nov 26, 2020
@LucasMMota LucasMMota self-assigned this Nov 26, 2020
@LucasMMota LucasMMota changed the title [DF-292] Add builders for Table [DF-292] Add builders: TableBuilder, ColumnBuilder, SerDeInfoBuilder and StorageDescriptorBuilder Nov 27, 2020
@LucasMMota LucasMMota changed the base branch from main to DF-291 November 27, 2020 18:19
@LucasMMota LucasMMota marked this pull request as ready for review November 30, 2020 15:19
@LucasMMota LucasMMota requested a review from a team as a code owner November 30, 2020 15:19
@LucasMMota LucasMMota mentioned this pull request Nov 30, 2020
Merged
11 tasks
@jufreire jufreire force-pushed the DF-291 branch 2 times, most recently from 9faed60 to cd117fa Compare November 30, 2020 18:00
jufreire
jufreire reviewed Nov 30, 2020
View reviewed changes
hive_metastore_client/builders/serde_info_builder.py Outdated
Comment on lines 30 to 33
:param description: (no information in thrift mapping)
:param serializer_class: (no information in thrift mapping)
:param deserializer_class: (no information in thrift mapping)
:param serde_type: (no information in thrift mapping)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to keep it like that: (no information in thrift mapping)?
I guess some of them are pretty obvious ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Partition doesn't have the parameters documented in thrift, however I added a brief description as I understood the param are

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 let the docstrings to be reviewed later (because there were many here) and I forgot. Thanks for noticing.
I update the ones I knew. For these in serde_info_builder I don't know what to describe. Could you guys please help me? (And review this commit)

felipemiquelim
felipemiquelim reviewed Nov 30, 2020
View reviewed changes
examples/create_table.py Outdated
HIVE_PORT = 9083

hive_metastore_client = HiveMetastoreClient(HIVE_HOST, HIVE_PORT)
with hive_metastore_client as conn:
Copy link
Contributor

Choose a reason for hiding this comment

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

As we talked, we could also only wrap with with in the final commands, right? These instantiations do not need to be inside the with

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, good catch. This way the connection will only be oppened when it is used.

felipemiquelim
felipemiquelim approved these changes Nov 30, 2020
View reviewed changes
Copy link
Contributor

@felipemiquelim felipemiquelim left a comment

Choose a reason for hiding this comment

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

lgtm, just minor comments :)

Base automatically changed from DF-291 to main November 30, 2020 19:48
jufreire
jufreire approved these changes Nov 30, 2020
View reviewed changes
Copy link
Contributor

@jufreire jufreire left a comment

Choose a reason for hiding this comment

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

just a small comment

@LucasMMota LucasMMota merged commit d5ff375 into main Dec 1, 2020
@LucasMMota LucasMMota deleted the df-292 branch December 1, 2020 13:26
This was referenced Dec 2, 2020
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felipemiquelim felipemiquelim felipemiquelim approved these changes

@jufreire jufreire jufreire approved these changes

Assignees

@LucasMMota LucasMMota

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LucasMMota @jufreire @felipemiquelim