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

fix: improve errors on field cast failures #2932

Merged
merged 1 commit into from
Oct 19, 2024

Conversation

jkylling
Copy link
Contributor

@jkylling jkylling commented Oct 8, 2024

Description

Adds information on the field, to-type and from-type when casting fails.

We could consider using our own error type for the casting errors to allow unrolling errors to get the full path to a field. Currently we only give the last part of the path.

When looking at cast_field I noticed that we might be missing a match on (DataType::List(_), DataType::LargeList(_)). Casting List to LargeList can currently cause some tricky behaviour. I had a record batch with a List type, and tried reading it with a LargeList schema. For some choices of schemas it failed with an error message, for other schemas is did not fail, but read the columns in the wrong order.

@github-actions github-actions bot added the binding/rust Issues for the Rust crate label Oct 8, 2024
Copy link

codecov bot commented Oct 8, 2024

Codecov Report

Attention: Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.

Project coverage is 72.36%. Comparing base (a846879) to head (e4bd1df).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/core/src/operations/cast/mod.rs 92.85% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2932   +/-   ##
=======================================
  Coverage   72.35%   72.36%           
=======================================
  Files         131      131           
  Lines       40586    40599   +13     
  Branches    40586    40599   +13     
=======================================
+ Hits        29368    29379   +11     
  Misses       9333     9333           
- Partials     1885     1887    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ion-elgreco
Copy link
Collaborator

@jkylling can you fix the tests please

@jkylling jkylling requested a review from fvaleye as a code owner October 11, 2024 09:56
@github-actions github-actions bot added the binding/python Issues for the Python package label Oct 11, 2024
@jkylling
Copy link
Contributor Author

@jkylling can you fix the tests please

Fixed the first expected error message in the failing test, but I suspect the next assertion will also fail. Unfortunately I'm unable to run the python tests locally. make develop within the venv fails with:

   ...
   Compiling g2p v1.0.1
   Compiling is-terminal v0.4.9
   Compiling termcolor v1.3.0
   Compiling hdfs-native v0.10.0
   Compiling tokio-native-tls v0.3.1
   Compiling env_logger v0.10.0
   Compiling hyper-tls v0.5.0
   Compiling reqwest v0.11.20
   Compiling pyo3-macros-backend v0.21.2
error[E0282]: type annotations needed for `Box<_>`
  --> /home/user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/time-0.3.34/src/format_description/parse/mod.rs:83:9
   |
83 |     let items = format_items
   |         ^^^^^
...
86 |     Ok(items.into())
   |              ---- type must be known at this point
   |
help: consider giving `items` an explicit type, where the placeholders `_` are specified
   |
83 |     let items: Box<_> = format_items
   |              ++++++++

   Compiling pyo3-ffi v0.21.2
   Compiling pyo3 v0.21.2
   Compiling arrow-cast v52.2.0
   Compiling arrow-ord v52.2.0
   Compiling arrow-string v52.2.0
   Compiling hyper-rustls v0.26.0
For more information about this error, try `rustc --explain E0282`.
error: could not compile `time` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...
💥 maturin failed
  Caused by: Failed to build a native library through cargo
  Caused by: Cargo build finished with "exit status: 101": `env -u CARGO PYO3_ENVIRONMENT_SIGNATURE="cpython-3.11-64bit" PYO3_PYTHON="/home/user/delta-rs/python/venv/bin/python" PYTHON_SYS_EXECUTABLE="/home/user/delta-rs/python/venv/bin/python" "cargo" "rustc" "--message-format" "json-render-diagnostics" "--manifest-path" "/home/user/delta-rs/python/Cargo.toml" "--lib"`

@ion-elgreco ion-elgreco force-pushed the improve-cast-error-message branch from 8c3c63e to d75e063 Compare October 13, 2024 08:16
@rtyler rtyler marked this pull request as draft October 16, 2024 10:14
@rtyler rtyler force-pushed the improve-cast-error-message branch from d75e063 to aa111b9 Compare October 19, 2024 14:05
@rtyler rtyler marked this pull request as ready for review October 19, 2024 14:05
@rtyler rtyler enabled auto-merge October 19, 2024 14:05
@rtyler rtyler force-pushed the improve-cast-error-message branch from aa111b9 to e4bd1df Compare October 19, 2024 14:09
@rtyler rtyler added this pull request to the merge queue Oct 19, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Oct 19, 2024
@rtyler rtyler merged commit 10c6b5c into delta-io:main Oct 19, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
binding/python Issues for the Python package binding/rust Issues for the Rust crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants