Add Common module #801
Add Common module #801
Conversation
terryyylim
requested review from
davidheryanto,
khorshuheng,
pyalex,
woop and
zhilingc
as code owners
| import java.util.regex.Pattern; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class StringUtils { |
There was a problem hiding this comment.
I would rather have those functions grouped by domain. Also name "StringUtils" doesn't bring clarity here - you can call "utils" almost anything of course :)
There was a problem hiding this comment.
I was expecting this haha, had a hard time thinking for a name so pushed for a review to have you guys help brainstorm 😂
There was a problem hiding this comment.
By domain you mean FeatureSet, Store etc?
There was a problem hiding this comment.
My trigger words are
- Info
- Spec
- Util
- Helper
- Metadata
Sometimes unavoidable, but we should pay attention when/why they are introduced.
There was a problem hiding this comment.
By domain you mean
FeatureSet,Storeetc?
exactly
There was a problem hiding this comment.
Updated classes based on domain.
| * @param exclude flag to determine if subscriptions with exclusion flag should be returned | ||
| * @return List of Subscription class objects | ||
| */ | ||
| public static List<StoreProto.Store.Subscription> getSubscriptionsByStr( |
There was a problem hiding this comment.
maybe better name "parseSubscriptionFrom"? similarly to protobuf api https://developers.google.com/protocol-buffers/docs/reference/java/com/google/protobuf/Parser
There was a problem hiding this comment.
Renamed function to follow protobuf api convention.
terryyylim
changed the title
terryyylim
changed the title
| @@ -14,25 +14,28 @@ | |||
| * See the License for the specific language governing permissions and | |||
| * limitations under the License. | |||
| */ | |||
| package feast.serving.util; | |||
| package feast.common.function; | |||
There was a problem hiding this comment.
Why do we have to nest this inside of function? I thought we wanted to have the functionality be domain specific. How about models or model?
common/src/test/java/feast/common/function/CommonFunctionsTest.java
Outdated
Show resolved
Hide resolved
|
/test test-end-to-end-batch |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: terryyylim, woop The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
What this PR does / why we need it:
Since common functionality can be found across existing modules (core, serving, ingestion, SDK), a common module has also been created in this PR, with tests added. The refactoring of parts of the codebase to be aligned with usage of common module will be incrementally done moving forward.
Does this PR introduce a user-facing change?: