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: Filtering implementation with support for None filter type #280

Merged
merged 16 commits into from
Jun 18, 2024

Conversation

DeshErBojhaa
Copy link
Contributor

@DeshErBojhaa DeshErBojhaa commented Jun 10, 2024

Motivation

Filtering fro data request wasm reveals.

Explanation of Changes

Right now only NONE and MODE are implemented.

Testing

Unit test included.

drfilters/filter.go Outdated Show resolved Hide resolved
Copy link
Member

@mariocao mariocao left a comment

Choose a reason for hiding this comment

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

Apart from Franklin's comments, I want to add that you have duplicated commits in the PR.

drfilters/filter.go Outdated Show resolved Hide resolved
drfilters/filter.go Outdated Show resolved Hide resolved
@DeshErBojhaa DeshErBojhaa changed the title feat: none filter for DR wasm feat: none and mode filter for DR wasm Jun 12, 2024
@DeshErBojhaa DeshErBojhaa force-pushed the tamjid/dr-filter branch 2 times, most recently from 1fedb88 to 33dcc8b Compare June 12, 2024 16:57
@DeshErBojhaa DeshErBojhaa requested a review from mariocao June 12, 2024 16:59
Comment on lines 22 to 26
//stdFilter struct {
// Algo uint
// JSONPath string
// MaxSigma uint64
// NumberType uint8
//}
Copy link
Member

Choose a reason for hiding this comment

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

Remove unused code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will implement std filter very soon. So left it here. If you feel we should remove it now and then reintroduce, please let me know.

Copy link
Member

@mariocao mariocao left a comment

Choose a reason for hiding this comment

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

I missed some testing for the mode filtering.

Comment on lines 10 to 13
const (
None = iota
Mode
StdDeviation
)
Copy link
Member

Choose a reason for hiding this comment

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

Let's create a type FilterType?

type FilterType byte

const (
	None FilterType = iota
	Mode
	StdDev
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we have any use case of the new type Filtertype as of now? If not, we don't have to complicate by introducing another type.

Copy link
Member

Choose a reason for hiding this comment

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

I see what you are saying. I converted it to byte so that we can compare the first byte of tally inputs directly to these.

Comment on lines +35 to +38
if tt.wantErr != nil {
require.ErrorIs(t, err, tt.wantErr)
return
}
Copy link
Member

Choose a reason for hiding this comment

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

This only checks for an error when we expect an error

Copy link
Member

Choose a reason for hiding this comment

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

And I think there is in fact an error here.

Comment on lines 82 to 89
var rlpAsList []any
if err := rlp.DecodeBytes(filterInput, &rlpAsList); err != nil {
return nil, false, err
}
filteringAlgo, ok := rlpAsList[0].([]uint)
if !ok || len(filteringAlgo) != 1 {
return nil, false, errors.New("can not RLP decode algo type from filter input")
}
Copy link
Member

Choose a reason for hiding this comment

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

Was confused about these lines. Why did you use RLP decoding here? I changed the code so that it just looks at the first byte. Let me know what you think.

return outliers, true, nil

case filterMode:
// TODO: Reactivate mode filter
Copy link
Member

Choose a reason for hiding this comment

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

Removed this for now - let's address this in a separate PR cc @DeshErBojhaa

@hacheigriega hacheigriega changed the title feat: none and mode filter for DR wasm feat: Filtering implementation with support for None filter type Jun 18, 2024
@hacheigriega hacheigriega self-requested a review June 18, 2024 19:29
@hacheigriega hacheigriega merged commit 64a6499 into hy/tally2 Jun 18, 2024
16 of 17 checks passed
@hacheigriega hacheigriega deleted the tamjid/dr-filter branch June 18, 2024 20:04
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.

4 participants