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

BigQuery Engine and Spark lib restructuring #269

Merged
merged 34 commits into from
Jun 28, 2022

Conversation

gdevanla
Copy link
Collaborator

@gdevanla gdevanla commented May 26, 2022

Description of what I changed

  1. Implementation of query lib interface for BigQuery (Issue Implement a BigQuery runner for the query library #180)
  2. Restructure spark implementation to reduce dependencies
  3. Black formatting to ease of maintenance and contribution. Note: Black formatting only applied to selected modules. We can start applying to all other modules if team OK with it.

E2E test

TESTED: Both e2e testing for Spark and BigQuery

Checklist: I completed these to help reviewers :)

  • My IDE is configured to follow the Python sytle guidelines

    No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend

  • [x ] I am familiar with Google Style Guides for the language I have coded in. In addition I have applied Black to make it uniform.

    No? Please take some time and review Java and Python style guides. Note, when in conflict, OpenMRS style guide overrules.

  • [ x] I have added tests to cover my changes. (If you refactored existing code that was well tested you do not have to add tests)

  • [ x] All new and existing tests passed.

  • [ x] My pull request is based on the latest changes of the master branch.

@gdevanla gdevanla requested a review from bashir2 May 26, 2022 01:23
@gdevanla
Copy link
Collaborator Author

@bashir2 Note that I have gone ahead and performed black formatting on the modules. That means we will be using " rather than single quotes since Black is opinionated on this. I think its better to decide on this now and make adding more code easier. We can chat more on this if you have concerns.

Copy link
Collaborator

@bashir2 bashir2 left a comment

Choose a reason for hiding this comment

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

Thanks @gdevanla for the changes; this looks much cleaner. I have not looked at the test related changes as not all the tests pass. You should be able to make BigQuery related tests to pass too. Please also have a single commit for the next round of review as GitHub review tool is horrible in showing changes in multiple commits.

@gdevanla gdevanla force-pushed the devanla/big_query_engine-2 branch 3 times, most recently from 38fc2f5 to ce5da62 Compare June 8, 2022 14:43
Copy link
Collaborator Author

@gdevanla gdevanla left a comment

Choose a reason for hiding this comment

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

@bashir2 Addressed most of the items and left responses to some.

The tests still fail due to permission errors. We probably have to work together on fixing those.

Regarding, branching of query_lib to retain history, we can chat.

@gdevanla gdevanla force-pushed the devanla/big_query_engine-2 branch 3 times, most recently from 2b1116f to 40e32d3 Compare June 8, 2022 19:59
Copy link
Collaborator Author

@gdevanla gdevanla left a comment

Choose a reason for hiding this comment

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

resovled issues

Copy link
Collaborator

@bashir2 bashir2 left a comment

Choose a reason for hiding this comment

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

Thanks @gdevanla for the changes; this looks good. Most of my comments are minor/suggestions. Also sorry for the delay as I was away most of last week; let's try to merge this soon.

@gdevanla gdevanla force-pushed the devanla/big_query_engine-2 branch 2 times, most recently from 6895c37 to 337575a Compare June 24, 2022 14:37
Copy link
Collaborator Author

@gdevanla gdevanla left a comment

Choose a reason for hiding this comment

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

Hi @bashir2 , I have resolved most of the items. I left some notes on unresolved items. Please take a look when you get a chance. Happy to have a quick chat if required.

@gdevanla gdevanla force-pushed the devanla/big_query_engine-2 branch from 337575a to 4baa63e Compare June 24, 2022 14:53
Copy link
Collaborator Author

@gdevanla gdevanla left a comment

Choose a reason for hiding this comment

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

Added another comment.

@gdevanla gdevanla force-pushed the devanla/big_query_engine-2 branch 3 times, most recently from 5fc362a to 0139d23 Compare June 27, 2022 16:15
Copy link
Collaborator

@bashir2 bashir2 left a comment

Choose a reason for hiding this comment

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

Thanks Guru; I believe the only remaining issue is the base_url in signature and the inheritance issues it has caused. As a side, I think you overwrote the git commit from the last review round. It would be great if we create one [and exactly one] new commit per review round. This makes the bad GitHub review process a little better :-)

@gdevanla
Copy link
Collaborator Author

Thanks Guru; I believe the only remaining issue is the base_url in signature and the inheritance issues it has caused. As a side, I think you overwrote the git commit from the last review round. It would be great if we create one [and exactly one] new commit per review round. This makes the bad GitHub review process a little better :-)

Hi @bashir2 , that is strange. I still see the last commit as before. Where do you see the new commit?

@gdevanla gdevanla force-pushed the devanla/big_query_engine-2 branch from 0139d23 to 4d3bd33 Compare June 28, 2022 13:46
Copy link
Collaborator

@bashir2 bashir2 left a comment

Choose a reason for hiding this comment

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

Thanks Guru; I think we only need to remove partial import (and if you like use setUp but that is not required if you prefer the current form).

@gdevanla gdevanla force-pushed the devanla/big_query_engine-2 branch from 9fd5f62 to a2b2540 Compare June 28, 2022 18:43
@bashir2 bashir2 merged commit e86514c into google:master Jun 28, 2022
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.

2 participants