adapter: fix TODO, add Timestamp::from_duration

[ParkMyCar]

Motivation

This PR fixes a TODO in the adapter crate that I noticed when reading through the code base:

// TODO: We can have only two consts here, instead of three,
// once there exists a const way to convert between a Timestamp
// and a Duration

Specifically it adds a new const fn from_duration(duration: Duration) on a mz_repr::Timestamp.

An issue with the implementation though is that Duration::as_millis returns a u128, meanwhile, internally Timestamp uses a u64. So it's possible that a large enough Duration would overflow what could fit in Timestamp. To support const contexts we can't return a Result<...> but we can panic (as of Rust 1.57). So I opted to check for overflow and panic if necessary.

Checklist

• This PR has adequate test coverage / QA involvement has been duly considered.
• This PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way) and therefore is tagged with a T-proto label.
• If this PR will require changes to cloud orchestration, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
• This PR includes the following user-facing behavior changes:
  • N/a

[umanwizard]

LGTM except for the Clippy issue.

Thanks for the contribution!

[umanwizard]

You will need #[allow(clippy::as_conversions)] here in order to pass CI.

Please leave a note that we are tolerating the use of as here (rather than something less error-prone like try_from followed by expect) in order to allow this function to be constant.

[jkosh44]

Thanks for the PR!

Our linter will likely complain here about using as, since we prefer to avoid as.

I think we should add a const version of try_cast_from that panics when the cast fails instead of returning None, here:

materialize/src/ore/src/cast.rs

Lines 194 to 212 in efbc8df

	/// Implement `TryCastFrom` for the specified types.
	/// This is only necessary for types for which `as` exists,
	/// but `TryFrom` doesn't (notably floats).
	macro_rules! try_cast_from {
	($from:ty, $to:ty) => {
	impl crate::cast::TryCastFrom<$from> for $to {
	#[allow(clippy::as_conversions)]
	fn try_cast_from(from: $from) -> Option<$to> {
	let to = from as $to;
	let inverse = to as $from;
	if from == inverse {
	Some(to)
	} else {
	None
	}
	}
	}
	};
	}

It would likely be very similar to the non-fallible version here:

materialize/src/ore/src/cast.rs

Lines 37 to 57 in efbc8df

	macro_rules! cast_from {
	($from:ty, $to:ty) => {
	paste::paste! {
	impl crate::cast::CastFrom<$from> for $to {
	#[allow(clippy::as_conversions)]
	fn cast_from(from: $from) -> $to {
	from as $to
	}
	}
	/// Casts [`$from`] to [`$to`].
	///
	/// This is equivalent to the [`crate::cast::CastFrom`] implementation but is
	/// available as a `const fn`.
	#[allow(clippy::as_conversions)]
	pub const fn [< $from _to_ $to >](from: $from) -> $to {
	from as $to
	}
	}
	};
	}

[ParkMyCar]

FWIW I'm not sure a macro here would work, since casting a non-primitive type to another non-primitive, probably would require different implementations? What are your thoughts?

[jkosh44]

We'll let @mjibson and @umanwizard determine if it's worth implementing this since they are more familiar with this code. However, if we go forward then I was thinking that the macro would implement a const conversion from u128 to u64. Then you can call the generated const function in from_duration instead of using as.

[maddyblue]

That'd still panic if the conversion fails right? That's my hangup.

[jkosh44]

I think it will still panic if conversion fails, but only at compile time. So the const conversions between primitives would be useful if we ever need to do a fallible conversion at compile time, that we know will not fail.

[maddyblue]

Fine with me if it fails at compile time.

[maddyblue]

Wait but could we enforce that it's never called for non-const contexts? I don't want one of us to later call it for user-supplied Durations.

[jkosh44]

There may be a lint, but out of the box it is possible to call this function outside of a const context, in which case the panic will happen at runtime. For example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=7dd5c8b6c45b54a692144fb6dbad2a70

[jkosh44]

Here's an example of panicking at compile time: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c87810ed6dffe850d8377e7cf98b7b03.

I don't have a strong opinion on whether adding the const function is worth it or not, and will defer to you and @umanwizard, since you too added this code.

[maddyblue]

We already implement try_from for Duration because it can fail:

materialize/src/repr/src/timestamp.rs

Line 307 in edb241f

impl TryFrom<Duration> for Timestamp {

I'm more inclined to just remove the TODO and not change any code?

[ParkMyCar]

Thanks for the review @umanwizard, @jkosh44, and @mjibson! I'll do whatever y'all would like :)

[maddyblue]

Unless someone comes up with a very compelling argument why we should have code that panics during conversion when we have an existing method that produces an error instead, I'm inclined to request that this PR gets changed to only remove the TODO, and nothing else. I do not think that TODO will ever be possible, so let's remove it.

[umanwizard]

I do not think that TODO will ever be possible

It will be possible without adding anything to Timestamp if rust-lang/rust#87852 is ever stabilized, which is why I wrote that TODO.

[ParkMyCar]

Totally understand that panicking is not desirable! I changed the PR to just update the comment describing what currently unstable features would allow unwrapping a result at compile time, which would allow an API that can be checked and handled a runtime, or cause a build failure at compile time.

• const_option allows unwrapping an Option in const contexts
• const_result allows unwrapping a Result in const contexts
• As @umanwizard pointed out const_num_from_num tracks the const-ification of TryFrom for numeric types
• const_precise_live_drops would allow writing something like enum CastResult and implementing our own const fn unwrap() method

[umanwizard]

Thanks, @ParkMyCar . Sorry for all the churn!

I think we can let @mjibson own giving final approval.

[ParkMyCar]

@umanwizard not a problem! it's always the seemingly small changes that get ya :)