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

mark int type explicitly as int64 #332

Merged
merged 2 commits into from
May 11, 2024
Merged

Conversation

tjungblu
Copy link
Contributor

fixes #330

Second approach to #331

Returns on armv7l (32 bit) the same result as on 64 bits:

$ octosql "SELECT 2147483647+2147483647, 9223372036854775807+9223372036854775807"
+------------+-------+
|   col_0    | col_1 |
+------------+-------+
| 4294967294 |    -2 |
+------------+-------+

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
Copy link
Contributor Author

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.

@@ -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())
Copy link
Owner

@cube2222 cube2222 Apr 25, 2024

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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 :)

Copy link
Owner

@cube2222 cube2222 left a 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!

@cube2222
Copy link
Owner

cube2222 commented May 6, 2024

Hey, sorry for the delay, had a busy last week. I’ll try to get this in and make a release this week.

@tjungblu
Copy link
Contributor Author

tjungblu commented May 6, 2024

no rush at all, thanks for your help Kuba :)

@cube2222 cube2222 merged commit 4a66fe0 into cube2222:main May 11, 2024
@tjungblu tjungblu deleted the int64_inplace branch May 22, 2024 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Int64 type
2 participants