-
Notifications
You must be signed in to change notification settings - Fork 207
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
mark int type explicitly as int64 #332
Conversation
fixes cube2222#330 Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
@@ -385,7 +385,7 @@ octosql "SELECT * FROM plugins.plugins"`, | |||
|
|||
switch output { | |||
case "live_table", "batch_table": | |||
var limit *int | |||
var limit *int64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
datasources/parquet/reconstruct.go
Outdated
@@ -293,9 +293,9 @@ func assignValue(dst *octosql.Value, src parquet.Value) error { | |||
case parquet.Boolean: | |||
*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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added your suggestion as a separate commit, in case you want to reconsider it, it's just a simple revert away :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :) |
fixes #330
Second approach to #331
Returns on armv7l (32 bit) the same result as on 64 bits: