-
Notifications
You must be signed in to change notification settings - Fork 86
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
Reading parquet in Python has some rough edges, making it hard to diagnose problems #2139
Comments
Hi @prrao87, thanks for the bug report! I'll give some of my thoughts first. Data loading bugFirst I need admit in both scenarios the behaviour is unacceptable. A system should handle malformed input and error properly. For this specific case, the right behaviour is to strictly follow DDL and always try to cast the input data based on DDL. If a casting cannot succeed, e.g. casting a STRING to INT, a runtime error will be thrown. @acquamarin will send out a fix soon. Developer experienceYes, this is something we didn't do a good job. I think the deeper problem is that we don't have good error messages/reporting mechanism on the cpp side, so the python API doesn't have proper error message to begin with. We received multiple feedbacks related to this, and I also believe this is something we should address with high priority now. I'll make the following 2 proposals to the team this week.
Once we make a decision we will open an issue to describe our roadmap and will keep u updated on the solution. |
This is one of the most important things we should focus on. The main problem is the first bullet point: "Our current testing framework primarily focus on the correctness of result and does not test too many malformed queries.". We need to prioritize test cases that should throw errors, wrong inputs, malformed queries etc. to catch these and as our test cases catch these, we will write the necessary code to improve our error messages. The first thing to do is for code reviewers to think seriously about malformed queries that should error and ask the PR submitters to add those tests. |
Thank you! What you both said makes a lot of sense 👍🏽 |
@andyfengHKU and @acquamarin I think the new parquet reader test suite definitely needs to be looked at. Version Looking at things from a larger perspective, most of my parquet-driven workflows involve polars in some shape or form, but I believe that the tests on your end assume that the parquet comes from pyarrow? I wonder if it makes sense to increase the diversity of parquet files being tested, taking parquets from numerous sources like polars, pyarrow, duckdb, etc. so that the upcoming versions don't show any regressions. Considering that the parquet reader you guys have written is built from scratch, it's expected to have a lot of rough edges, so I hope that this request will be considered for the upcoming release! |
I assume this issue is still not resolved, right? |
We now throw exception if column type doesn't match parquet type. Though we should take one step further to perform casting for numerical cases. |
This should be solved with #4041 |
I've upgraded to the latest version
0.0.9
and there are a few rough edges with reading parquet from Python.Problem scenario
If I have a parquet file with two columns,
id
andname
, of typeINT64
andSTRING
respectively.During development, sometimes we may specify the wrong data type during table creation, for example in this case:
In this case, the data type of the id should be
INT64
, but was specified asSTRING
, it just segfaults, making it very hard for the Python developer to know what they did wrong (it's not a Kùzu bug, and could have very easily been fixed if the user just had a better error message).In the reverse situation, the error is even more complex. Say I have a parquet file with the same two columns,
id
andname
, but this time, theid
column is also of typeSTRING
like thename
column is.This time, if the user specified the
id
column in the node table creation as of the typeINT64
(by mistake, when they should have specifiedSTRING
), it doesn't segfault but instead gives this weird error.What should be a 2-second fix to the data type now becomes a tedious exercise for the Python developer to go back and inspect the data and learn (after some deliberation) that the data type specified in Python was wrong.
Developer experience focus
These sorts of issues are more generic, and I suppose adding these edge cases to the test suite could help avoid such issues, but it's impossible for the Kùzu team to know all the kinds of ways users may break the system.
In this case, malformed data types (or the user simply making a mistake) is a common enough scenario that the test suite can be expanded, but the longer term problem of C++ error propagation to Python is still something to ponder about. What should be a very simple fix, can take minutes of unnecessary developer effort to build even the simplest graphs. This is quite a big source of user frustration in Python, and it's pretty natural to expect a large portion of future Kùzu users to come from Python, so this is a serious enough issue to discuss -- I think it really makes sense to deep-dive into how to present better error messages to Python users, cc @andyfengHKU @semihsalihoglu-uw
The text was updated successfully, but these errors were encountered: