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

[FEAT] connect: add alias support #3342

Merged
merged 1 commit into from
Nov 21, 2024
Merged

[FEAT] connect: add alias support #3342

merged 1 commit into from
Nov 21, 2024

Conversation

andrewgazelka
Copy link
Member

@andrewgazelka andrewgazelka commented Nov 20, 2024

Copy link
Member Author

andrewgazelka commented Nov 20, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

@andrewgazelka andrewgazelka force-pushed the andrew/connect-alias branch 2 times, most recently from f4273d8 to 6790b74 Compare November 20, 2024 06:04
@andrewgazelka andrewgazelka changed the base branch from andrew/connect-consolidate-session to andrew/fix-addr-in-use November 20, 2024 06:14
Base automatically changed from andrew/connect-consolidate-session to main November 20, 2024 19:52
@github-actions github-actions bot added the enhancement New feature or request label Nov 20, 2024
Copy link

codspeed-hq bot commented Nov 20, 2024

CodSpeed Performance Report

Merging #3342 will degrade performances by 18.45%

Comparing andrew/connect-alias (3402dc6) with main (2c0f3cd)

Summary

❌ 1 regressions
✅ 16 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main andrew/connect-alias Change
test_iter_rows_first_row[100 Small Files] 289.2 ms 354.7 ms -18.45%

Copy link

codecov bot commented Nov 20, 2024

Codecov Report

Attention: Patch coverage is 26.00000% with 259 lines in your changes missing coverage. Please review.

Project coverage is 77.35%. Comparing base (f6eb993) to head (3402dc6).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/daft-connect/src/translation/datatype.rs 7.40% 100 Missing ⚠️
...-connect/src/translation/logical_plan/aggregate.rs 0.00% 44 Missing ⚠️
src/daft-connect/src/translation/literal.rs 0.00% 33 Missing ⚠️
...onnect/src/translation/expr/unresolved_function.rs 0.00% 31 Missing ⚠️
src/daft-connect/src/translation/expr.rs 54.38% 26 Missing ⚠️
...daft-connect/src/translation/logical_plan/range.rs 62.50% 18 Missing ⚠️
src/daft-dsl/src/lit.rs 0.00% 3 Missing ⚠️
src/daft-connect/src/translation/logical_plan.rs 33.33% 2 Missing ⚠️
src/daft-connect/src/lib.rs 80.00% 1 Missing ⚠️
...ft-connect/src/translation/logical_plan/project.rs 88.88% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3342      +/-   ##
==========================================
- Coverage   77.44%   77.35%   -0.10%     
==========================================
  Files         678      685       +7     
  Lines       83309    83629     +320     
==========================================
+ Hits        64519    64689     +170     
- Misses      18790    18940     +150     
Files with missing lines Coverage Δ
src/daft-connect/src/translation/schema.rs 100.00% <100.00%> (+100.00%) ⬆️
src/daft-connect/src/lib.rs 64.21% <80.00%> (+14.45%) ⬆️
...ft-connect/src/translation/logical_plan/project.rs 88.88% <88.88%> (ø)
src/daft-connect/src/translation/logical_plan.rs 61.53% <33.33%> (-2.26%) ⬇️
src/daft-dsl/src/lit.rs 56.28% <0.00%> (-0.49%) ⬇️
...daft-connect/src/translation/logical_plan/range.rs 62.50% <62.50%> (ø)
src/daft-connect/src/translation/expr.rs 54.38% <54.38%> (ø)
...onnect/src/translation/expr/unresolved_function.rs 0.00% <0.00%> (ø)
src/daft-connect/src/translation/literal.rs 0.00% <0.00%> (ø)
...-connect/src/translation/logical_plan/aggregate.rs 0.00% <0.00%> (ø)
... and 1 more

... and 7 files with indirect coverage changes

---- 🚨 Try these New Features:

Comment on lines +31 to +44
pub fn handle_count(arguments: Vec<daft_dsl::ExprRef>) -> eyre::Result<daft_dsl::ExprRef> {
let arguments: [daft_dsl::ExprRef; 1] = match arguments.try_into() {
Ok(arguments) => arguments,
Err(arguments) => {
bail!("requires exactly one argument; got {arguments:?}");
}
};

let [arg] = arguments;

let count = arg.count(CountMode::All);

Ok(count)
}
Copy link

Choose a reason for hiding this comment

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

The handle_count function needs to support both count(*) and count(expr) cases. Currently it only handles the single argument case. For zero arguments (i.e. count(*)), it should return a count of all rows. For one argument (i.e. count(expr)), it should count non-null values of that expression.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

Copy link

graphite-app bot commented Nov 21, 2024

Graphite Automations

"Notify author when CI fails" took an action on this PR • (11/21/24)

1 teammate was notified to this PR based on Andrew Gazelka's automation.

Copy link
Member Author

Merge activity

  • Nov 20, 8:59 PM EST: Graphite disabled "merge when ready" on this PR due to: a merge conflict with the target branch; resolve the conflict and try again..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants