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: Remove more cast.rs logic from parquet_support.rs for experimental native scans #1413

Merged
merged 2 commits into from
Feb 18, 2025

Conversation

mbutrovich
Copy link
Contributor

@mbutrovich mbutrovich commented Feb 17, 2025

Which issue does this PR close?

Closes #.

Rationale for this change

See #1387 for more discussion on this topic.

What changes are included in this PR?

The rules for whether we can convert between types when building our SchemaAdapter come from cast.rs, which may or may not be accurate. For example, I removed this check and native_datafusion went from 27 failures to 22 failures.

How are these changes tested?

Local tests on the experimental scans.

native_datafusion: Tests: succeeded 657, failed 22, canceled 2, ignored 54, pending 0
native_iceberg_compat: Tests: succeeded 649, failed 30, canceled 2, ignored 54, pending 0

Copy link
Contributor

@kazuyukitanimura kazuyukitanimura left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pending ci

@codecov-commenter
Copy link

codecov-commenter commented Feb 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 39.32%. Comparing base (f09f8af) to head (f855514).
Report is 38 commits behind head on main.

Additional details and impacted files
@@              Coverage Diff              @@
##               main    #1413       +/-   ##
=============================================
- Coverage     56.12%   39.32%   -16.81%     
- Complexity      976     2081     +1105     
=============================================
  Files           119      265      +146     
  Lines         11743    61132    +49389     
  Branches       2251    12962    +10711     
=============================================
+ Hits           6591    24039    +17448     
- Misses         4012    32587    +28575     
- Partials       1140     4506     +3366     

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

@parthchandra
Copy link
Contributor

@mbutrovich I opened #1415 which modifies cast_supported to address a couple of failures. Can you verify if I can remove the change I made?

@andygrove andygrove merged commit d61bcfc into apache:main Feb 18, 2025
74 checks passed
@mbutrovich mbutrovich deleted the schema_adapter_cast branch February 19, 2025 04:16
kazuyukitanimura pushed a commit that referenced this pull request Feb 20, 2025
…ceberg_compat readers (#1415)

major changes :
- allow Uint64 to decimal and  FixedWidthBinary to Binary conversions in complex readers
- do not enable prefetch reads in tests if complex reader is enabled
- fix more incompatible checks in uint_8/uint_16 tests
- skip datetime rebase tests for complex readers (not supported)

There may be conflicts between this and #1413  (@mbutrovich) which removes the `cast_supported` method but can be reconciled afterwards

Without #1413 the failure counts are: 
```
native_datafusion: Tests: succeeded 658, failed 19, canceled 4, ignored 54, pending 0
native_iceberg_compat: Tests: succeeded 662, failed 15, canceled 4, ignored 54, pending 0
```
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.

5 participants