Skip to content

Commit d9ded20

Browse files
authored
Convert extension type scalars to Value correctly (#8635)
Before this PR, we were converting Vortex extension dtypes e.g. timestamp to underlying types i.e. i64 instead of TIMESTAMP while creating a Duckdb Value. Signed-off-by: Mikhail Kot <mikhail@spiraldb.com>
1 parent c7e8ae9 commit d9ded20

1 file changed

Lines changed: 123 additions & 7 deletions

File tree

vortex-duckdb/src/convert/scalar.rs

Lines changed: 123 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -255,6 +255,14 @@ impl TryFrom<Value> for Scalar {
255255
}
256256
}
257257

258+
impl TryFrom<Scalar> for Value {
259+
type Error = VortexError;
260+
261+
fn try_from(scalar: Scalar) -> Result<Self, Self::Error> {
262+
scalar.try_to_duckdb_scalar()
263+
}
264+
}
265+
258266
impl<'a> TryFrom<&'a ValueRef> for Scalar {
259267
type Error = VortexError;
260268

@@ -375,15 +383,21 @@ mod tests {
375383
use rstest::rstest;
376384
use vortex::dtype::DType;
377385
use vortex::dtype::Nullability;
386+
use vortex::dtype::PType;
378387
use vortex::dtype::extension::ExtDType;
388+
use vortex::extension::datetime::Date;
389+
use vortex::extension::datetime::Time;
390+
use vortex::extension::datetime::TimeUnit;
379391
use vortex::extension::datetime::Timestamp;
380392
use vortex::extension::datetime::TimestampOptions;
381393
use vortex::scalar::Scalar;
394+
use vortex::scalar::ScalarValue;
382395
use vortex_geo::extension::GeoMetadata;
383396
use vortex_geo::extension::WellKnownBinary;
384397

385398
use crate::convert::ToDuckDBScalar;
386399
use crate::cpp::DUCKDB_TYPE;
400+
use crate::duckdb::ExtractedValue;
387401
use crate::duckdb::Value;
388402

389403
#[test]
@@ -409,13 +423,6 @@ mod tests {
409423

410424
#[test]
411425
fn test_timestamp_roundtrip() {
412-
use vortex::dtype::DType;
413-
use vortex::dtype::Nullability;
414-
use vortex::dtype::PType;
415-
use vortex::extension::datetime::TimeUnit;
416-
use vortex::scalar::Scalar;
417-
use vortex::scalar::ScalarValue;
418-
419426
#[rustfmt::skip]
420427
let test_cases = [
421428
(TimeUnit::Seconds, 1703980800i64), // 2023-12-30 16:00:00 UTC
@@ -522,4 +529,113 @@ mod tests {
522529
assert!(roundtrip.is_null());
523530
assert_eq!(roundtrip.dtype(), original.dtype());
524531
}
532+
533+
fn timestamp_scalar(unit: TimeUnit, v: i64) -> Scalar {
534+
Scalar::extension::<Timestamp>(
535+
TimestampOptions { unit, tz: None },
536+
Scalar::try_new(
537+
DType::Primitive(PType::I64, Nullability::NonNullable),
538+
Some(ScalarValue::from(v)),
539+
)
540+
.unwrap(),
541+
)
542+
}
543+
544+
#[rstest]
545+
#[case::seconds(TimeUnit::Seconds, 1_372_723_200, DUCKDB_TYPE::DUCKDB_TYPE_TIMESTAMP_S)]
546+
#[case::millis(
547+
TimeUnit::Milliseconds,
548+
1_372_723_200_123,
549+
DUCKDB_TYPE::DUCKDB_TYPE_TIMESTAMP_MS
550+
)]
551+
#[case::micros(
552+
TimeUnit::Microseconds,
553+
1_372_723_200_000_000,
554+
DUCKDB_TYPE::DUCKDB_TYPE_TIMESTAMP
555+
)]
556+
#[case::nanos(
557+
TimeUnit::Nanoseconds,
558+
1_372_723_200_000_000_123,
559+
DUCKDB_TYPE::DUCKDB_TYPE_TIMESTAMP_NS
560+
)]
561+
fn try_from_timestamp_scalar(
562+
#[case] unit: TimeUnit,
563+
#[case] raw: i64,
564+
#[case] expected: DUCKDB_TYPE,
565+
) {
566+
let value = Value::try_from(timestamp_scalar(unit, raw)).unwrap();
567+
assert_eq!(value.logical_type().as_type_id(), expected);
568+
let extracted = match value.extract() {
569+
ExtractedValue::TimestampS(v)
570+
| ExtractedValue::TimestampMs(v)
571+
| ExtractedValue::Timestamp(v)
572+
| ExtractedValue::TimestampNs(v) => v,
573+
other => panic!("unexpected extracted value: {other:?}"),
574+
};
575+
assert_eq!(extracted, raw);
576+
}
577+
578+
#[test]
579+
fn try_from_date_scalar() {
580+
let scalar = Scalar::extension::<Date>(
581+
TimeUnit::Days,
582+
Scalar::try_new(
583+
DType::Primitive(PType::I32, Nullability::NonNullable),
584+
Some(ScalarValue::from(19000i32)),
585+
)
586+
.unwrap(),
587+
);
588+
let value = Value::try_from(scalar).unwrap();
589+
assert_eq!(
590+
value.logical_type().as_type_id(),
591+
DUCKDB_TYPE::DUCKDB_TYPE_DATE
592+
);
593+
let ExtractedValue::Date(days) = value.extract() else {
594+
panic!("expected date");
595+
};
596+
assert_eq!(days, 19000);
597+
}
598+
599+
#[test]
600+
fn try_from_time_scalar() {
601+
let scalar = Scalar::extension::<Time>(
602+
TimeUnit::Microseconds,
603+
Scalar::try_new(
604+
DType::Primitive(PType::I64, Nullability::NonNullable),
605+
Some(ScalarValue::from(42_000_000i64)),
606+
)
607+
.unwrap(),
608+
);
609+
let value = Value::try_from(scalar).unwrap();
610+
assert_eq!(
611+
value.logical_type().as_type_id(),
612+
DUCKDB_TYPE::DUCKDB_TYPE_TIME
613+
);
614+
let ExtractedValue::Time(micros) = value.extract() else {
615+
panic!("expected time");
616+
};
617+
assert_eq!(micros, 42_000_000);
618+
}
619+
620+
#[test]
621+
fn try_from_primitive_i64() {
622+
let value = Value::try_from(Scalar::from(1_372_723_200_000_000i64)).unwrap();
623+
assert_eq!(
624+
value.logical_type().as_type_id(),
625+
DUCKDB_TYPE::DUCKDB_TYPE_BIGINT
626+
);
627+
}
628+
629+
#[test]
630+
fn try_from_null_timestamp() {
631+
let dtype = timestamp_scalar(TimeUnit::Microseconds, 0)
632+
.dtype()
633+
.as_nullable();
634+
let value = Value::try_from(Scalar::null(dtype)).unwrap();
635+
assert_eq!(
636+
value.logical_type().as_type_id(),
637+
DUCKDB_TYPE::DUCKDB_TYPE_TIMESTAMP
638+
);
639+
assert!(matches!(value.extract(), ExtractedValue::Null));
640+
}
525641
}

0 commit comments

Comments
 (0)