[FEATURE] daft-connect

[andrewgazelka]

• resolve todos
• use workspace deps
• remove panics / better error messages

fmt/etc an hour ago
└─ Andrew Gazelka
   └─ Cargo.toml:305 - # todo: remove this at some point

fix up 2 hours ago
└─ Andrew Gazelka
   └─ src/daft-connect/src/convert.rs:22 - // todo: support more truncate options

update 19 hours ago
└─ Andrew Gazelka
   ├─ src/daft-connect/src/convert.rs:15 - // todo: a way to do something like tracing scopes but with errors?
   ├─ src/daft-connect/src/convert.rs:72 - // todo: test
   ├─ src/daft-connect/src/convert/expr.rs:78 - "/" => Operator::FloorDivide, // todo is this what we want?
   └─ src/daft-plan/src/builder.rs:302 - // todo: should NOT broadcast; should only set first row

stash 5 days ago
└─ Andrew Gazelka
   └─ src/daft-connect/src/lib.rs:262 - operation_id: Some(request.operation_id), // todo: impl properly

stash 6 days ago
└─ Andrew Gazelka
   └─ src/daft-connect/src/config.rs:146 - // todo: need to implement this

stash a week ago
└─ Andrew Gazelka
   ├─ src/daft-connect/proto/spark/connect/commands.proto:266 - // TODO: How do we indicate errors?
   ├─ src/daft-connect/proto/spark/connect/commands.proto:267 - // TODO: Consider adding status, last progress etc here.
   └─ src/daft-connect/proto/spark/connect/commands.proto:313 - // TODO: Consider reusing Explain from AnalyzePlanRequest message.

[github-actions]

This PR is being prevented from merging because you need at least one of the required labels:

enhancement | performance | bug | chore | documentation | dependencies

The canonical and easiest way of adding them is to add the following prefixes to your PR title:

• [FEAT]: adds the enhancement label
• [PERF]: adds the performance label
• [BUG]: adds the bug label
• [CHORE]: adds the chore label
• [DOCS]: adds the documentation label

Thanks for helping us categorize and manage our PRs!

[codspeed-hq]

CodSpeed Performance Report

Merging #3038 will not alter performance

_{Comparing andrew/daft-connect (b5b0953) with main (a3453d1)}

Summary

✅ 17 untouched benchmarks

[codecov]

Codecov Report

Attention: Patch coverage is 50.60976% with 405 lines in your changes missing coverage. Please review.

Project coverage is 76.82%. Comparing base (a3453d1) to head (b5b0953).

Files with missing lines	Patch %	Lines
src/daft-connect/src/lib.rs	0.00%	147 Missing ⚠️
src/daft-connect/src/command.rs	8.41%	98 Missing ⚠️
src/daft-connect/src/config.rs	31.91%	96 Missing ⚠️
src/daft-connect/src/convert/expr.rs	54.79%	33 Missing ⚠️
src/daft-connect/src/main.rs	0.00%	21 Missing ⚠️
src/daft-connect/src/convert.rs	91.66%	6 Missing ⚠️
src/daft-table/src/lib.rs	0.00%	3 Missing ⚠️
src/daft-sql/src/planner.rs	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3038      +/-   ##
==========================================
- Coverage   78.45%   76.82%   -1.64%     
==========================================
  Files         610      618       +8     
  Lines       71993    74089    +2096     
==========================================
+ Hits        56484    56920     +436     
- Misses      15509    17169    +1660     

Files with missing lines	Coverage Δ
src/daft-connect/src/convert/tests.rs	100.00% <100.00%> (ø)
src/daft-dsl/src/expr/mod.rs	75.98% <100.00%> (+0.42%)	⬆️
src/daft-local-execution/src/lib.rs	90.47% <ø> (ø)
src/daft-plan/src/builder.rs	84.01% <100.00%> (+3.16%)	⬆️
src/spark-connect/src/lib.rs	0.00% <ø> (ø)
src/daft-sql/src/planner.rs	62.90% <66.66%> (-0.29%)	⬇️
src/daft-table/src/lib.rs	89.59% <0.00%> (-0.46%)	⬇️
src/daft-connect/src/convert.rs	91.66% <91.66%> (ø)
src/daft-connect/src/main.rs	0.00% <0.00%> (ø)
src/daft-connect/src/convert/expr.rs	54.79% <54.79%> (ø)
... and 3 more

... and 4 files with indirect coverage changes

[universalmind303]

@andrewgazelka just a suggestion, but since the conversion logic from spark to daft is soo similar to what we've done in daft-sql with the conversion between sql and daft, I think it'd make a lot of sense to structure the code similarly. It'd reduce a lot of cognitive overhead for developers if they follow the same general patterns and code structure.

[universalmind303]

We should add a README to the spark-connect crate stating that it's prost generated rust structs from the spark connect protobufs.

[andrewgazelka]

@andrewgazelka just a suggestion, but since the conversion logic from spark to daft is soo similar to what we've done in daft-sql with the conversion between sql and daft, I think it'd make a lot of sense to structure the code similarly. It'd reduce a lot of cognitive overhead for developers if they follow the same general patterns and code structure.

OK, I will look over your code and if I have over a certain number of questions, I’m thinking it could be good to pair program to figure out what makes the most sense but I’m all for making things easier for contributors