feat: update arrow to 51, datafusion to 37 #2240

westonpace · 2024-04-22T21:26:35Z

No description provided.

westonpace · 2024-04-22T21:27:34Z

rust/lance-datafusion/src/exec.rs

+            schema: schema.clone(),
+            properties: PlanProperties::new(
+                EquivalenceProperties::new(schema),
+                Partitioning::RoundRobinBatch(1),
+                datafusion::physical_plan::ExecutionMode::Bounded,
+            ),
        }


The ExecPlan interface changed. output_partitioning and output_ordering have gone away and properties() now replaces both of those (as well as a new field, "execution mode".

westonpace · 2024-04-22T21:28:21Z

rust/lance/src/datafusion/logical_expr.rs

+pub trait ExprExt {
+    // Helper function to replace Expr::field in DF 37 since DF
+    // confuses itself with the GetStructField returned by Expr::field
+    fn field_newstyle(&self, name: &str) -> Expr;


This was the most complicated change. The comment explains it ok. I expect this may eventually go away (e.g. when apache/datafusion#10181 is resolved)

westonpace · 2024-04-22T21:28:55Z

rust/lance/src/datafusion/logical_expr.rs

        // TODO: consider making this dispatch more generic, i.e. fun.output_type -> coerce
        // instead of hardcoding coerce method for each function
        Expr::ScalarFunction(ScalarFunction {
-            func_def: ScalarFunctionDefinition::BuiltIn(BuiltinScalarFunction::RegexpMatch),
+            func_def: ScalarFunctionDefinition::UDF(udf),


There are no more builtin functions. So we need to look for regexp_match as a UDF instead.

westonpace · 2024-04-22T21:30:04Z

rust/lance/src/io/exec/optimizer.rs

@@ -25,16 +25,18 @@ impl PhysicalOptimizerRule for CoalesceTake {
        plan: Arc<dyn ExecutionPlan>,
        _config: &ConfigOptions,
    ) -> DFResult<Arc<dyn ExecutionPlan>> {
-        plan.transform_down(&|plan| {


DF made minor changes to the transform_down function but no real change here (e.g. Yes -> yes and we return transform_down(...).data()

westonpace · 2024-04-22T21:30:56Z

rust/lance/src/io/exec/planner.rs

@@ -183,25 +203,23 @@ impl ContextProvider for LanceContextProvider {
        )))
    }

+    fn get_aggregate_meta(&self, name: &str) -> Option<Arc<AggregateUDF>> {


SqlToRel now relies on the ContextProvider for functions (e.g. regexp_match). These come from a SessionState instance and so we needed to add one of those to the LanceContextProvider (the default is fine)

westonpace · 2024-04-22T21:31:42Z

rust/lance/src/io/exec/testing.rs

@@ -69,6 +75,10 @@ impl ExecutionPlan for TestingExec {
    }

    fn statistics(&self) -> datafusion::error::Result<datafusion::physical_plan::Statistics> {
-        todo!()


Note: DF now actually calls statistics and so we can't get away with todo!() here anymore.

eddyxu · 2024-04-22T21:36:55Z

Cargo.toml

+    "array_expressions",
+    "regex_expressions",
+] }
+datafusion-common = "37.0"


Datafusion is on 37.1 https://docs.rs/datafusion/latest/datafusion/

Should we bump to datafusion 37.1?

Oh yea, maybe just 37 for simplicity.

Actually, probably best to do 37.1 just to avoid issues. It doesn't change that often.

If we don't have a reason to require >=37.1, I feel like it would be nice to keep at 37.0. Then users can use any 37.X version.

Do you mean 37? Or does 37.0 allow 37.1?

Scanning through apache/datafusion#9904 I don't see any defects that would impact us.

eddyxu

Pending CI

codecov-commenter · 2024-04-22T21:58:47Z

Codecov Report

Attention: Patch coverage is 84.70588% with 52 lines in your changes are missing coverage. Please review.

Project coverage is 81.07%. Comparing base (1b8adb5) to head (98b2049).
Report is 1 commits behind head on main.

Files	Patch %	Lines
rust/lance/src/io/exec/planner.rs	77.21%	18 Missing ⚠️
rust/lance/src/datafusion/logical_expr.rs	83.33%	3 Missing and 5 partials ⚠️
rust/lance/src/io/exec/scalar_index.rs	80.55%	7 Missing ⚠️
rust/lance-index/src/scalar/expression.rs	0.00%	6 Missing ⚠️
rust/lance/src/io/exec/optimizer.rs	85.71%	0 Missing and 5 partials ⚠️
rust/lance-datafusion/src/exec.rs	81.81%	2 Missing ⚠️
rust/lance/src/io/exec/testing.rs	84.61%	2 Missing ⚠️
rust/lance-index/src/scalar/btree.rs	0.00%	1 Missing ⚠️
rust/lance/src/io/exec/projection.rs	95.00%	0 Missing and 1 partial ⚠️
rust/lance/src/io/exec/pushdown_scan.rs	95.65%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2240      +/-   ##
==========================================
+ Coverage   81.01%   81.07%   +0.06%     
==========================================
  Files         186      186              
  Lines       53583    53722     +139     
  Branches    53583    53722     +139     
==========================================
+ Hits        43408    43554     +146     
+ Misses       7707     7690      -17     
- Partials     2468     2478      +10

Flag	Coverage Δ
unittests	`81.07% <84.70%> (+0.06%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

wjones127 · 2024-04-22T21:54:18Z

rust/lance/src/datafusion/logical_expr.rs

@@ -34,6 +38,45 @@ fn resolve_value(expr: &Expr, data_type: &DataType) -> Result<Expr> {
    }
 }

+/// A simple helper function that interprets an Expr as a string scalar
+/// or returns None if it is not.
+pub fn get_as_string_scalar_opt(expr: &Expr) -> Option<&String> {


nit: &String in a signature is pretty much never desirable.

Suggested change

pub fn get_as_string_scalar_opt(expr: &Expr) -> Option<&String> {

pub fn get_as_string_scalar_opt(expr: &Expr) -> Option<&str> {

Changed to &str.

wjones127 · 2024-04-22T21:55:48Z

rust/lance/src/datafusion/logical_expr.rs

-                        Ok(expr.clone())
-                    }
+                }))
+            } else if let Some(left_type) = dbg!(resolve_column_type(left.as_ref(), schema)) {


Left over dbg!?

Suggested change

} else if let Some(left_type) = dbg!(resolve_column_type(left.as_ref(), schema)) {

} else if let Some(left_type) = resolve_column_type(left.as_ref(), schema) {

Good catch. Removed.

…tions from review

Update arrow to 51, datafusion to 37

2eb43d8

westonpace requested review from eddyxu and wjones127 April 22, 2024 21:26

westonpace commented Apr 22, 2024

View reviewed changes

eddyxu reviewed Apr 22, 2024

View reviewed changes

Use 37.1 instead of 37.0

84d683f

eddyxu approved these changes Apr 22, 2024

View reviewed changes

wjones127 approved these changes Apr 22, 2024

View reviewed changes

westonpace added 3 commits April 22, 2024 15:02

A few more fixes missed because I didn't have the substrait feature on

2298a71

Remove debug lines, move test-only trait into tests mod. Apply sugges…

370694d

…tions from review

One more clippy fix

98b2049

westonpace merged commit 8ac02bc into lancedb:main Apr 23, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update arrow to 51, datafusion to 37 #2240

feat: update arrow to 51, datafusion to 37 #2240

westonpace commented Apr 22, 2024

westonpace Apr 22, 2024

westonpace Apr 22, 2024

westonpace Apr 22, 2024

westonpace Apr 22, 2024

westonpace Apr 22, 2024

westonpace Apr 22, 2024

eddyxu Apr 22, 2024

westonpace Apr 22, 2024

westonpace Apr 22, 2024

westonpace Apr 22, 2024

wjones127 Apr 22, 2024

westonpace Apr 22, 2024

eddyxu left a comment

codecov-commenter commented Apr 22, 2024 •

edited

Loading

wjones127 Apr 22, 2024

westonpace Apr 22, 2024

wjones127 Apr 22, 2024

westonpace Apr 22, 2024

	pub fn get_as_string_scalar_opt(expr: &Expr) -> Option<&String> {
	pub fn get_as_string_scalar_opt(expr: &Expr) -> Option<&str> {

	} else if let Some(left_type) = dbg!(resolve_column_type(left.as_ref(), schema)) {
	} else if let Some(left_type) = resolve_column_type(left.as_ref(), schema) {

feat: update arrow to 51, datafusion to 37 #2240

feat: update arrow to 51, datafusion to 37 #2240

Conversation

westonpace commented Apr 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyxu left a comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 22, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 22, 2024 •

edited

Loading