From a158bba125dd4584635c97f69f8f50562810c60b Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 7 Apr 2023 17:02:37 -0500 Subject: [PATCH] Simplify/centralize time parsing We had a bunch of different places which had basically the same logic, except that some were testing both RFC3339Nano and RFC3339 formats, and some weren't. This turns out not to matter, because the fractional second part is always permitted and never required, so those two formats are identical. Mostly, though, we now ensure that everything we do that is trying to convert timestamps has the same logic, so if we want to make changes to that logic, we have a central point, which lives in the parser. This came out of an attempt to figure out why the RFC3339 case wasn't getting any test coverage. --- sql3/parser/ast.go | 21 +++++++++------ sql3/planner/expression.go | 41 +++++++----------------------- sql3/planner/expression_it_test.go | 3 +-- sql3/planner/opbulkinsert.go | 24 +++++++---------- sql3/planner/opinsert.go | 2 +- sql3/planner/oppqldistinctscan.go | 3 +-- sql3/sql_complex_test.go | 7 ++--- sql3/test/defs/defs.go | 7 ++--- sql3/test/defs/defs_delete.go | 10 +++++--- sql3/test/defs/defs_sql1.go | 4 ++- 10 files changed, 52 insertions(+), 70 deletions(-) diff --git a/sql3/parser/ast.go b/sql3/parser/ast.go index befbac498..241660d4e 100644 --- a/sql3/parser/ast.go +++ b/sql3/parser/ast.go @@ -1838,16 +1838,21 @@ func (expr *StringLit) Pos() Pos { } func (expr *StringLit) ConvertToTimestamp() *DateLit { - //try to coerce to a date - if tm, err := time.ParseInLocation(time.RFC3339Nano, expr.Value, time.UTC); err == nil { - return &DateLit{ValuePos: expr.ValuePos, Value: tm} - } else if tm, err := time.ParseInLocation(time.RFC3339, expr.Value, time.UTC); err == nil { - return &DateLit{ValuePos: expr.ValuePos, Value: tm} - } else if tm, err := time.ParseInLocation("2006-01-02", expr.Value, time.UTC); err == nil { - return &DateLit{ValuePos: expr.ValuePos, Value: tm} - } else { + tm, err := ConvertStringToTimestamp(expr.Value) + if err != nil { return nil } + return &DateLit{ValuePos: expr.ValuePos, Value: tm} +} + +func ConvertStringToTimestamp(date string) (time.Time, error) { + if tm, err := time.ParseInLocation(time.RFC3339, date, time.UTC); err == nil { + return tm, nil + } else if tm, err := time.ParseInLocation("2006-01-02", date, time.UTC); err == nil { + return tm, nil + } else { + return time.Time{}, err + } } // Clone returns a deep copy of lit. diff --git a/sql3/planner/expression.go b/sql3/planner/expression.go index 721ff86e3..b53e76406 100644 --- a/sql3/planner/expression.go +++ b/sql3/planner/expression.go @@ -89,13 +89,11 @@ func coerceValue(sourceType parser.ExprDataType, targetType parser.ExprDataType, if !ok { return nil, sql3.NewErrInternalf("unexpected value type '%T'", value) } - if tm, err := time.ParseInLocation(time.RFC3339Nano, val, time.UTC); err == nil { - return tm, nil - } else if tm, err := time.ParseInLocation("2006-01-02", val, time.UTC); err == nil { - return tm, nil - } else { + tm, err := parser.ConvertStringToTimestamp(val) + if err != nil { return nil, sql3.NewErrInvalidTypeCoercion(0, 0, val, targetType.TypeDescription()) } + return tm, nil } case *parser.DataTypeTimestamp: @@ -2140,16 +2138,11 @@ func (n *stringLiteralPlanExpression) WithChildren(children ...types.PlanExpress } func (expr *stringLiteralPlanExpression) ConvertToTimestamp() *time.Time { - // try to coerce to a date - if tm, err := time.ParseInLocation(time.RFC3339Nano, expr.value, time.UTC); err == nil { - return &tm - } else if tm, err := time.ParseInLocation(time.RFC3339, expr.value, time.UTC); err == nil { - return &tm - } else if tm, err := time.ParseInLocation("2006-01-02", expr.value, time.UTC); err == nil { - return &tm - } else { + tm, err := parser.ConvertStringToTimestamp(expr.value) + if err != nil { return nil } + return &tm } // castPlanExpressionis a cast op @@ -2289,15 +2282,11 @@ func (n *castPlanExpression) Evaluate(currentRow []interface{}) (interface{}, er return nl, nil case *parser.DataTypeTimestamp: - if tm, err := time.ParseInLocation(time.RFC3339Nano, nl, time.UTC); err == nil { - return tm, nil - } else if tm, err := time.ParseInLocation(time.RFC3339, nl, time.UTC); err == nil { - return tm, nil - } else if tm, err := time.ParseInLocation("2006-01-02", nl, time.UTC); err == nil { - return tm, nil - } else { + tm, err := parser.ConvertStringToTimestamp(nl) + if err != nil { return nil, sql3.NewErrInvalidCast(0, 0, nl, n.targetType.TypeDescription()) } + return tm, nil } case *parser.DataTypeStringSet: @@ -2999,15 +2988,3 @@ func wildCardToRegexp(pattern string) string { return result.String() } - -// timeFromString attempts to parse the string to a time.Time using a series of -// time formats. -func timestampFromString(s string) (time.Time, error) { - if tm, err := time.ParseInLocation(time.RFC3339Nano, s, time.UTC); err == nil { - return tm, nil - } else if tm, err := time.ParseInLocation("2006-01-02", s, time.UTC); err == nil { - return tm, nil - } - - return time.Time{}, sql3.NewErrInvalidTypeCoercion(0, 0, s, "time.Time") -} diff --git a/sql3/planner/expression_it_test.go b/sql3/planner/expression_it_test.go index d9f8d870b..f39e0486c 100644 --- a/sql3/planner/expression_it_test.go +++ b/sql3/planner/expression_it_test.go @@ -2,7 +2,6 @@ package planner import ( "testing" - "time" "github.com/featurebasedb/featurebase/v3/sql3/parser" "github.com/featurebasedb/featurebase/v3/sql3/planner/types" @@ -64,7 +63,7 @@ func TestExpressions(t *testing.T) { blop := newBoolLiteralPlanExpression(false) assert.Equal(t, blop.String(), "false") - tm, _ := time.ParseInLocation(time.RFC3339, "2012-11-01T22:08:41+00:00", time.UTC) + tm, _ := parser.ConvertStringToTimestamp("2012-11-01T22:08:41+00:00") dlop := newTimestampLiteralPlanExpression(tm) assert.Equal(t, dlop.String(), "2012-11-01T22:08:41Z") diff --git a/sql3/planner/opbulkinsert.go b/sql3/planner/opbulkinsert.go index 2586ad34b..91f943763 100644 --- a/sql3/planner/opbulkinsert.go +++ b/sql3/planner/opbulkinsert.go @@ -317,13 +317,11 @@ func (i *bulkInsertSourceCSVRowIter) Next(ctx context.Context) (types.Row, error case *parser.DataTypeTimestamp: intVal, err := strconv.ParseInt(evalValue, 10, 64) if err != nil { - if tm, err := time.ParseInLocation(time.RFC3339Nano, evalValue, time.UTC); err == nil { - result[idx] = tm - } else if tm, err := time.ParseInLocation("2006-01-02", evalValue, time.UTC); err == nil { - result[idx] = tm - } else { + tm, err := parser.ConvertStringToTimestamp(evalValue) + if err != nil { return nil, sql3.NewErrTypeConversionOnMap(0, 0, evalValue, mapColumn.colType.TypeDescription()) } + result[idx] = tm } else { // implicit conversion of int to timestamp will treat int as seconds since unix epoch result[idx] = time.Unix(intVal, 0).UTC() @@ -661,13 +659,11 @@ func (i *bulkInsertSourceNDJsonRowIter) Next(ctx context.Context) (types.Row, er return nil, sql3.NewErrTypeConversionOnMap(0, 0, v, mapColumn.colType.TypeDescription()) case string: - if tm, err := time.ParseInLocation(time.RFC3339Nano, v, time.UTC); err == nil { - result[idx] = tm - } else if tm, err := time.ParseInLocation("2006-01-02", v, time.UTC); err == nil { - result[idx] = tm - } else { + tm, err := parser.ConvertStringToTimestamp(v) + if err != nil { return nil, sql3.NewErrTypeConversionOnMap(0, 0, v, mapColumn.colType.TypeDescription()) } + result[idx] = tm case bool: return nil, sql3.NewErrTypeConversionOnMap(0, 0, v, mapColumn.colType.TypeDescription()) @@ -1153,13 +1149,11 @@ func (i *bulkInsertSourceParquetRowIter) Next(ctx context.Context) (types.Row, e // implicit conversion of int to timestamp will treat int as seconds since unix epoch result[idx] = time.Unix(intVal, 0).UTC() } else if stringVal, ok := evalValue.(string); ok { - if tm, err := time.ParseInLocation(time.RFC3339Nano, stringVal, time.UTC); err == nil { - result[idx] = tm - } else if tm, err := time.ParseInLocation("2006-01-02", stringVal, time.UTC); err == nil { - result[idx] = tm - } else { + tm, err := parser.ConvertStringToTimestamp(stringVal) + if err != nil { return nil, sql3.NewErrTypeConversionOnMap(0, 0, stringVal, mapColumn.colType.TypeDescription()) } + result[idx] = tm } case *parser.DataTypeString: diff --git a/sql3/planner/opinsert.go b/sql3/planner/opinsert.go index f7512456f..d18242902 100644 --- a/sql3/planner/opinsert.go +++ b/sql3/planner/opinsert.go @@ -375,7 +375,7 @@ func (i *insertRowIter) Next(ctx context.Context) (types.Row, error) { // string is the normal case for dates; used when the date is // provided as a string in the INSERT INTO statement. case string: - ts, err := timestampFromString(v) + ts, err := parser.ConvertStringToTimestamp(v) if err != nil { return nil, errors.Wrapf(err, "parsing timestamp: %s", v) } diff --git a/sql3/planner/oppqldistinctscan.go b/sql3/planner/oppqldistinctscan.go index 47a2a6a4f..f6da3d5f5 100644 --- a/sql3/planner/oppqldistinctscan.go +++ b/sql3/planner/oppqldistinctscan.go @@ -6,7 +6,6 @@ import ( "context" "fmt" "strings" - "time" pilosa "github.com/featurebasedb/featurebase/v3" "github.com/featurebasedb/featurebase/v3/dax" @@ -230,7 +229,7 @@ func (i *distinctScanRowIter) Next(ctx context.Context) (types.Row, error) { case pilosa.DistinctTimestamp: result := make([]interface{}, 0) for _, n := range res.Values { - if tm, err := time.ParseInLocation(time.RFC3339Nano, n, time.UTC); err == nil { + if tm, err := parser.ConvertStringToTimestamp(n); err == nil { result = append(result, tm) } else { return nil, sql3.NewErrInternalf("unable to convert to time.Time: %v", n) diff --git a/sql3/sql_complex_test.go b/sql3/sql_complex_test.go index 5629a8199..9715e711d 100644 --- a/sql3/sql_complex_test.go +++ b/sql3/sql_complex_test.go @@ -21,6 +21,7 @@ import ( pilosa "github.com/featurebasedb/featurebase/v3" "github.com/featurebasedb/featurebase/v3/dax" "github.com/featurebasedb/featurebase/v3/pql" + "github.com/featurebasedb/featurebase/v3/sql3/parser" sql_test "github.com/featurebasedb/featurebase/v3/sql3/test" "github.com/featurebasedb/featurebase/v3/test" "github.com/featurebasedb/featurebase/v3/vprint" @@ -3055,9 +3056,9 @@ func TestPlanner_BulkInsertParquet(t *testing.T) { // order by timestamp results, _, _, err = sql_test.MustQueryRows(t, nil, c.GetNode(0).Server, `select _id, t from j1 order by t`) assert.NoError(t, err) - t2, _ := time.ParseInLocation(time.RFC3339Nano, "1970-01-28T00:00:00Z", time.UTC) - t3, _ := time.ParseInLocation(time.RFC3339Nano, "1988-05-30T12:02:00Z", time.UTC) - t1, _ := time.ParseInLocation(time.RFC3339Nano, "2022-01-28T12:14:04Z", time.UTC) + t2, _ := parser.ConvertStringToTimestamp("1970-01-28T00:00:00Z") + t3, _ := parser.ConvertStringToTimestamp("1988-05-30T12:02:00Z") + t1, _ := parser.ConvertStringToTimestamp("2022-01-28T12:14:04Z") if diff := cmp.Diff([][]interface{}{ {int64(2), t2}, diff --git a/sql3/test/defs/defs.go b/sql3/test/defs/defs.go index f72acec2a..97a2948c1 100644 --- a/sql3/test/defs/defs.go +++ b/sql3/test/defs/defs.go @@ -11,6 +11,7 @@ import ( "github.com/PaesslerAG/gval" "github.com/PaesslerAG/jsonpath" "github.com/featurebasedb/featurebase/v3/errors" + "github.com/featurebasedb/featurebase/v3/sql3/parser" ) // TableTests is the list of tests which get run by TestSQL_Execute in @@ -219,7 +220,7 @@ var TableTests []TableTest = []TableTest{ } func knownTimestamp() time.Time { - tm, err := time.ParseInLocation(time.RFC3339, "2012-11-01T22:08:41+00:00", time.UTC) + tm, err := parser.ConvertStringToTimestamp("2012-11-01T22:08:41+00:00") if err != nil { panic(err.Error()) } @@ -237,7 +238,7 @@ func knownSubSecondTimestamp() time.Time { } func knownSubSecondTimestamp2() time.Time { - tm, err := time.ParseInLocation(time.RFC3339, "2022-12-09T18:04:54+00:00", time.UTC) + tm, err := parser.ConvertStringToTimestamp("2022-12-09T18:04:54+00:00") if err != nil { panic(err.Error()) } @@ -250,7 +251,7 @@ func knownSubSecondTimestamp2() time.Time { } func timestampFromString(s string) time.Time { - tm, err := time.ParseInLocation(time.RFC3339, s, time.UTC) + tm, err := parser.ConvertStringToTimestamp(s) if err != nil { panic(err.Error()) } diff --git a/sql3/test/defs/defs_delete.go b/sql3/test/defs/defs_delete.go index cfa6b5d49..a4ea79d3b 100644 --- a/sql3/test/defs/defs_delete.go +++ b/sql3/test/defs/defs_delete.go @@ -1,10 +1,14 @@ // Copyright 2021 Molecula Corp. All rights reserved. package defs -import "time" +import ( + "time" + + "github.com/featurebasedb/featurebase/v3/sql3/parser" +) func earlyMay2022() time.Time { - tm, err := time.ParseInLocation(time.RFC3339, "2022-05-05T13:00:00+00:00", time.UTC) + tm, err := parser.ConvertStringToTimestamp("2022-05-05T13:00:00+00:00") if err != nil { panic(err.Error()) } @@ -12,7 +16,7 @@ func earlyMay2022() time.Time { } func lateMay2022() time.Time { - tm, err := time.ParseInLocation(time.RFC3339, "2022-05-28T13:00:00+00:00", time.UTC) + tm, err := parser.ConvertStringToTimestamp("2022-05-28T13:00:00+00:00") if err != nil { panic(err.Error()) } diff --git a/sql3/test/defs/defs_sql1.go b/sql3/test/defs/defs_sql1.go index d0fa34b6c..c92232b93 100644 --- a/sql3/test/defs/defs_sql1.go +++ b/sql3/test/defs/defs_sql1.go @@ -3,6 +3,8 @@ package defs import ( "fmt" "time" + + "github.com/featurebasedb/featurebase/v3/sql3/parser" ) var sql1TestsGrouper = TableTest{ @@ -74,7 +76,7 @@ var sql1TestsDelete = TableTest{ // grouperTimeX extracts the time associated with record ID x. // note that the record IDs are 1..10, not 0..9, so we subtract one. func grouperTimeX(x int) time.Time { - t, err := time.ParseInLocation(time.RFC3339, sql1TestsGrouper.Table.rows[0][x-1][5].(string), time.UTC) + t, err := parser.ConvertStringToTimestamp(sql1TestsGrouper.Table.rows[0][x-1][5].(string)) if err != nil { panic(fmt.Sprintf("failed to parse time for id %d", x)) }