Fixed bug in certain limit .. offset queries #1015
Conversation
|
Footnotes
|
| @@ -21,17 +21,21 @@ import ( | |||
| ) | |||
|
|
|||
| // IDs are basically arbitrary, we just need to ensure that they do not conflict with existing IDs | |||
| // Comments are to match the Stringer formatting rules in the original rule definition file, but we can't generate | |||
| // human-readable strings for these extended types because they are in another package. | |||
| // We could move these into the main GMS package to fix this deficit, if we wanted. | |||
There was a problem hiding this comment.
I think that would be a bad idea. I'd argue that we shouldn't need to modify and coordinate changes across packages unless we absolutely need to. Adding a quick analyzer rule in Doltgres shouldn't need a GMS PR to first pass, then merge, then pass Dolt, then merge into Dolt. Especially if GMS becomes broken by someone else pushing to main first.
| "github.com/dolthub/doltgresql/server/functions/framework" | ||
| ) | ||
|
|
||
| func ReplaceArithmeticExpressions(ctx *sql.Context, a *analyzer.Analyzer, node sql.Node, scope *plan.Scope, selector analyzer.RuleSelector, qFlags *sql.QueryFlags) (sql.Node, transform.TreeIdentity, error) { |
There was a problem hiding this comment.
Function comment for this
| ) | ||
|
|
||
| func TestLimitOffset(t *testing.T) { | ||
| RunScripts(t, []ScriptTest{ |
There was a problem hiding this comment.
We could probably throw these into Smoke since it's just one test in the file.
| case *gms_expression.Arithmetic: | ||
| switch e.Operator() { | ||
| case "+": | ||
| expr, err := expression.NewBinaryOperator(framework.Operator_BinaryPlus).WithResolvedChildren(childrenAsAnySlice(e)) |
There was a problem hiding this comment.
Seems way better to replace this with a call to WithChildren. Currently WithChildren makes an assumption that we've already found the operator function, but we could change it to:
func (b *BinaryOperator) WithChildren(children ...sql.Expression) (sql.Expression, error) {
if len(children) != 2 {
return nil, sql.ErrInvalidChildrenNumber.New(b, len(children), 2)
}
if b.compiledFunc != nil {
compiledFunc, err := b.compiledFunc.WithChildren(children...)
if err != nil {
return nil, err
}
return &BinaryOperator{
operator: b.operator,
compiledFunc: compiledFunc.(framework.Function),
}, nil
} else {
binOp, err := b.WithResolvedChildren([]any{children[0], children[1]})
if err != nil {
return nil, err
}
return binOp.(sql.Expression), nil
}
}Using WithResolvedChildren outside of the GMS-only expectation adds a potential logical dependency that we probably shouldn't make a precedent for. If WithChildren calls it internally, then its usage is more of an implementation detail than a sort of API contract.
We could also make this WithChildren change to unary_operator since they're very similar expressions.
| }) | ||
| } | ||
|
|
||
| func childrenAsAnySlice(e sql.Expression) []any { |
There was a problem hiding this comment.
We won't need this function anymore with the change I mentioned earlier, but I would normally say to leave a function comment. Hard to over comment. Technically possible, but probably nobody but me is gonna over comment on anything lol
| Expected: []sql.Row{{2, 0}, {3, 0}}, | ||
| }, | ||
| { | ||
| Query: "with recursive cte as ((select * from xy order by y asc limit 1 offset 1) union (select * from xy order by y asc limit 1 offset 2)) select * from cte", |
There was a problem hiding this comment.
I'm not seeing these tests being added to our actual tests anywhere.
In GMS we used this TestSingleScript workflow because we didn't have anything better, but we do have something better in Doltgres with the Focus field on scripts. That way, you could write these in SmokeTests (or wherever) to test your logic, and throw on that Focus: true flag so that only these run. In effect you get the same experience here. But then when you're done, delete the flag, and now they're actual tests in our suite (with the added benefit of CI protection in case you forget to remove the flag). Easy way to expand our test coverage since you don't have to copy the tests out later, we're writing new tests directly into our normal test structures.
No description provided.