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

chore: Remove the subsumption inference used for label polymorphism #4776

Closed
wants to merge 43 commits into from

Conversation

Marwes
Copy link
Contributor

@Marwes Marwes commented May 20, 2022

Removes the old subsumption (subtyping) based label polymorphism in preparation of #4927, #4928 and #4929 . Tests that fail are ignored or removed if they assumed subtyping.

I also added the updated type signatures which are hidden by @feature usage so they do not appear in the current standard library.

@Marwes
Copy link
Contributor Author

Marwes commented Jun 13, 2022

Trying to think of a solution without involving sub typing.

I am thinking we compile the standard library with label polymorphism everywhere (so there is no alternate signatures to deal with). When checking queries outside of the standard library we do an extra pass before typechecking that checks walks the semantic graph and detects when a builtin using labels is accessed. For these cases, if we can statically infer that a builtin is called with string literals we use the type signature with labels, if we are unable to determine that a string literal is used we translate the type signature with labels to one without.

builtin sum : (<-tables: stream[{ A with C: B }], ?column: C = "_value") => stream[{ D with C: B }]
    where A: Record, B: Numeric, C: Label
// translate label variables to string and records containing label fields to an unconstrained record variable
// This should match its original signature
builtin sum : (<-tables: stream[A], ?column: string) => stream[B] where A: Record, B: Record

The cases where we can statically know that a string literal is used is simply

sum(column: "mycolumn")

x = "mycolumn2"
sum(column: x) 
// Gets rewritten to sum(column: "mycolumn2") to match the first case

This should give label inference for most cases, except when users implement their own functions that takes a label as argument.

f = (tables, column) => tables |> sum(column: column) // Defaults to the old string based signature

Marwes pushed a commit that referenced this pull request Jun 21, 2022
The documentation implies that these fields are mandatory to call this function and looking at the implementation it will indeed error if they are missing. Changing this is still technically a breaking change so maybe we want some caution before merging this? (#4773 could perhaps be used to allow an incremental rollout, or I could run this through the query log analyzer I made for #4776)
Marwes pushed a commit that referenced this pull request Jun 21, 2022
The documentation implies that these fields are mandatory to call this function and looking at the implementation it will indeed error if they are missing. Changing this is still technically a breaking change so maybe we want some caution before merging this? (#4773 could perhaps be used to allow an incremental rollout, or I could run this through the query log analyzer I made for #4776)
Marwes pushed a commit that referenced this pull request Jun 21, 2022
The documentation implies that these fields are mandatory to call this function and looking at the implementation it will indeed error if they are missing. Changing this is still technically a breaking change so maybe we want some caution before merging this? (#4773 could perhaps be used to allow an incremental rollout, or I could run this through the query log analyzer I made for #4776)
@Marwes Marwes force-pushed the query_log branch 2 times, most recently from f944ea6 to e8648d7 Compare June 27, 2022 11:41
@Marwes
Copy link
Contributor Author

Marwes commented Jun 27, 2022

So I think the most reasonable plan to move forward here is to not have any sort of subtyping between labels and strings.

To make use of labels we add a new syntax (I propose .mylabel). The type signatures are left as is to avoid breakage in old scripts however we also provide an option to opt-in to the new label based signatures. This option can be enabled on a script by script basis in the GUI (and also possibly via some sort of "use script"; annotation in the script itself).

An option comes with the disadvantage that it is easy for people to just not use it so we will want to enable it by default on any new scripts that are created. (Potentially we could also check if old scripts work with label polymorphism and enable the option if they work).

Marwes pushed a commit that referenced this pull request Jun 29, 2022
... and standard library. Extracted from #4776.
Currently this supports consuming either an sqlite database or a csv file which are the two
sources of scripts I am aware of at the moment.

```bash
cargo run --release --bin analyze_query_log --all-features -- "../../../Downloads/2022-06-28_18 30_influxdb_data.csv" --new-features unusedSymbolWarnings
```
Marwes pushed a commit that referenced this pull request Jul 4, 2022
... and standard library. Extracted from #4776.
Currently this supports consuming either an sqlite database or a csv file which are the two
sources of scripts I am aware of at the moment.

```bash
cargo run --release --bin analyze_query_log --all-features -- "../../../Downloads/2022-06-28_18 30_influxdb_data.csv" --new-features unusedSymbolWarnings
```
Marwes pushed a commit that referenced this pull request Jul 4, 2022
…4938)

* chore: Add a tool to analyze the impact of changes to the typesystem

... and standard library. Extracted from #4776.
Currently this supports consuming either an sqlite database or a csv file which are the two
sources of scripts I am aware of at the moment.

```bash
cargo run --release --bin analyze_query_log --all-features -- "../../../Downloads/2022-06-28_18 30_influxdb_data.csv" --new-features unusedSymbolWarnings
```

* chore: make generate
@Marwes Marwes force-pushed the query_log branch 4 times, most recently from c8ba754 to 817c60d Compare July 6, 2022 13:33
@Marwes Marwes marked this pull request as ready for review July 6, 2022 13:34
@Marwes Marwes requested review from a team as code owners July 6, 2022 13:34
@Marwes Marwes removed the request for review from a team July 6, 2022 13:34
Markus Westerlind added 27 commits August 8, 2022 16:36
Instead we ensure that every override are directly after the original.
Remove tests that checked subtyping
These alternative signatures are in the normal universe.flux now
The documentation already exists for the main signature
Just used for label polymorphism temporarily
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.

2 participants