Skip to content

Commit 1097334

Browse files
authored
Merge pull request #1408 from dolthub/zachmu/in-fix
Bug fix for dolt_ tables with incompatible types
2 parents 6a14386 + be604ae commit 1097334

File tree

3 files changed

+43
-13
lines changed

3 files changed

+43
-13
lines changed

server/analyzer/type_sanitizer.go

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ package analyzer
1717
import (
1818
"context"
1919
"strconv"
20+
"strings"
2021
"time"
2122

2223
"github.com/cockroachdb/errors"
@@ -39,10 +40,23 @@ import (
3940
// to GMS types, so by taking care of all conversions here, we can ensure that Doltgres only needs to worry about its
4041
// own types.
4142
func TypeSanitizer(ctx *sql.Context, a *analyzer.Analyzer, node sql.Node, scope *plan.Scope, selector analyzer.RuleSelector, qFlags *sql.QueryFlags) (sql.Node, transform.TreeIdentity, error) {
42-
return pgtransform.NodeExprsWithOpaque(node, func(expr sql.Expression) (sql.Expression, transform.TreeIdentity, error) {
43+
return pgtransform.NodeExprsWithNodeWithOpaque(node, func(n sql.Node, expr sql.Expression) (sql.Expression, transform.TreeIdentity, error) {
4344
// This can be updated if we find more expressions that return GMS types.
4445
// These should eventually be replaced with Doltgres-equivalents over time, rendering this function unnecessary.
4546
switch expr := expr.(type) {
47+
case *expression.GetField:
48+
switch n := n.(type) {
49+
case *plan.Project, *plan.Filter, *plan.GroupBy:
50+
child := n.Children()[0]
51+
// Some dolt_ tables do not have doltgres types for their columns, so we convert them here
52+
if rt, ok := child.(*plan.ResolvedTable); ok && strings.HasPrefix(rt.Name(), "dolt_") {
53+
// This is a projection on a table, so we can safely convert the type
54+
if _, ok := expr.Type().(*pgtypes.DoltgresType); !ok {
55+
return pgexprs.NewGMSCast(expr), transform.NewTree, nil
56+
}
57+
}
58+
}
59+
return expr, transform.SameTree, nil
4660
case *expression.Literal:
4761
return typeSanitizerLiterals(ctx, expr)
4862
case *expression.Not, *expression.And, *expression.Or, *expression.Like:

server/expression/gms_cast.go

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,12 @@
1515
package expression
1616

1717
import (
18-
"strconv"
18+
"encoding/json"
19+
"math"
1920
"time"
2021

2122
"github.com/cockroachdb/errors"
23+
"github.com/dolthub/dolt/go/store/prolly/tree"
2224
"github.com/dolthub/go-mysql-server/sql"
2325
"github.com/dolthub/go-mysql-server/sql/types"
2426
"github.com/dolthub/vitess/go/vt/proto/query"
@@ -124,8 +126,12 @@ func (c *GMSCast) Eval(ctx *sql.Context, row sql.Row) (any, error) {
124126
}
125127
return newVal, nil
126128
case query.Type_UINT64:
129+
// Postgres doesn't have a Uint64 type, so we return an int64 with an error if the value is too high
127130
if val, ok := val.(uint64); ok {
128-
return decimal.NewFromString(strconv.FormatUint(val, 10))
131+
if val > math.MaxInt64 {
132+
return nil, errors.Errorf("uint64 value out of range: %v", val)
133+
}
134+
return int64(val), nil
129135
}
130136
return nil, errors.Errorf("GMSCast expected type `uint64`, got `%T`", val)
131137
case query.Type_FLOAT32:
@@ -158,15 +164,26 @@ func (c *GMSCast) Eval(ctx *sql.Context, row sql.Row) (any, error) {
158164
if err != nil {
159165
return nil, err
160166
}
161-
if _, ok := newVal.(string); !ok {
167+
switch newVal := newVal.(type) {
168+
case string:
169+
return newVal, nil
170+
case sql.StringWrapper:
171+
return newVal.Unwrap(ctx)
172+
default:
162173
return nil, errors.Errorf("GMSCast expected type `string`, got `%T`", val)
163174
}
164-
return newVal, nil
165175
case query.Type_JSON:
166-
if val, ok := val.(types.JSONDocument); ok {
176+
switch val := val.(type) {
177+
case types.JSONDocument:
167178
return val.JSONString()
179+
case tree.IndexedJsonDocument:
180+
return val.String(), nil
181+
default:
182+
// TODO: there are particular dolt tables (dolt_constraint_violations) that return json-marshallable structs
183+
// that we need to handle here like this
184+
bytes, err := json.Marshal(val)
185+
return string(bytes), err
168186
}
169-
return nil, errors.Errorf("GMSCast expected type `JSONDocument`, got `%T`", val)
170187
case query.Type_NULL_TYPE:
171188
return nil, nil
172189
case query.Type_GEOMETRY:

testing/go/dolt_tables_test.go

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,6 @@ func TestUserSpaceDoltTables(t *testing.T) {
106106
Expected: []sql.Row{{1}},
107107
},
108108
{
109-
Skip: true, // this fails because the column type of this table is not a doltgres type, which IN requires
110109
Query: `SELECT "dolt_branches"."name" FROM "dolt_branches" WHERE "dolt_branches"."name" IN ('main') ORDER BY "dolt_branches"."name" DESC LIMIT 21;`,
111110
Expected: []sql.Row{{"main"}},
112111
},
@@ -555,7 +554,7 @@ func TestUserSpaceDoltTables(t *testing.T) {
555554
},
556555
{
557556
Query: `SELECT * FROM dolt_conflicts`,
558-
Expected: []sql.Row{{"test", Numeric("1")}},
557+
Expected: []sql.Row{{"test", 1}},
559558
},
560559
{
561560
Query: `SELECT dolt.conflicts.table FROM dolt.conflicts`,
@@ -763,7 +762,7 @@ func TestUserSpaceDoltTables(t *testing.T) {
763762
},
764763
{
765764
Query: `SELECT * FROM dolt_constraint_violations`,
766-
Expected: []sql.Row{{"test", Numeric("2")}},
765+
Expected: []sql.Row{{"test", 2}},
767766
},
768767
{
769768
Query: `SELECT dolt.constraint_violations.table FROM dolt.constraint_violations`,
@@ -1744,8 +1743,8 @@ func TestUserSpaceDoltTables(t *testing.T) {
17441743
},
17451744
},
17461745
},
1747-
//TODO: turn on statistics
1748-
//{
1746+
// TODO: turn on statistics
1747+
// {
17491748
// Name: "dolt statistics",
17501749
// SetUpScript: []string{
17511750
// "CREATE TABLE horses (id int primary key, name varchar(10));",
@@ -1871,7 +1870,7 @@ func TestUserSpaceDoltTables(t *testing.T) {
18711870
// Expected: []sql.Row{{"horses", "horses_name_idx"}, {"horses", "primary"}},
18721871
// },
18731872
// },
1874-
//},
1873+
// },
18751874
{
18761875
Name: "dolt status",
18771876
SetUpScript: []string{

0 commit comments

Comments
 (0)