repr: add Datum::Range + misc.
#16296
Conversation
sploiselle
force-pushed
the
range-datum
branch
from
238a7e2
to
9a47c2d
Compare
src/repr/src/row.rs
Outdated
| // This essentially tests the encoding we'll need for non-partition | ||
| // reclocking data. | ||
| (None, Some(Datum::Null), "(,null)"), | ||
| (Some((Datum::Null, true)), None, "[null,)"), | ||
| (Some((Datum::Null, false)), None, "(null,)"), |
There was a problem hiding this comment.
I'm confused about this! PostgreSQL doesn't support null as a range boundary. It seems like we shouldn't either? Do we need this for reclocking?
There was a problem hiding this comment.
Yes, unfortunately we need this for reclocking, or rather, this is the most sensible value to use for PartitionId::None in reclocking. The value never gets surfaced or returned to users, so permitting it "back here" seemed fine. We could use some other magical value, but this seemed the most semantic.
There was a problem hiding this comment.
Oh, weird, ok. My specific concern is that NULL is used to mean "infinity" in certain contexts in PostgreSQL ranges
benesch=# select int4range(NULL, 1);
int4range
-----------
(,1)
(1 row)
but as long as this is internal only it's probably fine! Why doesn't this ever get exposed to users? Does the reclocking relation just not expose the partition column for source types that don't have meaningful partitions?
There was a problem hiding this comment.
Dug into this and it's possible we don't need this null-supporting feature. I'll add the panics on Datum::Null back in and can remove them if native timestamp reclocking turns out to need them.
|
Exciting to see this come together so quickly, Sean! |
sploiselle
force-pushed
the
range-datum
branch
2 times, most recently
from
1dd8d73
to
9b5d234
Compare
|
@benesch Believe I addressed all comments; PTAL. |
src/repr/Cargo.toml
Outdated
| @@ -54,6 +55,7 @@ tracing-subscriber = { version = "0.3.16", default-features = false, optional = | |||
|
|
|||
| [dev-dependencies] | |||
| criterion = { version = "0.4.0" } | |||
| paste = "1.0.9" | |||
There was a problem hiding this comment.
| paste = "1.0.9" |
| /// Treats `Datum::Null` as an infinite bound (appropriate for PG | ||
| /// semantics), and all other `Datum`s as finite values. |
There was a problem hiding this comment.
This feels a little sketchy to me. I know it's what PostgreSQL does, but I'd almost prefer we do it in the one spot that PostgreSQL does it (range constructors, I think?) and nowhere else? But, idk, it's hard to know without seeing the rest of the code. No strong feels for the moment!
There was a problem hiding this comment.
Yeah--I think this API will shift as we build out SQL support for range types, but want to merge as is for sake convenience when building out the reclock migration.
sploiselle
force-pushed
the
range-datum
branch
from
9b5d234
to
8005880
Compare
We're looking to support range types, and this is the first step.
Once this lands, we'll be able to perform the migration of the current reclocking data to the new, more Timely-friendly format.
After that we'll extend support for the range types (e.g. making them
SELECTable).Motivation
This PR adds a known-desirable feature. Design doc
Checklist
This PR has adequate test coverage / QA involvement has been duly considered.
This PR evolves an existing
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way) and therefore is tagged with aT-protolabel.This PR includes the following user-facing behavior changes: