-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Avoid checking the returned sqlite dtype on all 32 bit platforms. #9427
base: main
Are you sure you want to change the base?
Conversation
There's still some 32-bit linux targets in addition to win32.
Can one of the admins verify this patch? Admins can comment |
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.
Thanks for opening this PR @detrout! I think there is maybe just one typo.
@@ -440,7 +440,7 @@ def test_query_with_meta(db): | |||
).select_from(sql.table("test")) | |||
out = read_sql_query(s1, db, npartitions=2, index_col="number", meta=meta) | |||
# Don't check dtype for windows https://github.com/dask/dask/issues/8620 | |||
assert_eq(out, df[["name", "age"]], check_dtype=sys.platform != "win32") | |||
assert_eq(out, df[["name", "age"]], check_dtype=sys.int_info.sizeof_digit != 2) |
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.
Should this be:
assert_eq(out, df[["name", "age"]], check_dtype=sys.int_info.sizeof_digit != 2) | |
assert_eq(out, df[["name", "age"]], check_dtype=sys.int_info.sizeof_digit != 32) |
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.
It definitely should be 2. (I think it's size in bytes.)
On a 32 bit linux system with python 3.9.2 I get:
python3 -c "import sys; print(sys.int_info)"
sys.int_info(bits_per_digit=15, sizeof_digit=2)
on the 32 bit windows version of python 3.10.6 running under the 32 bit wine (5.0.3) I get:
wine python.exe -c "import sys; print(sys.int_info)"
sys.int_info(bits_per_digit=15, sizeof_digit=2)
And on a 64 bit linux system with python 3.10.6 I get
python3 -c "import sys; print(sys.int_info)"
sys.int_info(bits_per_digit=30, sizeof_digit=4)
ok to test |
There's still some 32-bit linux targets in addition to win32.
read_sql_query
with meta converts dtypes from 32 to 64. #8620 It's really more of a work around, the problem of sqlite's data types not being consistent between 64 and 32 bit architectures is not fixed. This patch ignores that difference because it's pretty likely to not be that important.(test_query_with_meta was made slightly weaker on 32-bit architectures by not requiring the same dtype)
pre-commit run --all-files
I made the patch pushed it to my own branch on github and waited for the tests to run. There's (now) one test failure that looks unrelated to this change. I think that's equivalent to the pre-commit run --all-files
(On my first try I typoed the name of int_info and there were two test failures, so I'm pretty sure the test is actually running.)