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

Remove unsafe pushdown #3752

Merged
merged 6 commits into from
Nov 23, 2023
Merged

Remove unsafe pushdown #3752

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1b97b26
init
noisersup Nov 23, 2023
08bc1f8
wip
noisersup Nov 23, 2023
7cd1e0c
wip
noisersup Nov 23, 2023
ea3092a
wip
noisersup Nov 23, 2023
cae48b0
rm comments
noisersup Nov 23, 2023
ccbe8da
Merge branch 'main' into remove-unsafe-pushdown-3741
mergify[bot] Nov 23, 2023
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
Prev Previous commit
Next Next commit
wip
  • Loading branch information
@noisersup
noisersup committed Nov 23, 2023
commit 08bc1f8f361d11b9d19f55b4dd62153bd51dc02f
5 changes: 2 additions & 3 deletions internal/handlers/registry/mysql.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,8 @@
ConnMetrics: opts.ConnMetrics,
StateProvider: opts.StateProvider,

DisableFilterPushdown: opts.DisableFilterPushdown,
EnableUnsafeSortPushdown: opts.EnableUnsafeSortPushdown,
EnableOplog: opts.EnableOplog,
DisableFilterPushdown: opts.DisableFilterPushdown,
EnableOplog: opts.EnableOplog,

Check warning on line 43 in internal/handlers/registry/mysql.go

View check run for this annotation

Codecov / codecov/patch

internal/handlers/registry/mysql.go#L42-L43 

Added lines #L42 - L43 were not covered by tests
}

h, err := handler.New(handlerOpts)
Expand Down
5 changes: 2 additions & 3 deletions internal/handlers/registry/postgresql.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,8 @@ func init() {
ConnMetrics: opts.ConnMetrics,
StateProvider: opts.StateProvider,

DisableFilterPushdown: opts.DisableFilterPushdown,
EnableUnsafeSortPushdown: opts.EnableUnsafeSortPushdown,
EnableOplog: opts.EnableOplog,
DisableFilterPushdown: opts.DisableFilterPushdown,
EnableOplog: opts.EnableOplog,
}

h, err := handler.New(handlerOpts)
Expand Down
32 changes: 16 additions & 16 deletions internal/handlers/sqlite/msg_aggregate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@
var iter iterator.Interface[struct{}, *types.Document]

if len(collStatsDocuments) == len(stagesDocuments) {
filter, sort := aggregations.GetPushdownQuery(aggregationStages)
filter, _ := aggregations.GetPushdownQuery(aggregationStages)

Check warning on line 262 in internal/handlers/sqlite/msg_aggregate.go

View check run for this annotation

Codecov / codecov/patch

internal/handlers/sqlite/msg_aggregate.go#L262 

Added line #L262 was not covered by tests

// only documents stages or no stages - fetch documents from the DB and apply stages to them
qp := new(backends.QueryParams)
Expand All @@ -268,24 +268,24 @@
qp.Filter = filter
}

// Skip sorting if there are more than one sort parameters
if h.EnableUnsafeSortPushdown && sort.Len() == 1 {
var order types.SortType
//// Skip sorting if there are more than one sort parameters
//if h.EnableUnsafeSortPushdown && sort.Len() == 1 {
// var order types.SortType

k := sort.Keys()[0]
v := sort.Values()[0]
// k := sort.Keys()[0]
// v := sort.Values()[0]

order, err = common.GetSortType(k, v)
if err != nil {
closer.Close()
return nil, err
}
// order, err = common.GetSortType(k, v)
// if err != nil {
// closer.Close()
// return nil, err
// }

qp.Sort = &backends.SortField{
Key: k,
Descending: order == types.Descending,
}
}
// qp.Sort = &backends.SortField{
// Key: k,
// Descending: order == types.Descending,
// }
//}

iter, err = processStagesDocuments(ctx, closer, &stagesDocumentsParams{c, qp, stagesDocuments})
} else {
Expand Down
39 changes: 17 additions & 22 deletions internal/handlers/sqlite/msg_explain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,42 +87,37 @@
qp.Filter, params.Sort = aggregations.GetPushdownQuery(params.StagesDocs)
}

// Skip sorting if there are more than one sort parameters
// TODO https://github.com/FerretDB/FerretDB/issues/3742
if h.EnableUnsafeSortPushdown && params.Sort.Len() == 1 {
var order types.SortType
//// Skip sorting if there are more than one sort parameters
//// TODO https://github.com/FerretDB/FerretDB/issues/3742
//if h.EnableUnsafeSortPushdown && params.Sort.Len() == 1 {
// var order types.SortType

k := params.Sort.Keys()[0]
v := params.Sort.Values()[0]
// k := params.Sort.Keys()[0]
// v := params.Sort.Values()[0]

order, err = common.GetSortType(k, v)
if err != nil {
return nil, err
}
// order, err = common.GetSortType(k, v)
// if err != nil {
// return nil, err
// }

qp.Sort = &backends.SortField{
Key: k,
Descending: order == types.Descending,
}
}
// qp.Sort = &backends.SortField{
// Key: k,
// Descending: order == types.Descending,
// }
//}

// Limit pushdown is not applied if:
// - `filter` is set, it must fetch all documents to filter them in memory;
// - `sort` is set but `UnsafeSortPushdown` is not set, it must fetch all documents
// and sort them in memory;
// - `sort` is set, it must fetch all documents and sort them in memory;
// - `skip` is non-zero value, skip pushdown is not supported yet.
if params.Filter.Len() == 0 && (params.Sort.Len() == 0 || h.EnableUnsafeSortPushdown) && params.Skip == 0 {
if params.Filter.Len() == 0 && params.Sort.Len() == 0 && params.Skip == 0 {

Check warning on line 113 in internal/handlers/sqlite/msg_explain.go

View check run for this annotation

Codecov / codecov/patch

internal/handlers/sqlite/msg_explain.go#L113 

Added line #L113 was not covered by tests
qp.Limit = params.Limit
}

if h.DisableFilterPushdown {
qp.Filter = nil
}

if !h.EnableUnsafeSortPushdown {
qp.Sort = nil
}

res, err := coll.Explain(ctx, &qp)
if err != nil {
return nil, lazyerrors.Error(err)
Expand Down
35 changes: 17 additions & 18 deletions internal/handlers/sqlite/msg_find.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,31 +112,30 @@
qp.Filter = params.Filter
}

// Skip sorting if there are more than one sort parameters
// TODO https://github.com/FerretDB/FerretDB/issues/3742
if h.EnableUnsafeSortPushdown && params.Sort.Len() == 1 {
var order types.SortType
//// Skip sorting if there are more than one sort parameters
//// TODO https://github.com/FerretDB/FerretDB/issues/3742
//if h.EnableUnsafeSortPushdown && params.Sort.Len() == 1 {
// var order types.SortType

k := params.Sort.Keys()[0]
v := params.Sort.Values()[0]
// k := params.Sort.Keys()[0]
// v := params.Sort.Values()[0]

order, err = common.GetSortType(k, v)
if err != nil {
return nil, err
}
// order, err = common.GetSortType(k, v)
// if err != nil {
// return nil, err
// }

qp.Sort = &backends.SortField{
Key: k,
Descending: order == types.Descending,
}
}
// qp.Sort = &backends.SortField{
// Key: k,
// Descending: order == types.Descending,
// }
//}

// Limit pushdown is not applied if:
// - `filter` is set, it must fetch all documents to filter them in memory;
// - `sort` is set but `UnsafeSortPushdown` is not set, it must fetch all documents
// and sort them in memory;
// - `sort` is set, it must fetch all documents and sort them in memory;
// - `skip` is non-zero value, skip pushdown is not supported yet.
if params.Filter.Len() == 0 && (params.Sort.Len() == 0 || h.EnableUnsafeSortPushdown) && params.Skip == 0 {
if params.Filter.Len() == 0 && params.Sort.Len() == 0 && params.Skip == 0 {

Check warning on line 138 in internal/handlers/sqlite/msg_find.go

View check run for this annotation

Codecov / codecov/patch

internal/handlers/sqlite/msg_find.go#L138 

Added line #L138 was not covered by tests
qp.Limit = params.Limit
}

Expand Down
5 changes: 2 additions & 3 deletions internal/handlers/sqlite/sqlite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,8 @@ type NewOpts struct {
StateProvider *state.Provider

// test options
DisableFilterPushdown bool
EnableUnsafeSortPushdown bool
EnableOplog bool
DisableFilterPushdown bool
EnableOplog bool
}

// New returns a new handler.
Expand Down
Loading