Skip to content

Commit 9b920cf

Browse files
rodrigozhourodrigozhou
committed
Refactor SQL query converter
1 parent 4d5b279 commit 9b920cf

File tree

5 files changed

+127
-77
lines changed

5 files changed

+127
-77
lines changed

‎common/persistence/visibility/store/sql/query_converter.go‎

Lines changed: 46 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ var (
7676
// strings.Replacer takes a sequence of old to new replacements
7777
escapeCharMap = []string{
7878
"'", "''",
79-
`"`, `\\"`,
79+
"\"", "\\\"",
8080
"\b", "\\b",
8181
"\n", "\\n",
8282
"\r", "\\r",
@@ -277,7 +277,7 @@ func (c *QueryConverter) convertWhereExpr(expr *sqlparser.Expr) error {
277277
case *sqlparser.FuncExpr:
278278
return query.NewConverterError("%s: function expression", query.NotSupportedErrMessage)
279279
case *sqlparser.ColName:
280-
return query.NewConverterError("incomplete expression")
280+
return query.NewConverterError("%s: incomplete expression", query.InvalidExpressionErrMessage)
281281
default:
282282
return query.NewConverterError("%s: expression of type %T", query.NotSupportedErrMessage, e)
283283
}
@@ -286,7 +286,7 @@ func (c *QueryConverter) convertWhereExpr(expr *sqlparser.Expr) error {
286286
func (c *QueryConverter) convertAndExpr(exprRef *sqlparser.Expr) error {
287287
expr, ok := (*exprRef).(*sqlparser.AndExpr)
288288
if !ok {
289-
return query.NewConverterError("%v is not an 'and' expression", sqlparser.String(*exprRef))
289+
return query.NewConverterError("`%s` is not an 'AND' expression", sqlparser.String(*exprRef))
290290
}
291291
err := c.convertWhereExpr(&expr.Left)
292292
if err != nil {
@@ -298,7 +298,7 @@ func (c *QueryConverter) convertAndExpr(exprRef *sqlparser.Expr) error {
298298
func (c *QueryConverter) convertOrExpr(exprRef *sqlparser.Expr) error {
299299
expr, ok := (*exprRef).(*sqlparser.OrExpr)
300300
if !ok {
301-
return query.NewConverterError("%v is not an 'or' expression", sqlparser.String(*exprRef))
301+
return query.NewConverterError("`%s` is not an 'OR' expression", sqlparser.String(*exprRef))
302302
}
303303
err := c.convertWhereExpr(&expr.Left)
304304
if err != nil {
@@ -310,19 +310,9 @@ func (c *QueryConverter) convertOrExpr(exprRef *sqlparser.Expr) error {
310310
func (c *QueryConverter) convertComparisonExpr(exprRef *sqlparser.Expr) error {
311311
expr, ok := (*exprRef).(*sqlparser.ComparisonExpr)
312312
if !ok {
313-
return query.NewConverterError("%v is not a comparison expression", sqlparser.String(*exprRef))
314-
}
315-
316-
saName, saFieldName, err := c.convertColName(&expr.Left)
317-
if err != nil {
318-
return err
319-
}
320-
saType, err := c.saTypeMap.GetType(saFieldName)
321-
if err != nil {
322313
return query.NewConverterError(
323-
"%s: column name '%s' is not a valid search attribute",
324-
query.InvalidExpressionErrMessage,
325-
saName,
314+
"`%s` is not a comparison expression",
315+
sqlparser.String(*exprRef),
326316
)
327317
}
328318

@@ -335,11 +325,16 @@ func (c *QueryConverter) convertComparisonExpr(exprRef *sqlparser.Expr) error {
335325
)
336326
}
337327

338-
err = c.convertValueExpr(&expr.Right, saName, saType)
328+
saColNameExpr, err := c.convertColName(&expr.Left)
339329
if err != nil {
340330
return err
341331
}
342-
switch saType {
332+
333+
err = c.convertValueExpr(&expr.Right, saColNameExpr.alias, saColNameExpr.valueType)
334+
if err != nil {
335+
return err
336+
}
337+
switch saColNameExpr.valueType {
343338
case enumspb.INDEXED_VALUE_TYPE_KEYWORD_LIST:
344339
newExpr, err := c.convertKeywordListComparisonExpr(expr)
345340
if err != nil {
@@ -360,74 +355,79 @@ func (c *QueryConverter) convertRangeCond(exprRef *sqlparser.Expr) error {
360355
expr, ok := (*exprRef).(*sqlparser.RangeCond)
361356
if !ok {
362357
return query.NewConverterError(
363-
"%v is not a range condition expression",
358+
"`%s` is not a range condition expression",
364359
sqlparser.String(*exprRef),
365360
)
366361
}
367-
saName, saFieldName, err := c.convertColName(&expr.Left)
362+
saColNameExpr, err := c.convertColName(&expr.Left)
368363
if err != nil {
369364
return err
370365
}
371-
saType, err := c.saTypeMap.GetType(saFieldName)
372-
if err != nil {
373-
return query.NewConverterError(
374-
"%s: column name '%s' is not a valid search attribute",
375-
query.InvalidExpressionErrMessage,
376-
saName,
377-
)
378-
}
379-
if !isSupportedTypeRangeCond(saType) {
366+
if !isSupportedTypeRangeCond(saColNameExpr.valueType) {
380367
return query.NewConverterError(
381368
"%s: cannot do range condition on search attribute '%s' of type %s",
382369
query.InvalidExpressionErrMessage,
383-
saName,
384-
saType.String(),
370+
saColNameExpr.alias,
371+
saColNameExpr.valueType.String(),
385372
)
386373
}
387-
err = c.convertValueExpr(&expr.From, saName, saType)
374+
err = c.convertValueExpr(&expr.From, saColNameExpr.alias, saColNameExpr.valueType)
388375
if err != nil {
389376
return err
390377
}
391-
err = c.convertValueExpr(&expr.To, saName, saType)
378+
err = c.convertValueExpr(&expr.To, saColNameExpr.alias, saColNameExpr.valueType)
392379
if err != nil {
393380
return err
394381
}
395382
return nil
396383
}
397384

398-
func (c *QueryConverter) convertColName(
399-
exprRef *sqlparser.Expr,
400-
) (saAlias string, saFieldName string, retError error) {
385+
func (c *QueryConverter) convertColName(exprRef *sqlparser.Expr) (*saColName, error) {
401386
expr, ok := (*exprRef).(*sqlparser.ColName)
402387
if !ok {
403-
return "", "", query.NewConverterError(
388+
return nil, query.NewConverterError(
404389
"%s: must be a column name but was %T",
405390
query.InvalidExpressionErrMessage,
406391
*exprRef,
407392
)
408393
}
409-
saAlias = strings.ReplaceAll(sqlparser.String(expr), "`", "")
410-
saFieldName = saAlias
394+
saAlias := strings.ReplaceAll(sqlparser.String(expr), "`", "")
395+
saFieldName := saAlias
411396
if searchattribute.IsMappable(saAlias) {
412397
var err error
413398
saFieldName, err = c.saMapper.GetFieldName(saAlias, c.namespaceName.String())
414399
if err != nil {
415-
return "", "", query.NewConverterError(
400+
return nil, query.NewConverterError(
416401
"%s: column name '%s' is not a valid search attribute",
417402
query.InvalidExpressionErrMessage,
418403
saAlias,
419404
)
420405
}
421406
}
407+
saType, err := c.saTypeMap.GetType(saFieldName)
408+
if err != nil {
409+
// This should never happen since it came from mapping.
410+
return nil, query.NewConverterError(
411+
"%s: column name '%s' is not a valid search attribute",
412+
query.InvalidExpressionErrMessage,
413+
saAlias,
414+
)
415+
}
422416
if saFieldName == searchattribute.TemporalNamespaceDivision {
423417
c.seenNamespaceDivision = true
424418
}
425-
var newExpr sqlparser.Expr = newColName(searchattribute.GetSqlDbColName(saFieldName))
426419
if saAlias == searchattribute.CloseTime {
427-
newExpr = c.getCoalesceCloseTimeExpr()
428-
}
420+
*exprRef = c.getCoalesceCloseTimeExpr()
421+
return closeTimeSaColName, nil
422+
}
423+
newExpr := newSAColName(
424+
searchattribute.GetSqlDbColName(saFieldName),
425+
saAlias,
426+
saFieldName,
427+
saType,
428+
)
429429
*exprRef = newExpr
430-
return saAlias, saFieldName, nil
430+
return newExpr, nil
431431
}
432432

433433
func (c *QueryConverter) convertValueExpr(
@@ -573,20 +573,12 @@ func (c *QueryConverter) parseSQLVal(
573573
func (c *QueryConverter) convertIsExpr(exprRef *sqlparser.Expr) error {
574574
expr, ok := (*exprRef).(*sqlparser.IsExpr)
575575
if !ok {
576-
return query.NewConverterError("%v is not an 'IS' expression", sqlparser.String(*exprRef))
576+
return query.NewConverterError("`%s` is not an 'IS' expression", sqlparser.String(*exprRef))
577577
}
578-
saName, saFieldName, err := c.convertColName(&expr.Expr)
578+
_, err := c.convertColName(&expr.Expr)
579579
if err != nil {
580580
return err
581581
}
582-
_, err = c.saTypeMap.GetType(saFieldName)
583-
if err != nil {
584-
return query.NewConverterError(
585-
"%s: column name '%s' is not a valid search attribute",
586-
query.InvalidExpressionErrMessage,
587-
saName,
588-
)
589-
}
590582
switch expr.Operator {
591583
case sqlparser.IsNullStr, sqlparser.IsNotNullStr:
592584
// no-op

‎common/persistence/visibility/store/sql/query_converter_mysql.go‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ func (c *mysqlQueryConverter) getDatetimeFormat() string {
105105
func (c *mysqlQueryConverter) getCoalesceCloseTimeExpr() sqlparser.Expr {
106106
return newFuncExpr(
107107
coalesceFuncName,
108-
newColName(searchattribute.GetSqlDbColName(searchattribute.CloseTime)),
108+
closeTimeSaColName,
109109
&castExpr{
110110
Value: newUnsafeSQLString(maxDatetimeValue.Format(c.getDatetimeFormat())),
111111
Type: convertTypeDatetime,
@@ -118,9 +118,10 @@ func (c *mysqlQueryConverter) convertKeywordListComparisonExpr(
118118
) (sqlparser.Expr, error) {
119119
if !isSupportedKeywordListOperator(expr.Operator) {
120120
return nil, query.NewConverterError(
121-
"%s: operator %s not supported for KeywordList type search attribute",
121+
"%s: operator '%s' not supported for KeywordList type search attribute in `%s`",
122122
query.InvalidExpressionErrMessage,
123123
expr.Operator,
124+
formatComparisonExprStringForError(*expr),
124125
)
125126
}
126127

@@ -143,9 +144,10 @@ func (c *mysqlQueryConverter) convertKeywordListComparisonExpr(
143144
default:
144145
// this should never happen since isSupportedKeywordListOperator should already fail
145146
return nil, query.NewConverterError(
146-
"%s: operator %s not supported for KeywordList type search attribute",
147+
"%s: operator '%s' not supported for KeywordList type search attribute in `%s`",
147148
query.InvalidExpressionErrMessage,
148149
expr.Operator,
150+
formatComparisonExprStringForError(*expr),
149151
)
150152
}
151153

@@ -188,9 +190,10 @@ func (c *mysqlQueryConverter) convertTextComparisonExpr(
188190
) (sqlparser.Expr, error) {
189191
if !isSupportedTextOperator(expr.Operator) {
190192
return nil, query.NewConverterError(
191-
"%s: operator %s not supported for Text type search attribute",
193+
"%s: operator '%s' not supported for Text type search attribute in `%s`",
192194
query.InvalidExpressionErrMessage,
193195
expr.Operator,
196+
formatComparisonExprStringForError(*expr),
194197
)
195198
}
196199
// build the following expression:

‎common/persistence/visibility/store/sql/query_converter_postgresql.go‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ func (c *pgQueryConverter) getDatetimeFormat() string {
8686
func (c *pgQueryConverter) getCoalesceCloseTimeExpr() sqlparser.Expr {
8787
return newFuncExpr(
8888
coalesceFuncName,
89-
newColName(searchattribute.GetSqlDbColName(searchattribute.CloseTime)),
89+
closeTimeSaColName,
9090
newUnsafeSQLString(maxDatetimeValue.Format(c.getDatetimeFormat())),
9191
)
9292
}
@@ -96,9 +96,10 @@ func (c *pgQueryConverter) convertKeywordListComparisonExpr(
9696
) (sqlparser.Expr, error) {
9797
if !isSupportedKeywordListOperator(expr.Operator) {
9898
return nil, query.NewConverterError(
99-
"%s: operator %s not supported for KeywordList type search attribute",
99+
"%s: operator '%s' not supported for KeywordList type search attribute in `%s`",
100100
query.InvalidExpressionErrMessage,
101101
expr.Operator,
102+
formatComparisonExprStringForError(*expr),
102103
)
103104
}
104105

@@ -128,9 +129,10 @@ func (c *pgQueryConverter) convertKeywordListComparisonExpr(
128129
default:
129130
// this should never happen since isSupportedKeywordListOperator should already fail
130131
return nil, query.NewConverterError(
131-
"%s: operator %s not supported for KeywordList type search attribute",
132+
"%s: operator '%s' not supported for KeywordList type search attribute in `%s`",
132133
query.InvalidExpressionErrMessage,
133134
expr.Operator,
135+
formatComparisonExprStringForError(*expr),
134136
)
135137
}
136138
}
@@ -166,9 +168,10 @@ func (c *pgQueryConverter) convertTextComparisonExpr(
166168
) (sqlparser.Expr, error) {
167169
if !isSupportedTextOperator(expr.Operator) {
168170
return nil, query.NewConverterError(
169-
"%s: operator %s not supported for Text type search attribute",
171+
"%s: operator '%s' not supported for Text type search attribute in `%s`",
170172
query.InvalidExpressionErrMessage,
171173
expr.Operator,
174+
formatComparisonExprStringForError(*expr),
172175
)
173176
}
174177
valueExpr, ok := expr.Right.(*unsafeSQLString)

0 commit comments

Comments
 (0)