ARROW-4464: [Rust] [DataFusion] Add support for LIMIT by ntrinquier · Pull Request #3669 · apache/arrow

ntrinquier · 2019-02-16T22:17:49Z

No description provided.

andygrove

This is looking good so far ... I've left some feedback that should help you out

andygrove · 2019-02-16T22:32:38Z

rust/datafusion/src/execution/context.rs

+
+                let input_schema = input_rel.as_ref().borrow().schema().clone();
+
+                let compiled_expr = compile_scalar_expr(&self, expr, &input_schema)?;


The LIMIT expression will be a literal e.g. LIMIT 10 so I don't think we need to compile and evaluate the expression but just use pattern matching to extract the number.

match expr { &Expr::Literal(ref value) => /* extract value */, _ => /* error - only literals are supported */ }

``

andygrove · 2019-02-16T22:32:58Z

rust/datafusion/src/execution/limit.rs

+    input: Rc<RefCell<Relation>>,
+    expr: RuntimeExpr,
+    schema: Arc<Schema>,
+    num_consumed_rows: i64,


add limit: usize

andygrove · 2019-02-16T22:33:21Z

rust/datafusion/src/execution/limit.rs

+    input: Rc<RefCell<Relation>>,
+    expr: RuntimeExpr,
+    schema: Arc<Schema>,
+    num_consumed_rows: i64,


num_consumed_rows should be usize

andygrove · 2019-02-16T22:33:53Z

rust/datafusion/src/execution/limit.rs

+    fn next(&mut self) -> Result<Option<RecordBatch>> {
+        match self.input.borrow_mut().next()? {
+            Some(batch) => {
+                match self.expr.get_func()(&batch)?


no need to evaluate the expression, use limit field from struct

andygrove · 2019-02-16T22:34:42Z

rust/datafusion/src/execution/limit.rs

+}
+
+//TODO: move into Arrow array_ops
+fn filter(a: &Array, num_rows_to_read: i64) -> Result<ArrayRef> {


rename filter to limit

ntrinquier · 2019-02-16T23:30:14Z

Thanks a lot @andygrove

I extracted the limit and cast it with as, I don't know if there is a safer alternative though.

codecov-io · 2019-02-17T11:26:52Z

Codecov Report

Merging #3669 into master will decrease coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3669      +/-   ##
==========================================
- Coverage   87.85%   87.76%   -0.09%     
==========================================
  Files         689      689              
  Lines       84012    84023      +11     
  Branches     1081     1081              
==========================================
- Hits        73805    73745      -60     
- Misses      10094    10163      +69     
- Partials      113      115       +2

Impacted Files	Coverage Δ
go/arrow/math/int64_avx2_amd64.go	`0% <0%> (-100%)`	⬇️
go/arrow/memory/memory_avx2_amd64.go	`0% <0%> (-100%)`	⬇️
go/arrow/math/float64_avx2_amd64.go	`0% <0%> (-100%)`	⬇️
go/arrow/math/uint64_avx2_amd64.go	`0% <0%> (-100%)`	⬇️
go/arrow/math/float64_amd64.go	`33.33% <0%> (-33.34%)`	⬇️
go/arrow/math/int64_amd64.go	`33.33% <0%> (-33.34%)`	⬇️
go/arrow/math/uint64_amd64.go	`33.33% <0%> (-33.34%)`	⬇️
go/arrow/math/math_amd64.go	`31.57% <0%> (-31.58%)`	⬇️
go/arrow/memory/memory_amd64.go	`28.57% <0%> (-28.58%)`	⬇️
js/src/ipc/metadata/json.ts	`92.39% <0%> (-4.35%)`	⬇️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a374c3c...e93df93. Read the comment docs.

andygrove · 2019-02-17T19:19:56Z

rust/datafusion/src/execution/limit.rs

+                        .collect();
+
+                    let limited_batch: RecordBatch =
+                        RecordBatch::new(Arc::new(Schema::empty()), limited_columns?);


The new batch needs the same schema as the original, not an empty schema

Once this is fixed, I'm ready to approve. Thanks @ntrinquier !

Indeed that makes more sense. filter is also creating the new batch with an empty schema, so I guess this would have to be changed there too?

oops, good catch, yes I'll file a bug for that

@andygrove I created it: https://issues.apache.org/jira/browse/ARROW-4721

andygrove

LGTM

Add support for Limit

eac5a24

andygrove reviewed Feb 16, 2019

View reviewed changes

Make limit an usize and avoid evaluting the limit expression

dbc639f

Remove redundant variable

e93df93

andygrove requested changes Feb 17, 2019

View reviewed changes

Use the previous batch's schema for Limit

c78ae2c

andygrove approved these changes Feb 17, 2019

View reviewed changes

Nicolas Trinquier added 2 commits February 17, 2019 21:00

Merge remote-tracking branch 'upstream/master' into arrow-4464

2ed488c

Add Limit case to ProjectionPushDown

facc5c2

andygrove approved these changes Feb 17, 2019

View reviewed changes

andygrove closed this in 811c7dc Feb 17, 2019

ntrinquier deleted the arrow-4464 branch February 17, 2019 23:52

This was referenced Feb 28, 2019

[Rust] [DataFusion] Add support for LIMIT #21021

Closed

[Rust] [DataFusion] Propagate schema in filter #21244

Closed


		let input_schema = input_rel.as_ref().borrow().schema().clone();

		let compiled_expr = compile_scalar_expr(&self, expr, &input_schema)?;

Conversation

ntrinquier commented Feb 16, 2019

Uh oh!

andygrove left a comment

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!

ntrinquier commented Feb 16, 2019

Uh oh!

codecov-io commented Feb 17, 2019

Codecov Report

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!

andygrove left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants