Skip to content

Commit

Permalink
Fix ids query DSL (#1135)
Browse files Browse the repository at this point in the history
By accident I noticed our SQL generated for `ids` looks quite
nonsensical, e.g. that's what's generated for my new very simple
request:
```
WHERE "@timestamp" IN toDateTime64('2024-12-21 07:29:03.367', '2024-12-21 07:29:02.992', 3)
```
Also noticed we have 0 tests for this query. Fixing those 2 things here.
(adding `TupleExpr` for just this 1 usecase might seem unnecessary, but
it's needed in my 2 other open PRs, so I'd do that)

Also, minor: changed `return model.NewSimpleQuery(nil, false)` to more
descriptive `return model.NewSimpleQueryInvalid()` everywhere.
  • Loading branch information
trzysiek authored Dec 23, 2024
1 parent 876f88d commit cf0fd3f
Show file tree
Hide file tree
Showing 8 changed files with 196 additions and 70 deletions.
13 changes: 12 additions & 1 deletion quesma/logger/log_with_throttling.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (
"time"
)

var throttleMap = util.SyncMap[string, time.Time]{}
var throttleMap = util.NewSyncMap[string, time.Time]()

const throttleDuration = 30 * time.Minute

Expand All @@ -25,3 +25,14 @@ func WarnWithCtxAndThrottling(ctx context.Context, aggrName, paramName, format s
throttleMap.Store(mapKey, time.Now())
}
}

// WarnWithThrottling - logs a warning message with throttling.
// We only log once per throttleDuration for each warnName, so that we don't spam the logs.
func WarnWithThrottling(warnName, format string, v ...any) {
timestamp, ok := throttleMap.Load(warnName)
weThrottle := ok && time.Since(timestamp) < throttleDuration
if !weThrottle {
Warn().Msgf(format, v...)
throttleMap.Store(warnName, time.Now())
}
}
9 changes: 9 additions & 0 deletions quesma/model/base_visitor.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package model
type BaseExprVisitor struct {
OverrideVisitFunction func(b *BaseExprVisitor, e FunctionExpr) interface{}
OverrideVisitLiteral func(b *BaseExprVisitor, l LiteralExpr) interface{}
OverrideVisitTuple func(b *BaseExprVisitor, t TupleExpr) interface{}
OverrideVisitInfix func(b *BaseExprVisitor, e InfixExpr) interface{}
OverrideVisitColumnRef func(b *BaseExprVisitor, e ColumnRef) interface{}
OverrideVisitPrefixExpr func(b *BaseExprVisitor, e PrefixExpr) interface{}
Expand Down Expand Up @@ -43,6 +44,14 @@ func (v *BaseExprVisitor) VisitLiteral(e LiteralExpr) interface{} {

return NewLiteral(e.Value)
}

func (v *BaseExprVisitor) VisitTuple(e TupleExpr) interface{} {
if v.OverrideVisitTuple != nil {
return v.OverrideVisitTuple(v, e)
}
return NewTupleExpr(v.VisitChildren(e.Exprs)...)
}

func (v *BaseExprVisitor) VisitInfix(e InfixExpr) interface{} {
if v.OverrideVisitInfix != nil {
return v.OverrideVisitInfix(v, e)
Expand Down
14 changes: 14 additions & 0 deletions quesma/model/expr.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ type Expr interface {
var (
InvalidExpr = Expr(nil)
TrueExpr = NewLiteral(true)
FalseExpr = NewLiteral(false)
)

// ColumnRef is a reference to a column in a table, we can enrich it with more information (e.g. type used) as we go
Expand Down Expand Up @@ -86,6 +87,18 @@ func (e LiteralExpr) Accept(v ExprVisitor) interface{} {
return v.VisitLiteral(e)
}

type TupleExpr struct {
Exprs []Expr
}

func NewTupleExpr(exprs ...Expr) TupleExpr {
return TupleExpr{Exprs: exprs}
}

func (e TupleExpr) Accept(v ExprVisitor) interface{} {
return v.VisitTuple(e)
}

type InfixExpr struct {
Left Expr
Op string
Expand Down Expand Up @@ -278,6 +291,7 @@ func (e CTE) Accept(v ExprVisitor) interface{} {
type ExprVisitor interface {
VisitFunction(e FunctionExpr) interface{}
VisitLiteral(l LiteralExpr) interface{}
VisitTuple(t TupleExpr) interface{}
VisitInfix(e InfixExpr) interface{}
VisitColumnRef(e ColumnRef) interface{}
VisitPrefixExpr(e PrefixExpr) interface{}
Expand Down
17 changes: 17 additions & 0 deletions quesma/model/expr_string_renderer.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package model

import (
"fmt"
"quesma/logger"
"quesma/quesma/types"
"regexp"
"sort"
Expand Down Expand Up @@ -68,6 +69,22 @@ func (v *renderer) VisitLiteral(l LiteralExpr) interface{} {
return fmt.Sprintf("%v", l.Value)
}

func (v *renderer) VisitTuple(t TupleExpr) interface{} {
switch len(t.Exprs) {
case 0:
logger.WarnWithThrottling("VisitTuple", "TupleExpr with no expressions")
return "()"
case 1:
return t.Exprs[0].Accept(v)
default:
args := make([]string, len(t.Exprs))
for i, arg := range t.Exprs {
args[i] = arg.Accept(v).(string)
}
return fmt.Sprintf("tuple(%s)", strings.Join(args, ", ")) // can omit "tuple", but I think SQL's more readable with it
}
}

func (v *renderer) VisitInfix(e InfixExpr) interface{} {
var lhs, rhs interface{} // TODO FOR NOW LITTLE PARANOID BUT HELPS ME NOT SEE MANY PANICS WHEN TESTING
if e.Left != nil {
Expand Down
4 changes: 4 additions & 0 deletions quesma/model/simple_query.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ func NewSimpleQuery(whereClause Expr, canParse bool) SimpleQuery {
return SimpleQuery{WhereClause: whereClause, CanParse: canParse}
}

func NewSimpleQueryInvalid() SimpleQuery {
return SimpleQuery{CanParse: false}
}

// LimitForCount returns (limit, true) if we need count(*) with limit,
// (not-important, false) if we don't need count/limit
func (s *SimpleQuery) LimitForCount() (limit int, doWeNeedLimit bool) {
Expand Down
Loading

0 comments on commit cf0fd3f

Please sign in to comment.