Skip to content

Value unification - #140

Merged
incipit0 merged 16 commits into
mainfrom
value-unification
Sep 6, 2026
Merged

Value unification#140
incipit0 merged 16 commits into
mainfrom
value-unification

Conversation

@olynch

@olynch olynch commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

This pull request unifies the various "value" enums into a single generic enum that looks like

enum Value<I> {
  Id(I)
  Int(i32)
  Str(String)
}

Unfortunately, this means that some of the conversions that used to be done with .into have to be a little more explicit, because we can't implement a generic From<I>.

We also rename the various RowId variants to be a bit more consistent. The renaming is across the following axes

  • Txn vs no Txn -- row ids that might appear temporarily as part of a transaction, or permanent row ids
  • Live vs. Wire vs. Packed -- Live is row ids created as part of a transaction that automatically turn into permanent row ids after the transaction is done (which we will remove soon), Wire is row ids in the wire format (e.g. with commit hash), and Packed is row ids in the packed format (e.g. with commit id produced by an IdMapper).

So what used to be RowId is now WireRowId, what used to be RowHandle is TxnLiveRowId, what used to be RowRef is TxnWireRowId, PackedRowId remains the same.

As a convenience, we have type aliases type XXXValue = Value<XXXRowId>, so WireValue = Value<WireRowId>, and so on.

@olynch
olynch changed the base branch from main to trunk-for-2026-09-demo September 2, 2026 16:44
@olynch

olynch commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

I think I'm going to make another PR that removes the current coln-js-runtime on the trunk-for-2026-09-demo branch.

@incipit0 incipit0 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks really nice! Given this is a big refactoring, and does not have much functional change, perhaps merge it into main once it's ready, because it will affect almost all future work.

I skimmed through it, but the only functional difference is the liven_all method is that right? I think it otherwise looks good.

Also is the coln-js-runtime problem easy to fix?

Comment thread packages/coln-store/src/solver/bind.rs Outdated
@incipit0
incipit0 changed the base branch from trunk-for-2026-09-demo to main September 3, 2026 21:41
ir::Term::Lit {
lit: ir::Lit::Int { value },
lit: ir::Lit::Int {
value: value.try_into().unwrap(),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@lstwn this is a bit unsatisfying, but Owen temporarily changed the value in the IR to be a i32, to make the JS side nice, I think. Seems to be a pretty substantial change to make all values in coln-query i32, so I did this. What do you think?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Fine, it's just a test util and it's never called with a value that doesn't fit an i32 so we could also change call sites. But given this is just temporary, we can keep it like this.

@incipit0
incipit0 requested a review from lstwn September 3, 2026 21:49
@incipit0
incipit0 added this pull request to the merge queue Sep 6, 2026
Merged via the queue into main with commit 31196e9 Sep 6, 2026
6 checks passed
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.

3 participants