Implement data struct `PluralRangesV1` #4077

jedel1043 · 2023-09-22T10:30:57Z

Related to #3012.

~~This is still missing some details and documentation, but I wanted to put it out for you to evaluate the data model.~~ Should be feature-complete now.

One of the biggest space optimizations was obviating the need to store ranges where the resulting category of the range is the same as the end category, since TR35 specifies:

If there is no value for a <start,end> pair, the default result is end.

components/plurals/src/lib.rs

sffc

Praise: Nice job!

provider/datagen/src/transform/cldr/cldr_serde/plural_ranges.rs

sffc · 2023-09-22T17:21:34Z

provider/datagen/src/transform/cldr/cldr_serde/plural_ranges.rs

+    pub end: String,
+}
+
+impl<'de> Deserialize<'de> for PluralRange {


Praise: Really nice use of a custom Deserialize impl

components/plurals/src/provider.rs

sffc · 2023-09-22T18:46:02Z

components/plurals/src/lib.rs

@@ -155,7 +161,7 @@ pub enum PluralCategory {
    ///
    /// - 0 in Arabic (ar), Latvian (lv)
    /// - 10~20, 30, 40, 50, ... in Latvian (lv)
-    Zero,
+    Zero = 0,


Thought: For dataful enums I wonder if in general we should reserve 0 for the value "null"; or, in the case of the PluralCategory enum, null is "other" so should "other" be 0?

Thought about it, but I didn't know if reordering the variants would be a breaking change or not (because of the PartialOrd derive), so I opted for following the specified order.

Though, we could also implement PartialOrd by hand, but it'll be really weird to have Zero < Other and Zero as u8 > Other as u8

OK, I see, this enum has derive(Ord) which somewhat limits our options

My fully detailed thoughts: I think what you did is fine, but I don't see the harm in making a new enum whose purpose is serialization. We then leave this enum as the public-facing Rust enum. Another thing to consider is, the keys in the ZeroMap do not need to be validated; if you have an out-of-range plural category, you just won't ever find that entry in the map, so we can save a little bit of deserialization code and speed by using a plain u8 as the key. However, that key type does not serialize nicely in JSON, so what we've done in other places is make yet another enum which is a newtype wrapper over the primitive but has nice serialization (usually called UnvalidatedX: see UnvalidatedTinyAsciiStr, UnvalidatedStr, etc). However, the nice part about your proposed solution is that it is the least amount of additional boilerplate code. It just might be tying too many functional use cases together.

Ok, then it would be:

A new RawPluralCategory enum inside provider with RawPluralCategory::Other = 0 and a conversion from PluralCategory to RawPluralCategory. This will be the new value type of the map.

A new UnvalidatedPluralCategory(u8) inside provider that de/serializes from/to a string if the de/serializer is human readable, which will be the key type of the map. Also a conversion from RawPluralCategory to UnvalidatedPluralCategory.

Is that correct?

Also, should both types be #[doc(hidden]? Or should I just document them as unstable?

Yes, naming aside, that's basically the other model for this.

They should be public in the provider module with the yellow box warning people that the APIs may be unstable.

I would make the second one pub struct UnvalidatedPluralCategory(pub u8) that is a u8 for all purposes except that is_human_readable impl Serialize makes a nice string, returning an error if out of range, and is_human_readable impl Deserialize knows how to handle the string.

However I'd like to see what @robertbastian or @Manishearth think about these two approaches (re-use the existing enum and make it ZeroVec compatible, or introduce two new types which are data provider specific)

Yes, naming aside, that's basically the other model for this.

They should be public in the provider module with the yellow box warning people that the APIs may be unstable.

I would make the second one pub struct UnvalidatedPluralCategory(pub u8) that is a u8 for all purposes except that is_human_readable impl Serialize makes a nice string, returning an error if out of range, and is_human_readable impl Deserialize knows how to handle the string.

However I'd like to see what @robertbastian or @Manishearth think about these two approaches (re-use the existing enum and make it ZeroVec compatible, or introduce two new types which are data provider specific)

Sounds good! I've already finished implementing the API in another branch, so I'll wait for your consensus in the meantime.

I agree that a new type is the best solution.

components/plurals/src/provider.rs

jedel1043 · 2023-09-26T07:17:23Z

Applied the review. Let me know if those names are okay.

robertbastian · 2023-09-26T07:53:35Z

components/plurals/src/provider.rs

+    /// - `key1` corresponds to the end category of the range.
+    #[cfg_attr(feature = "serde", serde(borrow))]
+    pub ranges:
+        ZeroMap2d<'data, UnvalidatedPluralCategory, UnvalidatedPluralCategory, RawPluralCategory>,


thought: ZeroMap2d might be overkill. I think we don't need functionality to iterate over second-order keys, and we can fit both keys in a single byte.

Got it, I'll use a simple ZeroMap then.

Replaced it with a ZeroMap. It makes the human-readable de/serialization slightly worse but non-human-readable de/serialization is a lot smaller and faster.

robertbastian

Waiting for @sffc to take a look before merging.

sffc · 2023-10-02T02:47:58Z

components/plurals/src/provider.rs

+                .ok_or_else(|| S::Error::custom("invalid tag in UnvalidatedPluralRange"))?;
+            let end = RawPluralCategory::new_from_u8(self.0 & 0x0F)
+                .ok_or_else(|| S::Error::custom("invalid tag in UnvalidatedPluralRange"))?;
+            (start, end).serialize(serializer)


Thought: if we wanted to make this human-readable we could serialize to a separated string like "One/Other", but I think what you have here is good

Should be a pretty quick fix. Maybe something like One-Other to use a range-like syntax?

Yeah I like that!

(This is a totally optional suggestion; this is mergeable as-is)

sffc · 2023-10-02T02:48:14Z

components/plurals/src/provider.rs

+    databake(path = icu_plurals::provider),
+)]
+#[zerovec::make_ule(UnvalidatedPluralRangeULE)]
+pub struct UnvalidatedPluralRange(pub u8);


Praise: so compact!

components/plurals/src/provider.rs

sffc

lgtm!

jedel1043 force-pushed the plural-ranges-ds branch from 36763de to cbaa89e Compare September 22, 2023 18:05

jedel1043 commented Sep 22, 2023

View reviewed changes

components/plurals/src/lib.rs Outdated Show resolved Hide resolved

jedel1043 marked this pull request as ready for review September 22, 2023 18:28

jedel1043 requested review from a team, sffc, robertbastian, Manishearth and zbraniecki as code owners September 22, 2023 18:28

sffc reviewed Sep 22, 2023

View reviewed changes

jedel1043 requested a review from sffc September 22, 2023 20:30

jedel1043 force-pushed the plural-ranges-ds branch from 3b9734c to 2ac056b Compare September 26, 2023 06:45

robertbastian previously approved these changes Sep 26, 2023

View reviewed changes

jedel1043 dismissed robertbastian’s stale review via 7d74cae September 26, 2023 18:15

robertbastian previously approved these changes Sep 26, 2023

View reviewed changes

sffc previously approved these changes Oct 2, 2023

View reviewed changes

jedel1043 added 8 commits October 1, 2023 20:53

Deserialize pluralRanges.json

4308949

Implement data struct PluralRanges

ea0f27c

Remove as_cldr_str

6b0e5e0

Document and clippify

139a532

Apply review

0069b03

Substitute PluralCategory with provider-exclusive types

f98d41d

Replace 2d map with flat map

61ecea0

Prettify human-readable serialization

b1e132a

jedel1043 dismissed stale reviews from sffc and robertbastian via b1e132a October 2, 2023 04:59

jedel1043 force-pushed the plural-ranges-ds branch from 7d74cae to b1e132a Compare October 2, 2023 04:59

sffc reviewed Oct 2, 2023

View reviewed changes

components/plurals/src/provider.rs Show resolved Hide resolved

components/plurals/src/provider.rs Outdated Show resolved Hide resolved

Use CLDR string representation

53831d6

jedel1043 requested a review from sffc October 2, 2023 05:45

jedel1043 requested a review from robertbastian October 5, 2023 00:28

robertbastian approved these changes Oct 5, 2023

View reviewed changes

robertbastian merged commit d2669e1 into unicode-org:main Oct 5, 2023

jedel1043 deleted the plural-ranges-ds branch October 5, 2023 14:46

sffc reviewed Oct 5, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement data struct `PluralRangesV1` #4077

Implement data struct `PluralRangesV1` #4077

jedel1043 commented Sep 22, 2023 •

edited

Loading

sffc left a comment

sffc Sep 22, 2023

sffc Sep 22, 2023

jedel1043 Sep 22, 2023

jedel1043 Sep 22, 2023 •

edited

Loading

sffc Sep 22, 2023

jedel1043 Sep 22, 2023 •

edited

Loading

sffc Sep 23, 2023 •

edited

Loading

jedel1043 Sep 25, 2023

robertbastian Sep 26, 2023

Manishearth Sep 26, 2023

jedel1043 commented Sep 26, 2023

robertbastian Sep 26, 2023

jedel1043 Sep 26, 2023

jedel1043 Sep 26, 2023 •

edited

Loading

robertbastian left a comment

sffc Oct 2, 2023

jedel1043 Oct 2, 2023

sffc Oct 2, 2023

sffc Oct 2, 2023

sffc left a comment

Implement data struct PluralRangesV1 #4077

Implement data struct PluralRangesV1 #4077

Conversation

jedel1043 commented Sep 22, 2023 • edited Loading

sffc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedel1043 Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedel1043 Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

sffc Sep 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedel1043 commented Sep 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedel1043 Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

robertbastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc left a comment

Choose a reason for hiding this comment

Implement data struct `PluralRangesV1` #4077

Implement data struct `PluralRangesV1` #4077

jedel1043 commented Sep 22, 2023 •

edited

Loading

jedel1043 Sep 22, 2023 •

edited

Loading

jedel1043 Sep 22, 2023 •

edited

Loading

sffc Sep 23, 2023 •

edited

Loading

jedel1043 Sep 26, 2023 •

edited

Loading