fix: scan type of float columns (#52)

Ensure that the ScanType of float columns matches the type used when
reading into *any.
With the fix we use float64 for both - same as double. Less efficient
but consistent.

Extended integration tests to confirm that ScanType matches type in *any
for all types.

Part of #19

.github/workflows/ci.yml

@@ -24,7 +24,9 @@ jobs:
       run: go build -v ./...
 
     - name: Test
-      run: make test
+      run: |
+        sudo apt-get install moreutils -y
+        make test
 
     - name: Checks
       run: |

Makefile

@@ -18,10 +18,15 @@ PKGS=$(shell go list ./... | grep -v "./internal/generated")
 PKGS_LST=$(shell echo ${PKGS} | tr ' ' ',')
 test:
 	mkdir -p coverage/covdata
-	# Use the new binary format to ensure integration tests and cross-package calls are counted towards coverage
-	# https://go.dev/blog/integration-test-coverage
-	# -coverpkg can't be ./... because that will include generated code in the stats
-	go test -race -cover -covermode atomic -coverpkg "${PKGS_LST}" -v -vet=all ${PKGS} -args -test.gocoverdir="${PWD}/coverage/covdata"
+# Use the new binary format to ensure integration tests and cross-package calls are counted towards coverage
+# https://go.dev/blog/integration-test-coverage
+# -coverpkg can't be ./... because that will include generated code in the stats
+# -p 1 disable parallel testing in favor of streaming log output - https://github.com/golang/go/issues/24929#issuecomment-384484654
+	go test -race -cover -covermode atomic -coverpkg "${PKGS_LST}" -v -vet=all -timeout 15m -p 1\
+		${PKGS} \
+		-args -test.gocoverdir="${PWD}/coverage/covdata" \
+		| ts -s
+# NB: ts command requires moreutils package; awk trick from https://stackoverflow.com/a/25764579 doesn't stream output
 	go tool covdata percent -i=./coverage/covdata
 	# Convert to old text format for coveralls upload
 	go tool covdata textfmt -i=./coverage/covdata -o ./coverage/covprofile

internal/hive/metadata.go

@@ -27,7 +27,6 @@ type ColDesc struct {
 var (
 	dataTypeNull     = reflect.TypeOf(nil)
 	dataTypeBoolean  = reflect.TypeOf(true)
-	dataTypeFloat32  = reflect.TypeOf(float32(0))
 	dataTypeFloat64  = reflect.TypeOf(float64(0))
 	dataTypeInt8     = reflect.TypeOf(int8(0))
 	dataTypeInt16    = reflect.TypeOf(int16(0))
@@ -51,9 +50,8 @@ func typeOf(entry *cli_service.TPrimitiveTypeEntry) reflect.Type {
 		return dataTypeInt32
 	case cli_service.TTypeId_BIGINT_TYPE:
 		return dataTypeInt64
-	case cli_service.TTypeId_FLOAT_TYPE:
-		return dataTypeFloat32
-	case cli_service.TTypeId_DOUBLE_TYPE:
+	case cli_service.TTypeId_FLOAT_TYPE, cli_service.TTypeId_DOUBLE_TYPE:
+		// see comment in internal/hive/result_set.go#value()
 		return dataTypeFloat64
 	case cli_service.TTypeId_NULL_TYPE:
 		return dataTypeNull
@@ -65,6 +63,7 @@ func typeOf(entry *cli_service.TPrimitiveTypeEntry) reflect.Type {
 		return dataTypeString
 	case cli_service.TTypeId_DATE_TYPE, cli_service.TTypeId_TIMESTAMP_TYPE:
 		return dataTypeDateTime
+	// TODO Support decimal Github #51
 	case cli_service.TTypeId_DECIMAL_TYPE, cli_service.TTypeId_BINARY_TYPE, cli_service.TTypeId_ARRAY_TYPE,
 		cli_service.TTypeId_STRUCT_TYPE, cli_service.TTypeId_MAP_TYPE, cli_service.TTypeId_UNION_TYPE:
 		return dataTypeRawBytes

internal/hive/result_set.go

@@ -53,45 +53,52 @@ func (rs *ResultSet) Next(dest []driver.Value) error {
 	return nil
 }
 
+// isSet checks if the i-th member of the provided bitmap is set. Each byte contains 8 bit flags.
+func isSet(bitmap []byte, i int) bool {
+	return bitmap[i/8]&(1<<(uint(i)%8)) != 0
+}
+
 func value(col *cli_service.TColumn, cd *ColDesc, i int) (interface{}, error) {
 	switch cd.DatabaseTypeName {
 	case "STRING", "CHAR", "VARCHAR":
-		if col.StringVal.Nulls[i/8]&(1<<(uint(i)%8)) != 0 {
+		if isSet(col.StringVal.Nulls, i) {
 			return nil, nil
 		}
 		return col.StringVal.Values[i], nil
 	case "TINYINT":
-		if col.ByteVal.Nulls[i/8]&(1<<(uint(i)%8)) != 0 {
+		if isSet(col.ByteVal.Nulls, i) {
 			return nil, nil
 		}
 		return col.ByteVal.Values[i], nil
 	case "SMALLINT":
-		if col.I16Val.Nulls[i/8]&(1<<(uint(i)%8)) != 0 {
+		if isSet(col.I16Val.Nulls, i) {
 			return nil, nil
 		}
 		return col.I16Val.Values[i], nil
 	case "INT":
-		if col.I32Val.Nulls[i/8]&(1<<(uint(i)%8)) != 0 {
+		if isSet(col.I32Val.Nulls, i) {
 			return nil, nil
 		}
 		return col.I32Val.Values[i], nil
 	case "BIGINT":
-		if col.I64Val.Nulls[i/8]&(1<<(uint(i)%8)) != 0 {
+		if isSet(col.I64Val.Nulls, i) {
 			return nil, nil
 		}
 		return col.I64Val.Values[i], nil
 	case "BOOLEAN":
-		if col.BoolVal.Nulls[i/8]&(1<<(uint(i)%8)) != 0 {
+		if isSet(col.BoolVal.Nulls, i) {
 			return nil, nil
 		}
 		return col.BoolVal.Values[i], nil
 	case "FLOAT", "DOUBLE":
-		if col.DoubleVal.Nulls[i/8]&(1<<(uint(i)%8)) != 0 {
+		// we could return float values as float32(col.DoubleVal.Values[i])
+		// but it is not worth the complexity
+		if isSet(col.DoubleVal.Nulls, i) {
 			return nil, nil
 		}
 		return col.DoubleVal.Values[i], nil
 	case "TIMESTAMP", "DATETIME":
-		if col.StringVal.Nulls[i/8]&(1<<(uint(i)%8)) != 0 {
+		if isSet(col.StringVal.Nulls, i) {
 			return nil, nil
 		}
 		t, err := time.Parse(TimestampFormat, col.StringVal.Values[i])
@@ -100,7 +107,7 @@ func value(col *cli_service.TColumn, cd *ColDesc, i int) (interface{}, error) {
 		}
 		return t, nil
 	default:
-		if col.StringVal.Nulls[i/8]&(1<<(uint(i)%8)) != 0 {
+		if isSet(col.StringVal.Nulls, i) {
 			return nil, nil
 		}
 		return col.StringVal.Values[i], nil

internal/isql/connection_test.go

@@ -8,10 +8,14 @@ import (
 	"database/sql"
 	"database/sql/driver"
 	"fmt"
+	"log"
 	"net"
+	"net/http"
+	_ "net/http/pprof"
 	"net/url"
 	"os"
 	"path/filepath"
+	"reflect"
 	"slices"
 	"testing"
 	"time"
@@ -29,6 +33,12 @@ import (
 	"github.com/testcontainers/testcontainers-go/wait"
 )
 
+func init() {
+	go func() {
+		log.Println(http.ListenAndServe("localhost:6060", nil))
+	}()
+}
+
 func TestIntegration_FromEnv(t *testing.T) {
 	fi.SkipLongTest(t)
 
@@ -272,13 +282,27 @@ func testSelect(t *testing.T, db *sql.DB) {
 	require.NoError(t, err)
 	for _, tt := range tests {
 		t.Run(tt.sql, func(t *testing.T) {
-			err = conn.QueryRowContext(ctx, fmt.Sprintf("select %s", tt.sql)).Scan(&res)
+			query := fmt.Sprintf("select %s", tt.sql)
+			row := conn.QueryRowContext(ctx, query)
+			err = row.Scan(&res)
 			require.NoError(t, err)
-			require.Equal(t, tt.res, res)
+			require.NoError(t, row.Err())
+			expected := tt.res
+			require.Equal(t, expected, res)
+
+			checkScanType(ctx, t, conn, query, expected)
 		})
 	}
 }
 
+func checkScanType(ctx context.Context, t *testing.T, conn *sql.Conn, query string, expected any) {
+	dbRes := fi.NoError(conn.QueryContext(ctx, query)).Require(t)
+	defer fi.NoErrorF(dbRes.Close, t)
+	colTypes := fi.NoError(dbRes.ColumnTypes()).Require(t)
+	require.Equal(t, reflect.TypeOf(expected).String(), colTypes[0].ScanType().String())
+	require.Equal(t, reflect.TypeOf(expected), colTypes[0].ScanType())
+}
+
 func testMetadata(t *testing.T, conn *sql.DB) {
 	// We don't drop test if it exists because the tests doesn't care (for now) if the table has different columns
 	_, cerr := conn.Exec("CREATE TABLE IF NOT EXISTS test(a int)")
@@ -337,8 +361,9 @@ func Setup(ctx context.Context) (testcontainers.Container, error) {
 	})
 }
 
-func toCloser(ct testcontainers.Container) func() error {
+func toCloser(ct testcontainers.Container, t *testing.T) func() error {
 	return func() error {
+		t.Log("Terminating container", ct.GetContainerID())
 		return ct.Terminate(context.Background())
 	}
 }
@@ -388,7 +413,7 @@ func setupStack(ctx context.Context, t *testing.T) testcontainers.Container {
 		Started:          true,
 	})
 	require.NoError(t, err)
-	fi.CleanupF(t, toCloser(ct))
+	fi.CleanupF(t, toCloser(ct, t))
 
 	req = testcontainers.ContainerRequest{
 		Image: "apache/impala:4.4.1-statestored",
@@ -410,7 +435,7 @@ func setupStack(ctx context.Context, t *testing.T) testcontainers.Container {
 		Started:          true,
 	})
 	require.NoError(t, err)
-	fi.CleanupF(t, toCloser(ct))
+	fi.CleanupF(t, toCloser(ct, t))
 
 	req = testcontainers.ContainerRequest{
 		Image: "apache/impala:4.4.1-catalogd",
@@ -435,7 +460,7 @@ func setupStack(ctx context.Context, t *testing.T) testcontainers.Container {
 		Started:          true,
 	})
 	require.NoError(t, err)
-	fi.CleanupF(t, toCloser(ct))
+	fi.CleanupF(t, toCloser(ct, t))
 
 	req = testcontainers.ContainerRequest{
 		Image:      "ghcr.io/rroemhild/docker-test-openldap:master",
@@ -455,7 +480,7 @@ func setupStack(ctx context.Context, t *testing.T) testcontainers.Container {
 		Started:          true,
 	})
 	require.NoError(t, err)
-	fi.CleanupF(t, toCloser(ct))
+	fi.CleanupF(t, toCloser(ct, t))
 
 	req = testcontainers.ContainerRequest{
 		Image: "apache/impala:4.4.1-impalad_coord_exec",
@@ -500,7 +525,7 @@ func setupStack(ctx context.Context, t *testing.T) testcontainers.Container {
 		Started:          true,
 	})
 	require.NoError(t, err)
-	fi.CleanupF(t, toCloser(ct))
+	fi.CleanupF(t, toCloser(ct, t))
 
 	return ct
 }