chore: update datafusion to 42.0 and arrow to 53.2 by jleibs · Pull Request #3176 · lance-format/lance

jleibs · 2024-11-25T23:51:12Z

Changes:

The way in which zero-copy Bytes -> Arrow::Buffer conversion are done was necessitated by arrow-53 dropping support for impl<T: AsRef<[u8]>> From<T> for Buffer (see: explanation)
Introduces new BufferExt to centralize these conversions and make behavior more explicit.

github-actions · 2024-11-25T23:51:32Z

ACTION NEEDED
Lance follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

For details on the error please inspect the "PR Title Check" action.

jleibs · 2024-11-26T14:08:09Z


 [dev-dependencies]
-substrait-expr = { version = "0.2.1" }
+# TODO: This is too old


I believe somebody needs to also update substrait-expr to use prost 0.13 to match the version used by datafusion-substrait

Guess that's on me. How do you feel about bumping all the way to datafusion 43? That release should include a change I made which will let us drop substrait-expr entirely.

Version 0.2.2 of substrait-expr is now available and will include prost 0.13

How do you feel about bumping all the way to datafusion 43

There were a couple more API surfaces that needed modifying so I was focusing on min-change possible, but I don't have a philosophiscal objection.

Ok. Let's keep it minimal for now. I'll need to do some more work to get rid of substrait-expr anyways.

westonpace · 2024-11-26T14:55:37Z


 [dev-dependencies]
-substrait-expr = { version = "0.2.1" }
+# TODO: This is too old


Guess that's on me. How do you feel about bumping all the way to datafusion 43? That release should include a change I made which will let us drop substrait-expr entirely.

westonpace · 2024-11-26T14:56:45Z

        }

-        let buf = bytes.into();
+        let buf = Buffer::from_vec(bytes.to_vec());


This triggers a data copy (and could be the source of your alignment issues). What changed in arrow-rs to necessitate this change? It should still be possible to go from Bytes to Buffer with zero-copy somehow.

Looks like this was the change:

https://github.com/apache/arrow-rs/blob/e1e881405ce9b180858d5ba278803b4795b63e74/arrow-buffer/src/buffer/immutable.rs#L354-L358

Will try that approach instead.

Based on the comment above I believe:
Buffer::from_vec(bytes.into()) is the intended pathway, though I also tried bytes.to_byte_slice().into(), but both still result in the same error:

---- index::vector::ivf::tests::test_create_ivf_hnsw_with_empty_partition stdout ---- thread 'index::vector::ivf::tests::test_create_ivf_hnsw_with_empty_partition' panicked at /home/jleibs/.cargo/registry/src/index.crates.io-6f17d22bba15001f/arrow-buffer-53.3.0/src/buffer/scalar.rs:133:42: Memory pointer is not aligned with the specified scalar type

That's still going to trigger a copy it seems. I'll take a look real quick.

That's still going to trigger a copy it seems.

Is it?

My reads of both:

impl From<Bytes> for Vec<u8>

Buffer::from_vec()

Look like they should both be zero-copy.

Sorry, I saw bytes.to_byte_slice().into() which I think is not zero-copy. You are right that Buffer::from_vec(bytes.into()) should be zero-copy.

Actually, no Buffer::from_vec(bytes.into()) will not be zero copy because bytes.into() is not zero-copy

Ah, nevermind, it looks like bytes does have a zero-copy conversion to Vec<u8>. My bad.

I'm guessing your fix was probably ok here. The root cause was probably just the deep_copy_buffer change.

Aha. Those new routines are much more clear. Thanks.

jleibs · 2024-11-26T18:34:56Z

With the changes to Buffer it looks like more tests are passing but now we get a failure in substrait:

thread 'substrait::tests::test_substrait_conversion' panicked at rust/lance-datafusion/src/substrait.rs:430:14:
called `Result::unwrap()` on an `Err` value: Schema { message: "Schema error: No field named c0. Valid fields are x.", location: Location { file: "rust/lance-datafusion/src/substrait.rs", line: 346, column: 9 } }

jleibs · 2024-11-26T19:20:57Z

It looks like the substrait error can be fixed by:

diff --git a/rust/lance-datafusion/src/substrait.rs b/rust/lance-datafusion/src/substrait.rs
index 57cffb12..c7b05e23 100644
--- a/rust/lance-datafusion/src/substrait.rs
+++ b/rust/lance-datafusion/src/substrait.rs
@@ -423,7 +423,7 @@ mod tests {
         let expr = expr_builder.build();
         let expr_bytes = expr.encode_to_vec();
 
-        let schema = Arc::new(Schema::new(vec![Field::new("x", DataType::Int32, true)]));
+        let schema = Arc::new(Schema::new(vec![Field::new("c0", DataType::Int32, true)]));
 
         let df_expr = parse_substrait(expr_bytes.as_slice(), schema)
             .await
@@ -432,7 +432,7 @@ mod tests {
         let expected = Expr::BinaryExpr(BinaryExpr {
             left: Box::new(Expr::Column(Column {
                 relation: None,
-                name: "x".to_string(),
+                name: "c0".to_string(),
             })),
             op: Operator::Lt,
             right: Box::new(Expr::Literal(ScalarValue::Int32(Some(0)))),

but I'm not sure if this hints at a bug or just a behavior change

jleibs · 2024-11-26T20:29:05Z

And the chain just keeps on going... looks like tfrecord depends on prost v12 which makes it incompatible with prost v13 as necessitated by datafusion-substrait...

jerry73204/rust-tfrecord#8

westonpace · 2024-11-26T22:13:28Z

@jleibs this will work around the tfrecord while we wait for them to upgrade. It's a bit ugly but 🤷

westonpace@ddcf53c

…into jleibs/update_arrow_datafusion

jleibs · 2024-11-26T22:58:21Z

@westonpace thanks! That's a helpful trick.

Looks like the wheel is building now. There are some new deprecation warnings to be cleaned up related to bumping pyo3, but those should be straightforward.

Any thoughts on the "x" vs "c0" business in substrait?

westonpace · 2024-11-26T23:07:28Z

Any thoughts on the "x" vs "c0" business in substrait?

If the tests in test_integration.py are passing then we are good. If not, then I'll take a closer look tomorrow morning.

westonpace · 2024-11-26T23:08:01Z

(we only use substrait for pushdown from duckdb / polars and that's what test_integration.py is checking)

jleibs · 2024-11-26T23:20:17Z

Any thoughts on the "x" vs "c0" business in substrait?

If the tests in test_integration.py are passing then we are good. If not, then I'll take a closer look tomorrow morning.

No luck. Integration tests show the same problem:

       actual = duckdb.query("SELECT * FROM ds WHERE filterme = 2").fetch_arrow_table()
E       duckdb.duckdb.Error: ValueError: LanceError(Schema): Schema error: No field named c0. Valid fields are filterme, othercol., /home/jleibs/lance/rust/lance-datafusion/src/substrait.rs:346:9

westonpace · 2024-11-26T23:27:43Z

Ok. I'll take a look tomorrow AM

eddyxu · 2024-11-28T02:10:48Z

@westonpace we can probably remove tfrecord dependency from rust

westonpace · 2024-12-02T17:26:54Z

Sorry, lost track of this over Thanksgiving. I'll get it wrapped up today.

westonpace · 2024-12-03T01:18:16Z

rerun-io#2 (let me know if you want me to just open a new PR from my branch so I can skip the intermediate PRs)

westonpace · 2024-12-04T19:21:14Z

Closing as I've taken this over in #3201

jleibs · 2024-12-05T15:54:14Z

@westonpace sorry I was out for an extended thanksgiving. Thanks for taking over.

Attempt to update arrow/datafusion

262ad05

jleibs changed the title ~~Attempt to update arrow/datafusion~~ chore: update datafusion to 42.0 and arrow to 53.2 Nov 26, 2024

github-actions Bot added the chore label Nov 26, 2024

jleibs marked this pull request as draft November 26, 2024 00:04

jleibs commented Nov 26, 2024

View reviewed changes

Comment thread rust/lance/src/dataset/scanner.rs

jleibs commented Nov 26, 2024

View reviewed changes

Use proper snafu converter

6449f3e

westonpace reviewed Nov 26, 2024

View reviewed changes

jleibs added 2 commits November 26, 2024 11:29

Update substrait dep

03c9b72

Change the handling of bytes.into()

08aa39b

jleibs force-pushed the jleibs/update_arrow_datafusion branch from e128098 to 08aa39b Compare November 26, 2024 17:18

Clean up and centralize bytes::Bytes -> arrow_buffer::Buffer routines

e587921

jleibs and others added 6 commits November 26, 2024 13:51

Attempt to update arrow/datafusion

388740b

Use proper snafu converter

79fe55f

Update substrait dep

6305329

Change the handling of bytes.into()

fcea62e

Clean up and centralize bytes::Bytes -> arrow_buffer::Buffer routines

4187644

Eliminate more arrow-52 deps

8eb87ea

jleibs force-pushed the jleibs/update_arrow_datafusion branch from 63fec42 to 8eb87ea Compare November 26, 2024 18:56

github-actions Bot added the python label Nov 26, 2024

Fix lance-datafusion build

6e88cb0

Add a prost_old to deal with tfrecord

ddcf53c

jleibs added 3 commits November 26, 2024 17:40

Fixing the python deps

9a38e2e

Merge remote-tracking branch 'weston/jleibs/update_arrow_datafusion' …

f2dbdf7

…into jleibs/update_arrow_datafusion

Need py-clone feature to build wheel

155bc5c

Update lock again

7dd031b

westonpace closed this Dec 4, 2024

Conversation

jleibs commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Nov 25, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jleibs Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jleibs commented Nov 26, 2024

Uh oh!

jleibs commented Nov 26, 2024

Uh oh!

jleibs commented Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

westonpace commented Nov 26, 2024

Uh oh!

jleibs commented Nov 26, 2024

Uh oh!

westonpace commented Nov 26, 2024

Uh oh!

westonpace commented Nov 26, 2024

Uh oh!

jleibs commented Nov 26, 2024

Uh oh!

westonpace commented Nov 26, 2024

Uh oh!

eddyxu commented Nov 28, 2024

Uh oh!

westonpace commented Dec 2, 2024

Uh oh!

westonpace commented Dec 3, 2024

Uh oh!

westonpace commented Dec 4, 2024

Uh oh!

jleibs commented Dec 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jleibs commented Nov 25, 2024 •

edited

Loading

jleibs Nov 26, 2024 •

edited

Loading

jleibs commented Nov 26, 2024 •

edited

Loading