Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Panic in library when reading an array of tuple: Array(Tuple(price Nullable(Float64), qty Nullable(Int64), total Nullable(Float64))) #816

Closed
stephane-moreau opened this issue Nov 15, 2022 · 2 comments
Labels

Comments

@stephane-moreau
Copy link
Contributor

Issue description

In a table storing events from a navigation, I have a Nullable(Stirng) column, that I parse using the following SQL

JSONExtract(
        Coalesce(T0."data", '{}'),
        'Tuple(count Nullable(Int64), products Array(Tuple(price Nullable(Float64), qty Nullable(Int64))), price Nullable(Float64))'
    )

When using library version 2.0.14, the property "product" (array of map) was generating a value which was a []interface[} (for the array) and each tuple was in fact decoded as []interface{} where each item in the array represented one column of the tuple

While upgrading to version 2.3.0, the same property is exposed as a []interface{} (for the array part) and each tuple is in value decoded as a map[string]interface{}
This is a good progress for decoding such structure but in my case the values stored in the JSON are nullable and when creating the map[string]interace{}, the code in tuple.go line 290

		default:
			field := reflect.New(reflect.TypeOf(c.Row(0, false))).Elem()
			value := reflect.ValueOf(c.Row(row, false))
			if err := setJSONFieldValue(field, value); err != nil {
				return err
			}
			targetMap.SetMapIndex(reflect.ValueOf(colName), field)

do generate a panic because c.Row(0, false) is nil

Configuration

Seen on both MacOS and Windows 11 using

Interface: database/sql

Driver version:
v2.3.0
Go version: go version
go1.19.2 windows/amd64

ClickHouse Server version:
21.11

@stephane-moreau
Copy link
Contributor Author

Using a git clone of the main source code the issue still occurs even though some nullable handling code has been added

New code is the following

		default:
			val := c.Row(row, false)
			if val != nil {
				field := reflect.New(reflect.TypeOf(c.Row(0, false))).Elem()
				value := reflect.ValueOf(c.Row(row, false))
				if err := setJSONFieldValue(field, value); err != nil {
					return err
				}
				targetMap.SetMapIndex(reflect.ValueOf(colName), field)
			} else {
				targetMap.SetMapIndex(reflect.ValueOf(colName), reflect.Zero(c.ScanType().Elem()))
			}

the problem is that if the data of the first item of the array is nill the code still panics on line 293, if value of the "row"th item is not nill

I would suggest to use the following code which fixes the issue on my side

			val := c.Row(row, false)
			if val != nil {
				field := reflect.New(reflect.TypeOf(val)).Elem()
				value := reflect.ValueOf(val)
				if err := setJSONFieldValue(field, value); err != nil {
					return err
				}
				targetMap.SetMapIndex(reflect.ValueOf(colName), field)
			} else {
				targetMap.SetMapIndex(reflect.ValueOf(colName), reflect.Zero(c.ScanType().Elem()))
			}

I'll create a PR soon so that you could integrate proposed bugfix if you wish

@gingerwizard
Copy link
Collaborator

Have a test for this @stephane-moreau thankyou. I'll verify and merge your PR shortly. Thanks for taking the time to diagnose and more importantly fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants