JuliaDatabases / ODBC.jl

An ODBC interface for the Julia programming language
https://odbc.juliadatabases.org/stable
Other
106 stars 63 forks source link

converting null-terminated numeric strings #341

Closed hhaensel closed 2 years ago

hhaensel commented 2 years ago

Currently, converting a query result to a dataframe of results in an error when the numeric data contains a null-terminated string. The origin is that the parsing routine of DecFP, which is called by jlcast errors out.

The proposed PR makes the conversion more robust

hhaensel commented 2 years ago

This PR refers to #340

codecov[bot] commented 2 years ago

Codecov Report

Merging #341 (b06446c) into main (f38f771) will increase coverage by 0.68%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #341      +/-   ##
==========================================
+ Coverage   74.97%   75.65%   +0.68%     
==========================================
  Files           6        6              
  Lines         855      834      -21     
==========================================
- Hits          641      631      -10     
+ Misses        214      203      -11     
Impacted Files Coverage Δ
src/utils.jl 79.45% <100.00%> (-0.16%) :arrow_down:
src/load.jl 86.88% <0.00%> (-0.42%) :arrow_down:
src/dbinterface.jl 94.08% <0.00%> (+1.30%) :arrow_up:
src/API.jl 58.94% <0.00%> (+1.46%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

quinnj commented 2 years ago

Hmmmm, there are some test failures here that may or may not be related if you don't mind taking a look. I can try to take a look later as well if you need.

hhaensel commented 2 years ago

I tried to set up an environment where I could run the tests, but I failed. I'm typically pinned to Windows, but I also tried to run the tests on an AWS EC2 instance. It seems that installing the ODBC driver fails. Finally I installed MariaDB on my private windows machine and could run the tests with slight adaptations line by line.

Finding:

The error occurs on 1.7.2 also without my modification.

When I replace the Chinese (?) characters by latin characters, the test passes.

Conclusion:

The error seems to be linked to the handling of the character set, but not to the introduction of the rstrip().

@quinnj Do you have an idea where this error might originate exactly?

hhaensel commented 2 years ago

I think I found a fix, will file a PR ...

hhaensel commented 2 years ago

At least on my windows machine the test in line 174 passes. This would mean that on the different julia versions there are different default character sets for mariadb. Let's hope that this is fixed now.

quinnj commented 2 years ago

Looks like they're all failing consistently now? 😅

hhaensel commented 2 years ago

That's a plus isn't it? 😉 Well, some more work to be done, but I'm quite sure it's not linked to my patch because that's only called when parsing strings to numbers. I'm quite sure it has to do with the character set. Maybe the character set I chose is only available on windows.

hhaensel commented 2 years ago

@quinnj I think, I need some help here. I could get the tests running on both a windows machine and an amazon linux ec2 instance. The tests pass with and without my modification. However, during manual testing I experienced that ODBC.load() sometimes returns different results on first and second execution. So if I replaced

ODBC.load(Base.structdiff(expected, NamedTuple{(:LastLogin2, :Wage,)}), conn, "Employee_copy"; limit=4, columnsuffix=Dict(:Name=>"CHARACTER SET utf8mb4"))

by

ODBC.load(Base.structdiff(expected, NamedTuple{(:LastLogin2, :Wage,)}), conn, "Employee_copy"; limit=4, columnsuffix=Dict(:Name=>"CHARACTER SET utf8mb4"))
DBInterface.execute(conn, "DROP TABLE Employee_copy")
ODBC.load(Base.structdiff(expected, NamedTuple{(:LastLogin2, :Wage,)}), conn, "Employee_copy"; limit=4, columnsuffix=Dict(:Name=>"CHARACTER SET utf8mb4"))

I could get rid of the errors.

hhaensel commented 2 years ago

So, I think the characterset was not the culprit...

quinnj commented 2 years ago

@hhaensel, sorry for the slow response here. I cherry-picked your commit and merged it here: https://github.com/JuliaDatabases/ODBC.jl/pull/348. There was just some CI weirdness that I fixed by bumping some versions.

hhaensel commented 2 years ago

Cool, thanks!