Skip to content

Dataflow: Move the shared library to a properly shared qlpack. #13863

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

Merged
merged 13 commits into from
Aug 2, 2023

Conversation

aschackmull
Copy link
Contributor

🎉 This makes the data flow library into a proper qlpack!

Several small tweaks were needed for the individual languages to ensure that the predicate types matched the signature.

Comment on lines +57 to +63
/**
* The cost limits for the `AccessPathFront` to `AccessPathApprox` expansion.
*
* `apLimit` bounds the acceptable fan-out, and `tupleLimit` bounds the
* estimated per-`AccessPathFront` tuple cost. Access paths exceeding both of
* these limits are represented with lower precision during pruning.
*/

Check warning

Code scanning / CodeQL

Predicate QLDoc style.

The QLDoc for a predicate without a result should start with 'Holds'.
Comment on lines +69 to +75
/**
* The cost limits for the `AccessPathApprox` to `AccessPath` expansion.
*
* `apLimit` bounds the acceptable fan-out, and `tupleLimit` bounds the
* estimated per-`AccessPathApprox` tuple cost. Access paths exceeding both of
* these limits are represented with lower precision.
*/

Check warning

Code scanning / CodeQL

Predicate QLDoc style.

The QLDoc for a predicate without a result should start with 'Holds'.
* Constructs a global data flow computation.
*/
module Global<ConfigSig Config> implements GlobalFlowSig {
private module C implements FullStateConfigSig {

Check warning

Code scanning / CodeQL

Data flow configuration module naming

Modules implementing a data flow configuration should end in `Config`.
* Constructs a global data flow computation using flow state.
*/
module GlobalWithState<StateConfigSig Config> implements GlobalFlowSig {
private module C implements FullStateConfigSig {

Check warning

Code scanning / CodeQL

Data flow configuration module naming

Modules implementing a data flow configuration should end in `Config`.
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

Swift changes LGTM.

I'd like to see DCA runs on this pull request.

@aschackmull
Copy link
Contributor Author

I'd like to see DCA runs on this pull request.

I'll start those shortly - I just wanted to clear out the worst compilation errors with some quick CI first.

@aschackmull
Copy link
Contributor Author

Dca looks uneventful.

@aschackmull aschackmull added no-change-note-required This PR does not need a change note and removed no-change-note-required This PR does not need a change note labels Aug 2, 2023
Copy link
Contributor

@atorralba atorralba left a comment

Choose a reason for hiding this comment

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

Java changes look plausible to me (aside from the QLDoc check failures, which I'm not sure whether you intend to address).

Thanks for doing this!

@aschackmull
Copy link
Contributor Author

aside from the QLDoc check failures, which I'm not sure whether you intend to address

I don't 😄

Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

C/C++/Swift 👍

Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

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

Go LGTM

Copy link
Contributor

@alexrford alexrford left a comment

Choose a reason for hiding this comment

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

Ruby 👍

Copy link
Contributor

@alexrford alexrford left a comment

Choose a reason for hiding this comment

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

Python also 👍

Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

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

Looks plausible to me! 👍
Thank you Anders!

@aschackmull aschackmull merged commit 7bc8bf6 into github:main Aug 2, 2023
@aschackmull aschackmull deleted the dataflow/pack4 branch August 2, 2023 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants