Database tweaks - #290

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: smadarasmi
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: zhilingc

If they are not already assigned, you can assign the PR to them by writing /assign @zhilingc in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Hi @smadarasmi. Thanks for your PR.

I'm waiting for a gojek member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

/ok-to-test

/test test-core-and-ingestion

/test test-end-to-end

@smadarasmi
Since LIKE is not the same as ~, you will need to update the configuration for stores in the end to end tests - the stores should listen for featuresets matching %:>0 (sql LIKE pattern) rather than .*:>0 (POSIX regex)

you should update the documentation here as well:
https://github.com/gojek/feast/blob/0.3-dev/protos/feast/core/Store.proto#L128

SIMILAR TO is standard SQL but unfortunately doesn't seem widely supported, including by our target RDBMS 😞 Perhaps because the standard specifies a new regex dialect that is not quite POSIX, not many vendors want to bother with it 😩

LIKE_REGEX is standard, using yet another regex dialect, and also not widely supported even by Postgres.

It's unfortunate because I think a % wildcard is probably not as intuitive (nor powerful) of a way to specify a subscription as a regex, but Postgres ~ is definitely not portable.

I'm all for moving towards something more portable! :) I agree that the LIKE pattern matching isn't as ubiquitous as regular regex but it's nothing some documentation can't cure.
Just indicating that for this MR to pass some changes need to be made for consistency (and the tests to pass).

@zhilingc Sure, I'll do this later today.

@ches @smadarasmi out of curiosity, which RDBMS are you using?

@ches @smadarasmi out of curiosity, which RDBMS are you using?

MS SQL Server. Not Azure but maybe we're providing some degree of insurance that people can run Feast on Azure services 😉

/test test-end-to-end

/test test-end-to-end

I'm still wondering if regex is necessary here. It seems like using normal LIKE would be more than sufficient, and would probably simplify our API a bit.

Can I suggest that we only allow wildcards for this RPC? How about:

• Matching a single customer feature set: customer_transactions
• Matching many customer feature sets: customer_*

Behind the scenes we can replace this with % and use LIKE.

@zhilingc @ches thoughts?

I've spoken to @zhilingc and she is fine to move ahead with this. We will limit it to purely wildcards for now, and use LIKE. That should make this a much more workable solution for MSSQL.

I will merge this in for now. Of course, this will cause a breaking API change. Master will go into an inconsistent state for a bit, but I have a PR to address that: #309

Hoping to have that merged in tomorrow. It's close to done.

@smadarasmi: The following test failed, say /retest to rerun them all:

Full PR test history