dataframe: add row appending methods and sql scan helpers. #50

kylebrandt · 2020-01-22T15:53:44Z

wip for #49

…of reflect.Type

kylebrandt · 2020-01-29T20:37:02Z

Some notes:

Should the dataframe sql stuff be in a different package? Must not be dataframe/sql though because would be annoying on imports to always have to alias (since it would be used with database/sql). They also would no longer be methods on dataframe. Although something like a sqlframe could just be a type of frame.
Try two phase converter that replaces a string field with the desired output type of the conversion func with sql. In particular for use with alternate numeric types.
Need to check for valid types before attempting to create dataframe. Add some sort of validator method. Needed at newForSQLRows
Not sure if row ops should be a method on Fields instead of dataframe.

…r provided

kylebrandt · 2020-02-13T17:24:53Z

I would more time with but think it would be helpful to @sunker to get this merged soon then later if even means we change / fix some things in master.

papagian

Great work; I enjoyed reviewing it a lot!
I have left some comments/thoughts but with my limited knowledge on the subject I think it's good to get merged.

papagian · 2020-02-14T08:41:33Z

dataframe/dataframe.go

+		}
+	}
+	// second loop that modifies
+	f.AppendRow(vals...)


Instead of calling AppendRow and looping over vals twice you could do the appending inside the above loop using:
f.Fields[i].Vector.Append(v)

dataframe/dataframe_test.go

papagian · 2020-02-14T09:28:53Z

dataframe/generic_nullable_vector.go

 func (v *nullablegenVector) Len() int {
 	return len((*v))
 }

 func (v *nullablegenVector) PrimitiveType() VectorPType {
 	return vectorPType(v)
 }
+
+func (v *nullablegenVector) Extend(i int) {


I don't know the use case but maybe it would be useful to accept also an arbitrary number of values to initialise the new slice.

If I understand you correctly it already does. Extend grows the vector be argument i.

papagian · 2020-02-14T09:35:32Z

dataframe/sql.go

+)
+
+// NewFromSQLRows returns a new dataframe populated with the data from rows. The Vector types
+// will be []*T if nullable or will be []*T if it is unknown if the column is nullable.


this is a bit confusing

typod - will fix

papagian · 2020-02-14T09:39:46Z

dataframe/sql.go

+	seen := map[string]struct{}{}
+	for _, name := range colNames {
+		if _, ok := seen[name]; ok {
+			return nil, nil, fmt.Errorf(`duplicate column names no allowed, found identical name: "%v"`, name)


The index of the identical column could be included in the error

Alternatively, instead of having this restriction and returning an error I wonder if the new field name could be named after the column name and the column index?

I think this needs to be solved at a deeper level. I think perhaps something in MarshalArrow takes care of it. I meant to make an issue and then link it in the comment and forgot, will do that.

#57 (added to code comments as well).

papagian · 2020-02-14T10:04:29Z

dataframe/sql.go

+			// Nullabe types get passed to scan as a pointer to a pointer
+			vec = reflect.MakeSlice(reflect.SliceOf(ptrType), 0, 0).Interface()
+		}
+		if !ValidVectorType(vec) {


I wonder it this condition is enough; is it necessary for all the columns have a mapper? if yes then this condition should check also if a mapping exists? otherwise maybe here there should be an additional check for this?
As it is now I think it's possible to end up with a nil mapper.

There would only be a nil mapper if you passed a nil mapper to NewFromSQLRows. If a mapper isn't provided, and it isn't a type natively supported by both dataframe and SQL scanner, it becomes a *string (via auto-generated mapper) which anything can be scanned into.

papagian · 2020-02-14T10:09:54Z

dataframe/sql.go

+
+// scannableRow adds a row to the dataframe, and returns a slice of references
+// that can be passed to rows.Scan() in the in sql package.
+func (f *Frame) scannableRow() []interface{} {


I would prefer the name newScannableRow that highlights the fact the a new row is created.

papagian · 2020-02-14T10:35:43Z

dataframe/sql.go

+		}
+	}
+
+	for fieldIdx, mapper := range mappers {


I wonder if it would be feasible to apply conversions and replacements inside the above loop.
I see there is an on-going discussion here.

I had seen that discussion while researching this - but only at proposal stage :-(

papagian

LGTM

papagian · 2020-02-14T15:03:43Z

dataframe/dataframe.go

+// The dataframe's Fields and the Fields' Vectors must be initalized or AppendRow will panic.
+// The number of arguments must match the number of Fields in the Frame and each type must coorespond
+// to the Field type or AppendRow will panic.
+func (f *Frame) AppendRow(vals ...interface{}) {


Do we want to keep this?
Why someone would prefer this over the AppendRowSafe?

Speed. It should be faster to use if you already know that both your data and your frame will meet what is checked for, bypassing all the checks.

kylebrandt added 6 commits January 22, 2020 10:51

dataframe: (wip) explore appending scenarios

a0c764c

handle untyped/typed nil in safe append and vector append

5740bb1

sql scanning wip

9f2c5da

string converter wip

0a1bd5f

NewFromSQLRows

637a86d

assume nullable if driver does not tell us, use reflect.Kind instead …

14c4391

…of reflect.Type

kylebrandt added 6 commits February 6, 2020 10:37

changes that have been sitting on my computer for a week

4cf5dd9

Merge branch 'master' into frame_append

c0f1e43

move sql code

060be52

automatically add stringmapper if unsupported column type an no mappe…

664ce78

…r provided

populate more of autocreated mapping

a3f4766

Replacer to convert from string to other vector types

c003875

kylebrandt marked this pull request as ready for review February 13, 2020 17:24

kylebrandt requested a review from papagian February 13, 2020 17:24

kylebrandt changed the title ~~dataframe: (wip) explore appending scenarios~~ dataframe: add row appending methods and sql scan helpers. Feb 13, 2020

kylebrandt added 2 commits February 13, 2020 14:41

cleanup and some examples

df0e79f

typo

56deb4a

papagian approved these changes Feb 14, 2020

View reviewed changes

kylebrandt added 5 commits February 14, 2020 08:33

review-fix: AppendRow

efee37a

review-fix: additional AppendRowSafe test cases

b269184

review-fix: fixes to comments

73deda3

review-fix: s/scannableRow/newScannableRow/

ccf6ff1

review-fix: indices of dup col names in error

c9bde0b

kylebrandt requested a review from papagian February 14, 2020 14:48

papagian approved these changes Feb 14, 2020

View reviewed changes

kylebrandt merged commit 4127415 into master Feb 14, 2020

kylebrandt deleted the frame_append branch February 14, 2020 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dataframe: add row appending methods and sql scan helpers. #50

dataframe: add row appending methods and sql scan helpers. #50

kylebrandt commented Jan 22, 2020

kylebrandt commented Jan 29, 2020 •

edited

Loading

kylebrandt commented Feb 13, 2020

papagian left a comment

papagian Feb 14, 2020

papagian Feb 14, 2020

kylebrandt Feb 14, 2020

papagian Feb 14, 2020

kylebrandt Feb 14, 2020

papagian Feb 14, 2020

kylebrandt Feb 14, 2020

kylebrandt Feb 14, 2020

papagian Feb 14, 2020

kylebrandt Feb 14, 2020

papagian Feb 14, 2020

papagian Feb 14, 2020 •

edited

Loading

kylebrandt Feb 14, 2020

papagian left a comment

papagian Feb 14, 2020

kylebrandt Feb 14, 2020

dataframe: add row appending methods and sql scan helpers. #50

dataframe: add row appending methods and sql scan helpers. #50

Conversation

kylebrandt commented Jan 22, 2020

kylebrandt commented Jan 29, 2020 • edited Loading

kylebrandt commented Feb 13, 2020

papagian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papagian Feb 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papagian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylebrandt commented Jan 29, 2020 •

edited

Loading

papagian Feb 14, 2020 •

edited

Loading