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

Fix issue with built-in function #241

Merged
merged 2 commits into from
Apr 14, 2022
Merged

Conversation

tiopramayudi
Copy link
Collaborator

What this PR does / why we need it:

We faced issue when dealing with expr built function, such as filter, map, none, all, and any. This is because we override arithmetic and comparator operation. The override operation will register the operation and evaluate later, this is due to subset operation for series, but for operation between primitive types (int, bool, string, float) we can evaluate the value immediately. Hence the fix, will evaluate the value of operation between primitive types immediately
Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:


Checklist

  • Added unit test, integration, and/or e2e tests
  • Tested locally
  • Updated documentation
  • Update Swagger spec if the PR introduce API changes
  • Regenerated Golang and Python client if the PR introduce API changes

@tiopramayudi tiopramayudi requested review from pradithya and karzuo and removed request for pradithya April 14, 2022 00:26
@pradithya
Copy link
Member

Can you add test that will catch this issue in https://github.com/gojek/merlin/blob/d5f9a3b63fcc7e84d1bd3a43e9eda93f68b53460/api/pkg/transformer/server/server_test.go#L146 . This test cover standard transformer behavior almost end to end, so I would like to use it to minimise number of end to end test / manual test.

@tiopramayudi
Copy link
Collaborator Author

Can you add test that will catch this issue in

https://github.com/gojek/merlin/blob/d5f9a3b63fcc7e84d1bd3a43e9eda93f68b53460/api/pkg/transformer/server/server_test.go#L146

. This test cover standard transformer behavior almost end to end, so I would like to use it to minimise number of end to end test / manual test.

done

Copy link
Member

@pradithya pradithya left a comment

Choose a reason for hiding this comment

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

LGTM

@tiopramayudi tiopramayudi merged commit 46aebd7 into main Apr 14, 2022
@tiopramayudi tiopramayudi deleted the fix-issue-with-built-in-fn branch April 14, 2022 02:20
pradithya pushed a commit that referenced this pull request Oct 3, 2022
* Fix issue when dealing with expr built-in function

* Update server test

Co-authored-by: Tio Pramayudi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants