Skip to content
This repository was archived by the owner on May 17, 2024. It is now read-only.

Fix databricks #273

Merged
merged 4 commits into from
Nov 7, 2022
Merged

Fix databricks #273

merged 4 commits into from
Nov 7, 2022

Conversation

pik94
Copy link
Contributor

@pik94 pik94 commented Nov 2, 2022

No description provided.

@pik94 pik94 marked this pull request as ready for review November 2, 2022 12:38
@pik94 pik94 requested review from erezsh and nolar November 2, 2022 12:39
erezsh
erezsh suggested changes Nov 2, 2022
View reviewed changes
Copy link
Contributor

@erezsh erezsh left a comment

Choose a reason for hiding this comment

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

Needs 1 change and 1 explanation.

I assume you ran all the tests with at least mysql+postgresql+databricks?
@pik94
Copy link
Contributor Author

pik94 commented Nov 2, 2022

Needs 1 change and 1 explanation.

I assume you ran all the tests with at least mysql+postgresql+databricks?

@erezsh No, I did not. Don't they run on our CI/CD?
@erezsh
Copy link
Contributor

erezsh commented Nov 2, 2022

@pik94 They do not. We don't have a proper way to set it up for tests.
@pik94
Copy link
Contributor Author

pik94 commented Nov 2, 2022

@pik94 They do not. We don't have a proper way to set it up for tests.

@erezsh , ok, i will run them locally then. For now move a PR to draft then.
@pik94 pik94 marked this pull request as draft November 2, 2022 13:52
@erezsh
Copy link
Contributor

erezsh commented Nov 2, 2022

Sorry, I caused a small conflict. It should be easy to resolve, but let me know if you need help with it.
@pik94
Copy link
Contributor Author

pik94 commented Nov 2, 2022

Sorry, I caused a small conflict. It should be easy to resolve, but let me know if you need help with it.

@erezsh np, resolved it.
@erezsh
Copy link
Contributor

erezsh commented Nov 2, 2022

Thanks. Please rebase and reduce commits before removing the 'draft' flag.
@pik94
support multithreading for databricks
1790a38 
The databricks connector is not thread-safe so we should inherit ThreadedDatabase class
@pik94
Copy link
Contributor Author

pik94 commented Nov 3, 2022

Thanks. Please rebase and reduce commits before removing the 'draft' flag.

@erezsh done. Also ran unit-tests for Databricks vs Databricks, Databricks vs PostgreSQL, Databricks vs MySQL, they were passed after some fixes.
@pik94 pik94 marked this pull request as ready for review November 3, 2022 06:49
erezsh
erezsh suggested changes Nov 4, 2022
View reviewed changes
Copy link
Contributor

@erezsh erezsh left a comment

Choose a reason for hiding this comment

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

Just one small thing.

Also don't forget to run black -l 120 data_diff/databases/databricks.py
@pik94
Copy link
Contributor Author

pik94 commented Nov 6, 2022

Just one small thing.

Also don't forget to run black -l 120 data_diff/databases/databricks.py

Done. :)
@erezsh erezsh self-requested a review November 6, 2022 10:55
@erezsh erezsh merged commit 04c2917 into datafold:master Nov 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
2 participants
@pik94 @erezsh