Add new golden files with VARIANT values#6140
Conversation
|
|
||
| // There are 24 kinds of variant | ||
| pub type GoldenRowVariant = Tup25< | ||
| Variant, |
There was a problem hiding this comment.
can we make some of them Option to get more coverage (maybe every 2nd one?), it changes the layout
There was a problem hiding this comment.
There are no option Variant values. A NULL is stored as a NULL value at runtime.
| pub fn golden_row_variant(row: usize) -> GoldenRowVariant { | ||
| let row_u64 = 0x0101_0000_0000_0000u64.wrapping_add(row as u64); | ||
|
|
||
| // This has the me monotone |
There was a problem hiding this comment.
we're not building any nested variants here? should we?
There was a problem hiding this comment.
There are no nested variants - we are building one of each of the possible types.
The only array supported is VARIANT ARRAY.
Similar for MAP.
There was a problem hiding this comment.
There should be full coverage of the Variant enum type in this test.
Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
mythical-fred
left a comment
There was a problem hiding this comment.
LGTM. Two small comment nits inline.
| // None optimization added to larger tups with the v4 storage format. | ||
| pub type GoldenRowSmall = Tup8<u64, bool, i32, i64, F32, SqlString, Opt<i32>, Opt<ByteArray>>; | ||
|
|
||
| // There are 24 kinds of variant |
There was a problem hiding this comment.
Nit: there are actually 25 Variant variants (SqlNull, VariantNull, Boolean, TinyInt, SmallInt, Int, BigInt, UTinyInt, USmallInt, UInt, UBigInt, Real, Double, SqlDecimal, String, Date, Time, Timestamp, ShortInterval, LongInterval, Binary, Geometry, Uuid, Array, Map) — which matches Tup25 and the 25 fields you build in golden_row_variant. The comment understates by one.
| pub fn golden_row_variant(row: usize) -> GoldenRowVariant { | ||
| let row_u64 = 0x0101_0000_0000_0000u64.wrapping_add(row as u64); | ||
|
|
||
| // This has the me monotone |
There was a problem hiding this comment.
Typo: // This has the me monotone — should be // This has to be monotone (or just merge with the comment a few lines below).
Fixes #6132