mark int type explicitly as int64#332
Conversation
fixes cube2222#330 Signed-off-by: Thomas Jungblut <[email protected]>
| switch output { | ||
| case "live_table", "batch_table": | ||
| var limit *int | ||
| var limit *int64 |
There was a problem hiding this comment.
let me know if that's OK for you, I think we could safely use the 32 bit here as before. Most of the PR here is to adjust for that type change.
| *dst = octosql.NewBoolean(src.Boolean()) | ||
| case parquet.Int32: | ||
| *dst = octosql.NewInt(int(src.Int32())) | ||
| *dst = octosql.NewInt(src.Int64()) |
There was a problem hiding this comment.
I believe this one might break (that is, if it's Int32 and you ask for Int64, the parquet library won't figure out that it can just upcast). Let's please leave it like it was, so int64(src.Int32()) now.
There was a problem hiding this comment.
the code that I can see in parquet looks like that:
u64 uint64
...
// Int32 returns v as a int32, assuming the underlying type is INT32.
func (v Value) Int32() int32 { return int32(v.u64) }
// Int64 returns v as a int64, assuming the underlying type is INT64.
func (v Value) Int64() int64 { return int64(v.u64) }
so we would simply cast the 64 bit integer to 32 bit and back
There was a problem hiding this comment.
I've added your suggestion as a separate commit, in case you want to reconsider it, it's just a simple revert away :)
cube2222
left a comment
cube2222
left a comment
There was a problem hiding this comment.
Just one minor issue, looks good to me in general!
|
Hey, sorry for the delay, had a busy last week. I’ll try to get this in and make a release this week. |
|
no rush at all, thanks for your help Kuba :) |
2 participants
fixes #330
Second approach to #331
Returns on armv7l (32 bit) the same result as on 64 bits: