feat: Add simple enumerations to type system#6104
Conversation
prql-bot
left a comment
There was a problem hiding this comment.
Nice feature — the design of storing enum members as a module (with NS_SELF for the type and one Decl per alias) so bare option names resolve is clean, and it keeps join side:left etc. backwards-compatible. I built it locally and confirmed the new tests pass and existing call styles (side:left, from_text format:json) still compile. Two things worth a look, plus one inline nit.
Error message regression for invalid built-in option values. Because these params now resolve their argument in the enum namespace and then fall back to normal name resolution, an invalid value that happens to match (or wildcard-match) a column no longer produces the old crisp message. On main today join side:xyz gives `side` expected inner, left, right or full, but found xyz; with this PR it becomes:
│ ╰─── Ambiguous name
│ Help: could be any of: that.b.xyz, this.a.xyz
│ Note: perhaps you meant one of: inner, left, right, full
The Note: you added does carry the valid options, but leading with Ambiguous name + column candidates is more confusing than the previous message for what's usually a typo'd option. Same shape shows up in the updated append_by_wrong snapshot. Not a blocker — flagging so a maintainer can weigh whether the enum-namespace fallback should still short-circuit to the "expected one of …" error when the name isn't a valid member.
Missing CHANGELOG entry. This is a user-facing language feature but CHANGELOG.md isn't touched; the [unreleased] → Features section is empty. Your comparable append by:name feature (#6046) got an entry under the previous release. Worth adding one line here.
|
The syntax in this PR is: I think the syntax in other languages such as Rust, Python and C# is little nicer. In Rust it would be a little cleaner: enum InvoiceStatus {
Paid = 0,
Unpaid = 1,
Canceled = 2,
}But maybe it needs the syntax in the PR to be able to know that |
|
The error message change is a function of the fact that now |
I decided that the easiest path to implementation here was to reuse the existing |
prql-bot
left a comment
There was a problem hiding this comment.
The CHANGELOG entry and the expr.alias = None fix look good — the new test_enum_1b correctly pins select { status = InvoiceStatus.Paid } to 0 AS status rather than the member's own alias clobbering it. One tiny formatting nit inline.
|
Fair point — you're right that once The only residual wrinkle is presentational: leading with Thanks for adding the CHANGELOG entry too. |
|
@vanillajonathan I was able to implement the cleaner enum syntax in the parser, so the syntax in this PR is now: None of the tests failed so I think we're all set. |
Resolves #4501.
This PR adds a new
Enumtype which takes a "literal tuple" (a tuple with all fields named and having literal values). This type can be used to represent simple explicit enumerations of a database column or a function parameter.In addition, special-case handling of name resolution was added to function parameters having been assigned an
Enumtype. In this case, theEnummodule namespace is preferentially searched prior to any other namespaces. This allows for functions such asjoinortuple_uniqto have their option parameters (such asside) be associated with anEnumand be fully syntax backwards compatible.Unfortunately, the discussion in #4501 that centered around the use of Named Unions to implement enumerations was not able to be followed, since named unions were removed in PR #5111. The design of the "simple enumeration" was carefully chosen such that when the type system is enhanced in the future to once again support sum types and named unions, the existing syntax can be maintained for backwards compatibility reasons yet have its implementation updated to align with sum types.