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

msyql integration added #965

Merged
merged 1 commit into from
Aug 31, 2023
Merged

msyql integration added #965

merged 1 commit into from
Aug 31, 2023

Conversation

rohithmulumudy
Copy link
Contributor

@rohithmulumudy rohithmulumudy commented Aug 26, 2023 •
edited
Loading

  • Integrated mysql data source

github-actions[bot]
github-actions bot reviewed Aug 26, 2023
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

👋 Hello @rohithmulumudy, thanks for submitting a EVA DB PR 🙏 To allow your work to be integrated as seamlessly as possible, we advise you to:

  • ✅ Verify that your PR is up-to-date with georgia-tech-db/eva master branch. If your PR is behind you can update your code by clicking the "Update branch" button or by running git pull and git merge master locally.
  • ✅ Verify that all EVA DB Continuous Integration (CI) checks are passing.
  • ✅ Reduce changes to the absolute minimum required for your bug fix or feature addition.

@xzdandy xzdandy added this to the v0.3.3 milestone Aug 26, 2023
@xzdandy xzdandy linked an issue Aug 26, 2023 that may be closed by this pull request
Integration with MySQL #954
Closed
2 tasks
@xzdandy xzdandy added the Integrations 🧩 Pull requests that update an integration label Aug 26, 2023
@jiashenC jiashenC changed the base branch from master to staging August 26, 2023 16:06
@xzdandy
Copy link
Collaborator

xzdandy commented Aug 27, 2023

Please merge the staging branch, which uses a newer version of circle ci, where the tests are not flaky and faster!

@rohithmulumudy
Copy link
Contributor Author

@xzdandy How do I ensure that the MySQL server runs in circle CI?

@xzdandy
Copy link
Collaborator

xzdandy commented Aug 27, 2023

@xzdandy How do I ensure that the MySQL server runs in circle CI?

My suggestion is adding a unit test that mocks the connection first. Regarding on adding a new integration test, we need to set up a docket container in circle ci. Hi @jiashenC, what do you think? Thanks!

@jiashenC
Copy link
Member

@xzdandy How do I ensure that the MySQL server runs in circle CI?

My suggestion is adding a unit test that mocks the connection first. Regarding on adding a new integration test, we need to set up a docket container in circle ci. Hi @jiashenC, what do you think? Thanks!

Sounds good!

@rohithmulumudy rohithmulumudy force-pushed the mysql_handler branch 3 times, most recently from 930ed2b to ff54219 Compare August 31, 2023 07:19
@rohithmulumudy
Copy link
Contributor Author

@jiashenC Added the unit tests, but I see that psycopg2 and mysql.connector are not installed in test env resulting in test failures

@jiashenC
Copy link
Member

@jiashenC Added the unit tests, but I see that psycopg2 and mysql.connector are not installed in test env resulting in test failures

Thanks for the updates. Yeah, we decide to not auto-install those packages for users. For unit tests, can you add a mock for those two packages as well?

@jiashenC jiashenC self-requested a review August 31, 2023 21:07
jiashenC
jiashenC approved these changes Aug 31, 2023
View reviewed changes
Copy link
Member

@jiashenC jiashenC left a comment

Choose a reason for hiding this comment

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

LGTM

@jiashenC jiashenC requested a review from xzdandy August 31, 2023 21:08
@jiashenC jiashenC merged commit 8aa807b into georgia-tech-db:staging Aug 31, 2023
@gaurav274
Copy link
Member

@rohithmulumudy A Few things need to be improved -

  1. get_columns has to be updated - It should have the following two columns: name and dtype. The dtype should be a Python dtype and will default to str.
  2. The init should have self.connection = None

@rohithmulumudy
Copy link
Contributor Author

2. elf.connection = None

@gaurav274 the first point is implemented in the following PR: af79199

Will incorporate the second suggestion in the coming PRs

hershd23 pushed a commit to hershd23/evadb that referenced this pull request Sep 4, 2023
@rohithmulumudy @hershd23
msyql integration added (georgia-tech-db#965)
5ef4706 
- Integrated mysql data source
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@jiashenC jiashenC jiashenC approved these changes

@xzdandy xzdandy Awaiting requested review from xzdandy

Assignees
No one assigned
Labels
Integrations 🧩 Pull requests that update an integration
Projects
None yet
Milestone
v0.3.3
Development

Successfully merging this pull request may close these issues.

Integration with MySQL
4 participants
@rohithmulumudy @xzdandy @jiashenC @gaurav274