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

feat: Add Mariadb offline store #4158

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

tmihalac
Copy link
Contributor

What this PR does / why we need it:

Adds option to use MariaDB as an offline store

@tmihalac tmihalac changed the title feature: Add Mariadb offline store feat: Add Mariadb offline store Apr 29, 2024
@tokoko
Copy link
Collaborator

tokoko commented Apr 29, 2024

@tmihalac @jeremyary my 2 cents here, if I may 😄 I've written this elsewhere a couple times and of course I have nothing against this particular implementation, I just think that we will eventually run out of maintenance effort if we keep reimplementing similar offline store engines from scratch every time. I think it will take too much effort ensuring they work and have parity with one another, also adding a new feature in offline store logic means changes in too many places. As you might have guessed, I'd rather see all new offline stores (and ideally existing ones as well eventually) to be ibis-based to avoid all that, but that's just me...

@tmihalac
Copy link
Contributor Author

tmihalac commented Apr 29, 2024

@tmihalac @jeremyary my 2 cents here, if I may 😄 I've written this elsewhere a couple times and of course I have nothing against this particular implementation, I just think that we will eventually run out of maintenance effort if we keep reimplementing similar offline store engines from scratch every time. I think it will take too much effort ensuring they work and have parity with one another, also adding a new feature in offline store logic means changes in too many places. As you might have guessed, I'd rather see all new offline stores (and ideally existing ones as well eventually) to be ibis-based to avoid all that, but that's just me...

@tokoko
I totally agree with you, thats why I started the effort with trying to create a "generic" implementation with abstract methods that each implementation needs to implement only them, but very soon I found myself struggling, maybe its because I don't know python well enough or feast itself, but I found out that a lot of the methods are static (for no apparent reason to me) beside preventing my approach.
Since I thought this is not the PR to change all the offline stores implementations and start drowning in changes (This is my first big code addition/change) I reverted to the ugly approach, which is the one you currently see in the PR.

@tokoko
Copy link
Collaborator

tokoko commented Apr 29, 2024

@tmihalac python static methods and inheritance just don't get along, that has beaten me before as well :). Anyway, I didn't mean to ask you to come up with a generic implementation, just pointing out that we already have one here. it's written with ibis which has a mysql backend (I'm assuming it will work with mariadb as well). It's right now used only by duckdb implementation and figuring out how to use it from mariadb will still be quite a bit of work, but I'm still pushing for that approach.

P.S. you can take a look at this RFC as well.

@tmihalac
Copy link
Contributor Author

@tmihalac python static methods and inheritance just don't get along, that has beaten me before as well :). Anyway, I didn't mean to ask you to come up with a generic implementation, just pointing out that we already have one here. it's written with ibis which has a mysql backend (I'm assuming it will work with mariadb as well). It's right now used only by duckdb implementation and figuring out how to use it from mariadb will still be quite a bit of work, but I'm still pushing for that approach.

P.S. you can take a look at this RFC as well.

@jeremyary and me talked before to try and use Sqlalchemy ORM for all the Offline Store DB support.
What is ibis I am not familiar with it? I see its like an ORM but according to them its not defined as one

@tokoko
Copy link
Collaborator

tokoko commented Apr 30, 2024

It's very much like a ORM, most of it's backends used to be powered by sqlalchemy actually in previous versions, but they recenly switched to sqlglot. You can think of it as an abstraction above that. it exposes a DataFrame-like API and executes these dataframes on the selected backend, in most cases it is actually generating sql first to do that. Having both ibis and sqlalchemy-based "abstract" offline stores sounds wasteful to me.

Added MariaDB offline store
Added MariaDB template

Signed-off-by: Theodor Mihalache <tmihalac@redhat.com>
Signed-off-by: Theodor Mihalache <tmihalac@redhat.com>
@tmihalac tmihalac marked this pull request as ready for review May 7, 2024 13:25
@tmihalac tmihalac requested a review from kevjumba as a code owner May 7, 2024 13:25
@franciscojavierarceo
Copy link
Member

@tmihalac do you still plan on doing this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants