Handle TimeStamp columns by converting int64 to time.Time and vice versa #15
Conversation
…erly handle type conversions, handle at least time.Time columns properly by using the column type provided by Crate within the query’s results. driver.DefaultParameterConverter override is scheduled for GO 1.9 (see golang/go#18415)
|
Hello, @echarlus! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information. |
![sourcelevel-bot[bot]](https://avatars.githubusercontent.com/in/597?s=60&v=4){kind=small}
crate.go
Outdated
| sec := v/int64(1000) | ||
| dest[i] = time.Unix(sec, (v-sec*int64(1000))*int64(1000000)) | ||
| } else { | ||
| return errors.New(fmt.Sprintf("Failed to convert column %s=%T to time\n", r.columns[i], r.values[r.pos][i])) |
There was a problem hiding this comment.
should replace errors.New(fmt.Sprintf(...)) with fmt.Errorf(...)
|
If we changed the output/parsing of time columns from int64 to time.Time, it would cause breaking changes. Although it would be nice to parse time.Time to int64 when receiving arguments. Let me give this some thought. |
|
Hello, |
|
Wouldn't this change the value we are assigning to each row column? https://golang.org/pkg/database/sql/#Rows.Scan So if we did: // create table users ( created_at timestmap );
rows, err := db.Query("select created_at from users limit 1")
for rows.Next() {
var created_at time.Time
// Now scan, expects a time.Time destination value
rows.Scan(&created_at)
}From what I understood, this PR changes what we are scanning into the destination we pass into Maybe I'm confused, and got this all wrong, I haven't worked on this for a while. I'll give this code a test. |
|
Since the conversion from int64 to time.Time is performed in the query method (which is called by Query) the row type would indeed by time.Time and your sample should work as expected. So I believe that my change does not break the Scan function. Let me know what your findings show ... |
…only, no more tweaks in gorm
crate.go
Outdated
| sec := v / int64(1000) | ||
| dest[i] = time.Unix(sec, (v-sec*int64(1000))*int64(1000000)) | ||
| } else { | ||
| return fmt.Errorf("Failed to convert column %s=%T to time\n", r.columns[i], r.values[r.pos][i]) |
There was a problem hiding this comment.
error strings should not be capitalized or end with punctuation or a newline
crate.go
Outdated
| @@ -53,6 +57,39 @@ type endpointQuery struct { | |||
| Args []driver.Value `json:"args,omitempty"` | |||
| } | |||
|
|
|||
| //CER : Convert map, Time & GeoPoint arguments to DB format. | |||
There was a problem hiding this comment.
comment on exported method CrateDriver.CheckNamedValue should be of the form "CheckNamedValue ..."
crate.go
Outdated
| ) | ||
|
|
||
| // Crate conn structure | ||
| type CrateDriver struct { | ||
| Url string // Crate http endpoint url | ||
| } | ||
|
|
||
| type GeoPoint [2]float64 |
There was a problem hiding this comment.
exported type GeoPoint should have comment or be unexported
Fixed code according to the GitHub boat comments Added Scan method on GeoPoint to allow gorm to handle GeoPoint embedded in an object
| } | ||
|
|
||
| //CrateArray represents an Array column type | ||
| type CrateArray []interface{} |
There was a problem hiding this comment.
type name will be used as crate.CrateArray by other packages, and that stutters; consider calling this Array
… handle floats with null decimals to prevent creating an int typed column in crate when a float is required
crate.go
Outdated
| fv := v.Float() | ||
| i := float64(int32(fv)) | ||
| if i == fv { | ||
| buf.WriteString(fmt.Sprintf("%0.1f", v)) |
There was a problem hiding this comment.
arg v for printf verb %f of wrong type: reflect.Value
… sent as JSON. Now use Marshaller interface to properly handle our own types
…-crate into time_column_handling merge from head.
crate.go
Outdated
| } | ||
|
|
||
| //MarshalJSON custom JSON marshal function to properly handle arrays of floats with decimal part equals to 0 | ||
| func (v CrateArray) MarshalJSON() ([]byte, error) { |
There was a problem hiding this comment.
receiver name v should be consistent with previous receiver name arr for CrateArray
…1000000000000003 ... //See https://floating-point-gui.de/ //Float values are now truncated to 10^-6 which is more than enough for our needs
…n maps): Prevents rounding errors seen with floats like 0.01*41 which is 0.41000000000000003 ... See https://floating-point-gui.de/ Float values are now truncated to 10^-6 which is more than enough for our needs
Speed up float processing by removing un-necessary conversion & comparison
| } | ||
|
|
||
| //MarshalJSON custom JSON marshal function to properly handle arrays of floats with decimal part equals to 0 | ||
| func (v CrateArray) MarshalJSON() ([]byte, error) { |
There was a problem hiding this comment.
receiver name v should be consistent with previous receiver name arr for CrateArray
|
Ebert has finished reviewing this Pull Request and has found:
But beware that this branch is 23 commits behind the You can see more details about this review at https://ebertapp.io/github/herenow/go-crate/pulls/15. |
|
SourceLevel has finished reviewing this Pull Request and has found:
But beware that this branch is 27 commits behind the |
Since one cannot yet replace the driver.DefaultParameterConverter to properly handle type conversions (see golang/go#18415)
I've made a quick patch to automatically handle TimeStamp (int64) columns conversion to time.Time by using the column type provided by Crate within the query’s results and to convert back to int64 any time.