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

Add session context to evaluation #446

Merged
merged 5 commits into from
Mar 7, 2024
Merged

Add session context to evaluation #446

merged 5 commits into from
Mar 7, 2024

Conversation

jpschorr
Copy link
Contributor

@jpschorr jpschorr commented Mar 6, 2024

Fixes #388

Adds both a system-level and a user-level session context to the evaluation context. Also plumbs the context through to BaseTableExpr used in partiql_catalog::Extensions.

There is a lot of fiddly lifetime management and testing changes, as well as some slight trait refactors.

It's probably easiest to start with

  • the new integration test in partiql/tests/user_context.rs to see what kinds of things are possible,
  • partiql-catalog/src/context.rs for the new context traits, and
  • partiql-eval/src/eval/mod.rs for how it's used in evaluation.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jpschorr jpschorr force-pushed the feat-context-user branch from 89bf8d5 to 20d255c Compare March 6, 2024 17:47
Copy link

codecov bot commented Mar 6, 2024

Codecov Report

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

Project coverage is 79.21%. Comparing base (00924f2) to head (0a8d71b).

Files Patch % Lines
partiql-eval/src/eval/mod.rs 59.37% 12 Missing and 1 partial ⚠️
partiql-eval/src/error.rs 0.00% 9 Missing ⚠️
partiql-eval/src/eval/expr/operators.rs 57.14% 6 Missing ⚠️
partiql-eval/src/eval/expr/strings.rs 0.00% 6 Missing ⚠️
partiql-catalog/src/context.rs 75.00% 1 Missing ⚠️
partiql-eval/src/eval/evaluable.rs 97.36% 1 Missing ⚠️
partiql-logical-planner/src/lower.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #446      +/-   ##
==========================================
+ Coverage   79.10%   79.21%   +0.11%     
==========================================
  Files          65       66       +1     
  Lines       17517    17721     +204     
  Branches    17517    17721     +204     
==========================================
+ Hits        13856    14037     +181     
- Misses       3091     3116      +25     
+ Partials      570      568       -2     

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

@jpschorr jpschorr changed the title Feat context user Add session context to evaluation Mar 6, 2024
@jpschorr jpschorr marked this pull request as ready for review March 6, 2024 17:56
@jpschorr jpschorr requested a review from am357 March 6, 2024 17:56
@jpschorr jpschorr force-pushed the feat-context-user branch from 20d255c to 10a4b32 Compare March 6, 2024 17:59
Copy link

github-actions bot commented Mar 6, 2024

Conformance comparison report

Base (00924f2) 553d22f +/-
% Passing 90.35% 90.35% 0.00%
✅ Passing 5731 5731 0
❌ Failing 612 612 0
🔶 Ignored 0 0 0
Total Tests 6343 6343 0

Number passing in both: 5731

Number failing in both: 612

Number passing in Base (00924f2) but now fail: 0

Number failing in Base (00924f2) but now pass: 0

&self.sys
}

fn user_context(&self, name: &str) -> Option<&(dyn Any)> {
Copy link

@sadderchris sadderchris Mar 6, 2024

Choose a reason for hiding this comment

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

Is there any way this API can output a generic C instead of a Option<&dyn Any>? I'd really like to implement these traits myself and just return a type that I choose, instead of going through a Option<&dyn Any>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there is, I don't see how.

Making it a generic parameter of the SessionContext would mean that any and all extensions that are loaded would need to be able to deal with that single type.

pub bindings: MapBindings<Value>,

pub sys: SystemContext,
pub user: HashMap<String, &'a (dyn Any)>, // TODO: Unicase the keys?
Copy link

@sadderchris sadderchris Mar 6, 2024

Choose a reason for hiding this comment

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

We'll probably define our own context type, but for the most flexibility, this should be &'a (dyn Any + 'a) to support types that contain lifetimes themselves (e.g. &'a Foo<'a> - else you can only store something that is &'a Foo<'static>).

...This doesn't seem to just work however. I think the user_context method on the SessionContext trait needs to be updated as well

fn user_context<'b>(&'b self, name: &str) -> Option<&'b (dyn Any + 'b)> {}

There might be some other hoops that need to be jumped through as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any is defined as

pub trait Any: 'static {...}

(See https://doc.rust-lang.org/std/any/trait.Any.html)

So you can't add a + 'a

Copy link

@sadderchris sadderchris Mar 7, 2024

Choose a reason for hiding this comment

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

Ok, this won't work for us then. Do you have some time to talk about this in person tomorrow?

I think I've worked out a way around this problem actually - let's still sync up tomorrow if possible.

partiql-eval/src/eval/mod.rs Outdated Show resolved Hide resolved
partiql-eval/src/eval/mod.rs Outdated Show resolved Hide resolved
@jpschorr jpschorr requested a review from am357 March 6, 2024 23:26
Copy link
Contributor

@am357 am357 left a comment

Choose a reason for hiding this comment

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

Approved with one suggestion

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Arash Maymandi <[email protected]>
@jpschorr jpschorr requested a review from sadderchris March 6, 2024 23:36
@jpschorr jpschorr merged commit 4d9ae54 into main Mar 7, 2024
19 checks passed
@jpschorr jpschorr deleted the feat-context-user branch September 4, 2024 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: EvaluationSession.context equivalent
3 participants