Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a checksum field to DataResponseMetadata, and use it for timezone consistency #6046

Merged
merged 12 commits into from
Feb 3, 2025
Merged

Add a checksum field to DataResponseMetadata, and use it for timezone consistency #6046

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
86dd950
checksum
robertbastian Jan 29, 2025
ca21228
typos
robertbastian Jan 29, 2025
e122434
store checksum in data buffer
robertbastian Feb 3, 2025
54b5762
fmt
robertbastian Feb 3, 2025
d9f0496
marker info flag
robertbastian Feb 3, 2025
949b2a8
set flag
robertbastian Feb 3, 2025
c537ff9
use more
robertbastian Feb 3, 2025
38c26c8
Merge branch 'main' into checksum
robertbastian Feb 3, 2025
8d10580
tests
robertbastian Feb 3, 2025
6011b64
\0c
robertbastian Feb 3, 2025
5f2dcc3
todo
robertbastian Feb 3, 2025
64798b6
regen
robertbastian Feb 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
112 changes: 44 additions & 68 deletions components/datetime/src/pattern/names.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2043,22 +2043,6 @@ impl<FSet: DateTimeNamesMarker> RawDateTimeNames<FSet> {
Ok(())
}

fn load_mz_periods<P>(
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I prefer the previous style of separate functions for loading separate keys. One important decision here is that we use Default as the DataRequest, and I don't want to inline that assumption to a bunch of places.

Just keep this function and make it return the DataResponseMetadata

&mut self,
provider: &P,
error_field: ErrorField,
) -> Result<(), PatternLoadError>
where
P: BoundDataProvider<tz::MzPeriodV1Marker> + ?Sized,
{
let variables = ();
self.mz_periods
.load_put(provider, Default::default(), variables)
.map_err(|e| MaybePayloadError::into_load_error(e, error_field))?
.map_err(|e| PatternLoadError::Data(e, error_field))?;
Ok(())
}

pub(crate) fn load_time_zone_generic_long_names(
&mut self,
provider: &(impl BoundDataProvider<tz::MzGenericLongV1Marker> + ?Sized),
Expand All @@ -2077,21 +2061,19 @@ impl<FSet: DateTimeNamesMarker> RawDateTimeNames<FSet> {
id: DataIdentifierBorrowed::for_locale(&locale),
..Default::default()
};
self.mz_generic_long
let cs1 = self
.mz_generic_long
.load_put(provider, req, variables)
.map_err(|e| MaybePayloadError::into_load_error(e, error_field))?
.map_err(|e| PatternLoadError::Data(e, error_field))?;
self.load_mz_periods(mz_period_provider, error_field)?;
#[allow(clippy::unwrap_used)] // we just loaded them
if self
.mz_generic_long
.get()
.inner
.get_option()
.unwrap()
.checksum
!= self.mz_periods.get().inner.get_option().unwrap().checksum
{
.map_err(|e| PatternLoadError::Data(e, error_field))?
.checksum;
let cs2 = self
.mz_periods
.load_put(mz_period_provider, Default::default(), ())
.map_err(|e| MaybePayloadError::into_load_error(e, error_field))?
.map_err(|e| PatternLoadError::Data(e, error_field))?
.checksum;
if cs1.is_none() || cs1 != cs2 {
return Err(PatternLoadError::Data(
DataErrorKind::InconsistentData(tz::MzPeriodV1Marker::INFO)
.with_req(tz::MzGenericLongV1Marker::INFO, req),
Expand Down Expand Up @@ -2119,21 +2101,19 @@ impl<FSet: DateTimeNamesMarker> RawDateTimeNames<FSet> {
id: DataIdentifierBorrowed::for_locale(&locale),
..Default::default()
};
self.mz_generic_short
let cs1 = self
.mz_generic_short
.load_put(provider, req, variables)
.map_err(|e| MaybePayloadError::into_load_error(e, error_field))?
.map_err(|e| PatternLoadError::Data(e, error_field))?;
self.load_mz_periods(mz_period_provider, error_field)?;
#[allow(clippy::unwrap_used)] // we just loaded them
if self
.mz_generic_short
.get()
.inner
.get_option()
.unwrap()
.checksum
!= self.mz_periods.get().inner.get_option().unwrap().checksum
{
.map_err(|e| PatternLoadError::Data(e, error_field))?
.checksum;
let cs2 = self
.mz_periods
.load_put(mz_period_provider, Default::default(), ())
.map_err(|e| MaybePayloadError::into_load_error(e, error_field))?
.map_err(|e| PatternLoadError::Data(e, error_field))?
.checksum;
if cs1.is_none() || cs1 != cs2 {
return Err(PatternLoadError::Data(
DataErrorKind::InconsistentData(tz::MzPeriodV1Marker::INFO)
.with_req(tz::MzGenericShortV1Marker::INFO, req),
Expand Down Expand Up @@ -2161,21 +2141,19 @@ impl<FSet: DateTimeNamesMarker> RawDateTimeNames<FSet> {
id: DataIdentifierBorrowed::for_locale(&locale),
..Default::default()
};
self.mz_specific_long
let cs1 = self
.mz_specific_long
.load_put(provider, req, variables)
.map_err(|e| MaybePayloadError::into_load_error(e, error_field))?
.map_err(|e| PatternLoadError::Data(e, error_field))?;
self.load_mz_periods(mz_period_provider, error_field)?;
#[allow(clippy::unwrap_used)] // we just loaded them
if self
.mz_specific_long
.get()
.inner
.get_option()
.unwrap()
.checksum
!= self.mz_periods.get().inner.get_option().unwrap().checksum
{
.map_err(|e| PatternLoadError::Data(e, error_field))?
.checksum;
let cs2 = self
.mz_periods
.load_put(mz_period_provider, Default::default(), ())
.map_err(|e| MaybePayloadError::into_load_error(e, error_field))?
.map_err(|e| PatternLoadError::Data(e, error_field))?
.checksum;
if cs1.is_none() || cs1 != cs2 {
return Err(PatternLoadError::Data(
DataErrorKind::InconsistentData(tz::MzPeriodV1Marker::INFO)
.with_req(tz::MzSpecificLongV1Marker::INFO, req),
Expand Down Expand Up @@ -2203,21 +2181,19 @@ impl<FSet: DateTimeNamesMarker> RawDateTimeNames<FSet> {
id: DataIdentifierBorrowed::for_locale(&locale),
..Default::default()
};
self.mz_specific_short
let cs1 = self
.mz_specific_short
.load_put(provider, req, variables)
.map_err(|e| MaybePayloadError::into_load_error(e, error_field))?
.map_err(|e| PatternLoadError::Data(e, error_field))?;
self.load_mz_periods(mz_period_provider, error_field)?;
#[allow(clippy::unwrap_used)] // we just loaded them
if self
.mz_specific_short
.get()
.inner
.get_option()
.unwrap()
.checksum
!= self.mz_periods.get().inner.get_option().unwrap().checksum
{
.map_err(|e| PatternLoadError::Data(e, error_field))?
.checksum;
let cs2 = self
.mz_periods
.load_put(mz_period_provider, Default::default(), ())
.map_err(|e| MaybePayloadError::into_load_error(e, error_field))?
.map_err(|e| PatternLoadError::Data(e, error_field))?
.checksum;
if cs1.is_none() || cs1 != cs2 {
return Err(PatternLoadError::Data(
DataErrorKind::InconsistentData(tz::MzPeriodV1Marker::INFO)
.with_req(tz::MzSpecificShortV1Marker::INFO, req),
Expand Down
45 changes: 28 additions & 17 deletions components/datetime/src/provider/time_zones.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,26 +153,31 @@ pub struct ExemplarCitiesV1<'data> {
/// An ICU4X mapping to generic metazone names.
/// See CLDR-JSON timeZoneNames.json for more context.
///
/// These markers use a checksum to ensure consistency with [`MetazonePeriodV1`].
///
/// <div class="stab unstable">
/// 🚧 This code is considered unstable; it may change at any time, in breaking or non-breaking ways,
/// including in SemVer minor releases. While the serde representation of data structs is guaranteed
/// to be stable, their Rust representation might not be. Use with caution.
/// </div>
#[icu_provider::data_struct(
marker(MetazoneGenericNamesLongV1Marker, "time_zone/generic_long@1"),
marker(MetazoneGenericNamesShortV1Marker, "time_zone/generic_short@1")
marker(
MetazoneGenericNamesLongV1Marker,
"time_zone/generic_long@1",
has_checksum
),
marker(
MetazoneGenericNamesShortV1Marker,
"time_zone/generic_short@1",
has_checksum
)
)]
#[derive(PartialEq, Debug, Clone, Default)]
#[cfg_attr(feature = "datagen", derive(serde::Serialize, databake::Bake))]
#[cfg_attr(feature = "datagen", databake(path = icu_datetime::provider::time_zones))]
#[cfg_attr(feature = "serde", derive(serde::Deserialize))]
#[yoke(prove_covariance_manually)]
pub struct MetazoneGenericNamesV1<'data> {
/// An XxHash64 checksum of the full metazone names.
///
/// The checksum here should match the checksum in [`MetazonePeriodV1`]
/// if these were generated from the same data set.
pub checksum: u64,
/// The default mapping between metazone id and localized metazone name.
#[cfg_attr(feature = "serde", serde(borrow))]
pub defaults: ZeroMap<'data, MetazoneId, str>,
Expand All @@ -185,26 +190,31 @@ pub struct MetazoneGenericNamesV1<'data> {
/// Specific names include time variants such as "daylight."
/// See CLDR-JSON timeZoneNames.json for more context.
///
/// These markers use a checksum to ensure consistency with [`MetazonePeriodV1`].
///
/// <div class="stab unstable">
/// 🚧 This code is considered unstable; it may change at any time, in breaking or non-breaking ways,
/// including in SemVer minor releases. While the serde representation of data structs is guaranteed
/// to be stable, their Rust representation might not be. Use with caution.
/// </div>
#[icu_provider::data_struct(
marker(MetazoneSpecificNamesLongV1Marker, "time_zone/specific_long@1"),
marker(MetazoneSpecificNamesShortV1Marker, "time_zone/specific_short@1")
marker(
MetazoneSpecificNamesLongV1Marker,
"time_zone/specific_long@1",
has_checksum
),
marker(
MetazoneSpecificNamesShortV1Marker,
"time_zone/specific_short@1",
has_checksum
)
)]
#[derive(PartialEq, Debug, Clone, Default)]
#[cfg_attr(feature = "datagen", derive(serde::Serialize, databake::Bake))]
#[cfg_attr(feature = "datagen", databake(path = icu_datetime::provider::time_zones))]
#[cfg_attr(feature = "serde", derive(serde::Deserialize))]
#[yoke(prove_covariance_manually)]
pub struct MetazoneSpecificNamesV1<'data> {
/// An XxHash64 checksum of the full metazone names.
///
/// The checksum here should match the checksum in [`MetazonePeriodV1`]
/// if these were generated from the same data set.
pub checksum: u64,
/// The default mapping between metazone id and localized metazone name.
#[cfg_attr(feature = "serde", serde(borrow))]
pub defaults: ZeroMap<'data, (MetazoneId, ZoneVariant), str>,
Expand All @@ -225,6 +235,8 @@ pub type MetazoneId = core::num::NonZeroU8;
/// An ICU4X mapping to the metazones at a given period.
/// See CLDR-JSON metaZones.json for more context.
///
/// This markers uses a checksum to ensure consistency with `Metazone*NamesV1`.
///
/// <div class="stab unstable">
/// 🚧 This code is considered unstable; it may change at any time, in breaking or non-breaking ways,
/// including in SemVer minor releases. While the serde representation of data structs is guaranteed
Expand All @@ -233,7 +245,8 @@ pub type MetazoneId = core::num::NonZeroU8;
#[icu_provider::data_struct(marker(
MetazonePeriodV1Marker,
"time_zone/metazone_period@1",
singleton
singleton,
has_checksum
))]
#[derive(PartialEq, Debug, Clone, Default)]
#[cfg_attr(feature = "datagen", derive(serde::Serialize, databake::Bake))]
Expand All @@ -245,6 +258,4 @@ pub struct MetazonePeriodV1<'data> {
/// the number of minutes since the local [`EPOCH`](icu_timezone::provider::EPOCH). It represents when the metazone started to be used.
#[cfg_attr(feature = "serde", serde(borrow))]
pub list: ZeroMap2d<'data, TimeZoneBcp47Id, IsoMinutesSinceEpoch, NichedOption<MetazoneId, 1>>,
/// An XxHash64 checksum of the full metazone names.
pub checksum: u64,
}
11 changes: 6 additions & 5 deletions components/datetime/src/scaffold/names_storage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ pub trait MaybePayload<M: DynamicDataMarker, Variables>: UnstableSealed {
provider: &P,
req: DataRequest,
variables: Variables,
) -> Result<Result<(), DataError>, MaybePayloadError>
) -> Result<Result<DataResponseMetadata, DataError>, MaybePayloadError>
where
P: BoundDataProvider<M> + ?Sized,
Self: Sized;
Expand Down Expand Up @@ -199,15 +199,16 @@ where
provider: &P,
req: DataRequest,
variables: Variables,
) -> Result<Result<(), DataError>, MaybePayloadError>
) -> Result<Result<DataResponseMetadata, DataError>, MaybePayloadError>
where
P: BoundDataProvider<M> + ?Sized,
Self: Sized,
{
let arg_variables = variables;
match &self.inner {
OptionalNames::SingleLength { variables, .. } if arg_variables == *variables => {
return Ok(Ok(()));
// TODO(#6063): probably not correct
return Ok(Ok(Default::default()));
Copy link
Member

Choose a reason for hiding this comment

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

Oof, is this ok? Or should we retain the DataResponseMetadata?

}
OptionalNames::SingleLength { .. } => {
return Err(MaybePayloadError::ConflictingField);
Expand All @@ -220,7 +221,7 @@ where
payload: response.payload,
variables: arg_variables,
};
Ok(Ok(()))
Ok(Ok(response.metadata))
}
Err(e) => Ok(Err(e)),
}
Expand All @@ -242,7 +243,7 @@ impl<M: DynamicDataMarker, Variables> MaybePayload<M, Variables> for () {
_: &P,
_: DataRequest,
_: Variables,
) -> Result<Result<(), DataError>, MaybePayloadError>
) -> Result<Result<DataResponseMetadata, DataError>, MaybePayloadError>
where
P: BoundDataProvider<M> + ?Sized,
Self: Sized,
Expand Down
Loading