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.

Sign up for GitHub

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: add is empty, is not empty, starts with, ends with filters operators #801

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: add is empty, is not empty, starts with, ends with filters operators #801

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
06fef3a
feat: add is empty, is not empty, starts with, ends with filters oper…
ChibiBlasphem Jan 16, 2025
b368249
PR feedbacks
ChibiBlasphem Jan 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions models/ast/ast_filter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package ast

import "slices"

type FilterOperator string

const (
Expand All @@ -11,9 +13,17 @@ const (
FILTER_LESSER_OR_EQUAL FilterOperator = "<="
FILTER_IS_IN_LIST FilterOperator = "IsInList"
FILTER_IS_NOT_IN_LIST FilterOperator = "IsNotInList"
FILTER_IS_EMPTY FilterOperator = "IsEmpty"
FILTER_IS_NOT_EMPTY FilterOperator = "IsNotEmpty"
FILTER_STARTS_WITH FilterOperator = "StringStartsWith"
FILTER_ENDS_WITH FilterOperator = "StringEndsWith"
FILTER_UNKNOWN_OPERATION FilterOperator = "FILTER_UNKNOWN_OPERATION"
)

func (op FilterOperator) IsUnary() bool {
return slices.Contains([]FilterOperator{FILTER_IS_EMPTY, FILTER_IS_NOT_EMPTY}, op)
}

type Filter struct {
TableName string
FieldName string
Expand Down
41 changes: 36 additions & 5 deletions repositories/ingested_data_read_repository.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,12 @@ import (
"github.com/checkmarble/marble-backend/models/ast"
)

// Define where to put that
type FilterWithType struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer to put it into models (most generally) or optionally in the usecase where you are using it (if it's really a local type that you'll use just there)

Copy link
Contributor

Choose a reason for hiding this comment

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

=> in this case, into models since you use it in the usecase and the repo

Filter ast.Filter
FieldType models.DataType
}

type IngestedDataReadRepository interface {
GetDbField(ctx context.Context, exec Executor, readParams models.DbFieldReadParams) (any, error)
ListAllObjectIdsFromTable(
Expand All @@ -33,7 +39,7 @@ type IngestedDataReadRepository interface {
fieldName string,
fieldType models.DataType,
aggregator ast.Aggregator,
filters []ast.Filter,
filters []FilterWithType,
) (any, error)
}

Expand Down Expand Up @@ -314,7 +320,7 @@ func createQueryAggregated(
fieldName string,
fieldType models.DataType,
aggregator ast.Aggregator,
filters []ast.Filter,
filters []FilterWithType,
) (squirrel.SelectBuilder, error) {
var selectExpression string
if aggregator == ast.AGGREGATOR_COUNT_DISTINCT {
Expand All @@ -339,7 +345,8 @@ func createQueryAggregated(

var err error
for _, filter := range filters {
query, err = addConditionForOperator(query, qualifiedTableName, filter.FieldName, filter.Operator, filter.Value)
query, err = addConditionForOperator(query, qualifiedTableName,
filter.Filter.FieldName, filter.FieldType, filter.Filter.Operator, filter.Filter.Value)
if err != nil {
return squirrel.SelectBuilder{}, err
}
Expand All @@ -354,7 +361,7 @@ func (repo *IngestedDataReadRepositoryImpl) QueryAggregatedValue(
fieldName string,
fieldType models.DataType,
aggregator ast.Aggregator,
filters []ast.Filter,
filters []FilterWithType,
) (any, error) {
if err := validateClientDbExecutor(exec); err != nil {
return nil, err
Expand All @@ -377,7 +384,7 @@ func (repo *IngestedDataReadRepositoryImpl) QueryAggregatedValue(
return result, nil
}

func addConditionForOperator(query squirrel.SelectBuilder, tableName string, fieldName string,
func addConditionForOperator(query squirrel.SelectBuilder, tableName string, fieldName string, fieldType models.DataType,
operator ast.FilterOperator, value any,
) (squirrel.SelectBuilder, error) {
switch operator {
Expand All @@ -393,6 +400,30 @@ func addConditionForOperator(query squirrel.SelectBuilder, tableName string, fie
return query.Where(squirrel.Lt{fmt.Sprintf("%s.%s", tableName, fieldName): value}), nil
case ast.FILTER_LESSER_OR_EQUAL:
return query.Where(squirrel.LtOrEq{fmt.Sprintf("%s.%s", tableName, fieldName): value}), nil
case ast.FILTER_IS_EMPTY:
orCondition := squirrel.Or{
squirrel.Eq{fmt.Sprintf("%s.%s", tableName, fieldName): nil},
}
if fieldType == models.String {
orCondition = append(orCondition, squirrel.Eq{
fmt.Sprintf("%s.%s", tableName, fieldName): "",
})
}
return query.Where(orCondition), nil
case ast.FILTER_IS_NOT_EMPTY:
andCondition := squirrel.And{
Copy link
Contributor

Choose a reason for hiding this comment

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

you can reduce code a little bit by avoiding the squirrel.And and just adding to the squirrel.NotEq conditionally

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That may be a stupid question, but I don't know how I can add the second NotEq conditionally on the first one.

Copy link
Contributor

Choose a reason for hiding this comment

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

squirrel.And is just an alias wrapper over a go map[string]any, so just by setting the value in the map

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I read the code squirrel.And is an alias for []Sqlizer, so I don't know how to add both NotEq without it. Can you suggest me some code for me to see what you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry, I didn't write what I meant to write. I meant to say that squirrel.(Not)Eq is a wrapper over a map.
So you could do

notEq := squirrel.NotEq{fmt.Sprintf("%s.%s", tableName, fieldName): nil}
if fieldType == models.String {
    notEq[fmt.Sprintf("%s.%s", tableName, fieldName)] = ""
}
return query.Where(notEq), nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh but won't the second one replace the first? The key being the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok you are right 🤦 sorry for misleading you

squirrel.NotEq{fmt.Sprintf("%s.%s", tableName, fieldName): nil},
}
if fieldType == models.String {
andCondition = append(andCondition,
squirrel.NotEq{fmt.Sprintf("%s.%s", tableName, fieldName): ""},
)
}
return query.Where(andCondition), nil
case ast.FILTER_STARTS_WITH:
return query.Where(squirrel.Like{fmt.Sprintf("%s.%s", tableName, fieldName): fmt.Sprintf("%s%%", value)}), nil
case ast.FILTER_ENDS_WITH:
return query.Where(squirrel.Like{fmt.Sprintf("%s.%s", tableName, fieldName): fmt.Sprintf("%%%s", value)}), nil
default:
return query, fmt.Errorf("unknown operator %s: %w", operator, models.BadParameterError)
}
Expand Down
28 changes: 17 additions & 11 deletions repositories/ingested_data_read_repository_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ func TestIngestedDataQueryAggregatedValueWithoutFilter(t *testing.T) {
utils.DummyFieldNameForInt,
models.Int,
ast.AGGREGATOR_AVG,
[]ast.Filter{},
[]FilterWithType{},
)
assert.Empty(t, err)
sql, args, err := query.ToSql()
Expand All @@ -123,7 +123,7 @@ func TestIngestedDataQueryCountWithoutFilter(t *testing.T) {
utils.DummyFieldNameForInt,
models.Int,
ast.AGGREGATOR_COUNT,
[]ast.Filter{})
[]FilterWithType{})
assert.Empty(t, err)
sql, args, err := query.ToSql()
assert.Empty(t, err)
Expand All @@ -134,18 +134,24 @@ func TestIngestedDataQueryCountWithoutFilter(t *testing.T) {
}

func TestIngestedDataQueryAggregatedValueWithFilter(t *testing.T) {
filters := []ast.Filter{
filters := []FilterWithType{
{
TableName: utils.DummyTableNameFirst,
FieldName: utils.DummyFieldNameForInt,
Operator: ast.FILTER_EQUAL,
Value: 1,
Filter: ast.Filter{
TableName: utils.DummyTableNameFirst,
FieldName: utils.DummyFieldNameForInt,
Operator: ast.FILTER_EQUAL,
Value: 1,
},
FieldType: models.Int,
},
{
TableName: utils.DummyTableNameFirst,
FieldName: utils.DummyFieldNameForBool,
Operator: ast.FILTER_NOT_EQUAL,
Value: true,
Filter: ast.Filter{
TableName: utils.DummyTableNameFirst,
FieldName: utils.DummyFieldNameForBool,
Operator: ast.FILTER_NOT_EQUAL,
Value: true,
},
FieldType: models.Bool,
},
}

Expand Down
26 changes: 21 additions & 5 deletions usecases/ast_eval/evaluate/evaluate_aggregator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ func (a AggregatorEvaluator) Evaluate(ctx context.Context, arguments ast.Argumen
}

// Filters validation
var filtersWithType []repositories.FilterWithType
if len(filters) > 0 {
for _, filter := range filters {
if filter.TableName != tableName {
Expand All @@ -82,14 +83,28 @@ func (a AggregatorEvaluator) Evaluate(ctx context.Context, arguments ast.Argumen
ast.NewNamedArgumentError("filters"),
))
}
// At the first nil filter value found, stop and just return the default value for the aggregator
if filter.Value == nil {

// At the first nil filter value found if we're not on an unary operator, stop and just return the default value for the aggregator
if filter.Value == nil && !filter.Operator.IsUnary() {
return a.defaultValueForAggregator(aggregator)
}

filterFieldType, err := getFieldType(a.DataModel, filter.TableName, filter.FieldName)
if err != nil {
return MakeEvaluateError(errors.Join(
errors.Wrap(err, fmt.Sprintf("field type for %s.%s not found in data model in Evaluate aggregator", filter.TableName, filter.FieldName)),
ast.NewNamedArgumentError("fieldName"),
))
}

filtersWithType = append(filtersWithType, repositories.FilterWithType{
Filter: filter,
FieldType: filterFieldType,
})
}
}

result, err := a.runQueryInRepository(ctx, tableName, fieldName, fieldType, aggregator, filters)
result, err := a.runQueryInRepository(ctx, tableName, fieldName, fieldType, aggregator, filtersWithType)
if err != nil {
return MakeEvaluateError(errors.Wrap(err, "Error running aggregation query in repository"))
}
Expand All @@ -107,7 +122,7 @@ func (a AggregatorEvaluator) runQueryInRepository(
fieldName string,
fieldType models.DataType,
aggregator ast.Aggregator,
filters []ast.Filter,
filters []repositories.FilterWithType,
) (any, error) {
if a.ReturnFakeValue {
return DryRunQueryAggregatedValue(a.DataModel, tableName, fieldName, aggregator)
Expand All @@ -117,7 +132,8 @@ func (a AggregatorEvaluator) runQueryInRepository(
if err != nil {
return nil, err
}
return a.IngestedDataReadRepository.QueryAggregatedValue(ctx, db, tableName, fieldName, fieldType, aggregator, filters)
return a.IngestedDataReadRepository.QueryAggregatedValue(ctx, db, tableName,
fieldName, fieldType, aggregator, filters)
}

func (a AggregatorEvaluator) defaultValueForAggregator(aggregator ast.Aggregator) (any, []error) {
Expand Down
4 changes: 4 additions & 0 deletions usecases/ast_eval/evaluate/evaluate_filter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ var validTypeForFilterOperators = map[ast.FilterOperator][]models.DataType{
ast.FILTER_LESSER_OR_EQUAL: {models.Int, models.Float, models.String, models.Timestamp},
ast.FILTER_IS_IN_LIST: {models.String},
ast.FILTER_IS_NOT_IN_LIST: {models.String},
ast.FILTER_IS_EMPTY: {models.Int, models.Float, models.String, models.Timestamp},
Copy link
Contributor

Choose a reason for hiding this comment

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

just for the sake of completeness: could also work with bool type

ast.FILTER_IS_NOT_EMPTY: {models.Int, models.Float, models.String, models.Timestamp},
ast.FILTER_STARTS_WITH: {models.String},
ast.FILTER_ENDS_WITH: {models.String},
}

func (f FilterEvaluator) Evaluate(ctx context.Context, arguments ast.Arguments) (any, []error) {
Expand Down
Loading
Loading