feast icon indicating copy to clipboard operation
feast copied to clipboard

feat: Add Mariadb offline store

Open tmihalac opened this issue 9 months ago • 6 comments

What this PR does / why we need it:

Adds option to use MariaDB as an offline store

tmihalac avatar Apr 29 '24 16:04 tmihalac

@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 avatar Apr 29 '24 20:04 tokoko

@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.

tmihalac avatar Apr 29 '24 21:04 tmihalac

@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.

tokoko avatar Apr 29 '24 22:04 tokoko

@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

tmihalac avatar Apr 30 '24 00:04 tmihalac

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.

tokoko avatar Apr 30 '24 04:04 tokoko