Implement without_descendants and without_ancestors in datagen #4846

sffc · 2024-04-25T00:43:42Z

Part of #4629

I rewrote select_locales_for_key and I think it is a lot cleaner and comprehensible now.

…region fallback

sffc · 2024-04-25T02:09:11Z

provider/blob/src/export/mod.rs

@@ -23,7 +23,7 @@
 //! DatagenDriver::new()
 //!     .with_keys([icu_provider::hello_world::HelloWorldV1Marker::KEY])
 //!     .with_all_locales()
-//!     .export(&DatagenProvider::new_latest_tested(), exporter)
+//!     .export(&icu_provider::hello_world::HelloWorldProvider, exporter)


This change doesn't impact behavior; it just makes it so that this crate doesn't need to depend on the provider feature of icu_datagen

provider/blob/tests/test_versions.rs

sffc · 2024-04-25T02:11:51Z

provider/baked/collator/data/macros/collator_meta_v1.rs.data

-                static VALUES: [&<icu::collator::provider::CollationMetadataV1Marker as icu_provider::DataMarker>::Yokeable; 87usize] = [&AF, &AM, &AR, &AR, &AR, &AR, &AM, &AR, &AF, &AR, &AF, &AM, &AF, &AF, &DA, &AF, &AF, &AM, &AF, &AF, &AF, &AF, &AR, &AF, &AF, &AF, &AF, &FR_CA, &AF, &AF, &AR, &AF, &AM, &AR, &AR, &AF, &AF, &AR, &AF, &AF, &AR, &AM, &AR, &AR, &AR, &AR, &AR, &AR, &AR, &AM, &LT, &AF, &AR, &AR, &AM, &AR, &DA, &AR, &AM, &AF, &AR, &AR, &AF, &AR, &AF, &AM, &AR, &AF, &AF, &AF, &AR, &AF, &AR, &AR, &TH, &AF, &AF, &AF, &AR, &AR, &UND, &AR, &AF, &VI, &AF, &AF, &AR];
-                static KEYS: [&str; 87usize] = ["af", "am", "ar", "as", "az", "be", "bg", "bn", "br", "bs", "ceb", "chr", "cs", "cy", "da", "de-u-co-phonebk", "dsb", "el", "en-US-posix", "eo", "es", "et", "fa", "ff-Adlm", "fi", "fil", "fo", "fr-CA", "fy", "gl", "gu", "ha", "he", "hi", "hr", "hsb", "hu", "hy", "ig", "is", "ja", "ka", "kk", "km", "kn", "ko", "kok", "ku", "ky", "lo", "lt", "lv", "mk", "ml", "mn", "mr", "mt", "my", "ne", "no", "or", "pa", "pl", "ps", "ro", "ru", "si", "sk", "sl", "sq", "sr", "sv", "ta", "te", "th", "tk", "to", "tr", "ug", "uk", "und", "ur", "uz", "vi", "wo", "yo", "zh"];
+                static VALUES: [&<icu::collator::provider::CollationMetadataV1Marker as icu_provider::DataMarker>::Yokeable; 88usize] = [&AF, &AM, &AR, &AR, &AR, &AR, &AM, &AR, &AF, &AR, &AF, &AM, &AF, &AF, &DA, &AF, &AF, &AF, &AM, &AF, &AF, &AF, &AF, &AR, &AF, &AF, &AF, &AF, &FR_CA, &AF, &AF, &AR, &AF, &AM, &AR, &AR, &AF, &AF, &AR, &AF, &AF, &AR, &AM, &AR, &AR, &AR, &AR, &AR, &AR, &AR, &AM, &LT, &AF, &AR, &AR, &AM, &AR, &DA, &AR, &AM, &AF, &AR, &AR, &AF, &AR, &AF, &AM, &AR, &AF, &AF, &AF, &AR, &AF, &AR, &AR, &TH, &AF, &AF, &AF, &AR, &AR, &UND, &AR, &AF, &VI, &AF, &AF, &AR];
+                static KEYS: [&str; 88usize] = ["af", "am", "ar", "as", "az", "be", "bg", "bn", "br", "bs", "ceb", "chr", "cs", "cy", "da", "de-AT-u-co-phonebk", "de-u-co-phonebk", "dsb", "el", "en-US-posix", "eo", "es", "et", "fa", "ff-Adlm", "fi", "fil", "fo", "fr-CA", "fy", "gl", "gu", "ha", "he", "hi", "hr", "hsb", "hu", "hy", "ig", "is", "ja", "ka", "kk", "km", "kn", "ko", "kok", "ku", "ky", "lo", "lt", "lv", "mk", "ml", "mn", "mr", "mt", "my", "ne", "no", "or", "pa", "pl", "ps", "ro", "ru", "si", "sk", "sl", "sq", "sr", "sv", "ta", "te", "th", "tk", "to", "tr", "ug", "uk", "und", "ur", "uz", "vi", "wo", "yo", "zh"];


This is the only behavioral change from this PR. de-AT-u-co-phonebk becomes a selected locale because we pull extensions up into all supported locales, not just explicit locales.

This does reveal an interesting attribute of the deduplicator behavior: de-AT-u-co-phonebk is retained because it inherits from de-AT, which has different data, before it inherits from de-u-co-phonebk. However, de-AT is deduplicated because it inherits from de. This means that we could deduplicate de-AT-u-co-phonebk in a second pass. 🤔

sffc · 2024-04-25T04:44:04Z

provider/datagen/src/driver.rs

+
+    // Fill in missing extensions and aux keys from parent locales,
+    // and calculate which langids are ancestors and descendants.
+    for current_langid in locales_map.keys().cloned().collect::<Vec<_>>() {


There's a lot of cloning going on here because otherwise I would need multiple mutable borrows of the HashMap at the same time.

It might be clearer to build a new one instead of mutating

robertbastian

Algorithm looks good, just would like a bit less mutation going on.

provider/blob/tests/test_versions.rs

robertbastian · 2024-04-29T15:38:05Z

provider/datagen/src/driver.rs

        Self {
            langid: Some(langid),
            include_ancestors: true,
            include_descendants: false,
        }
    }

+    /// The family containing all descendants of the selected locale.
+    ///
+    /// This family is primarily useful if the root locale is not desired.


I wouldn't say it's the primary use case, your example is another (more general) one.

robertbastian · 2024-04-29T15:40:53Z

provider/datagen/src/driver.rs

+        is_selected: bool,
+        data_locales: HashSet<DataLocale>,
+    }
+    let mut locales_map: HashMap<LanguageIdentifier, LocalesMapValue> = Default::default();


nit: writing this as

Suggested change

let mut locales_map: HashMap<LanguageIdentifier, LocalesMapValue> = Default::default();

let mut locales_map = HashMap::<LanguageIdentifier, LocalesMapValue>::default();

makes the code more maintainable, as that expression is context-independent.

robertbastian · 2024-04-29T15:47:03Z

provider/datagen/src/driver.rs

            }
-            if explicit.contains(&locale) {
-                return true;
+            for family in families {


nit: put this all in an else, as it doesn't happen when families is empty

robertbastian · 2024-04-29T15:49:32Z

provider/datagen/src/driver.rs

+            if families.is_empty() {
+                // If no locales are selected but fallback is enabled, select the root locale
+                let value = locales_map.entry(LanguageIdentifier::UND).or_default();
+                value.is_selected = true;


not big fan of keeping state in the locales_map, would prefer building a new collection

Maybe, but the code was already mutating the map in locales_map.retain; I'm just mutating it a bit more. I don't really see the point in re-building the structure. When there aren't aux keys or extensions at play, expensive mutations are not necessary, so we'd basically just end up cloning the map a few times.

For this code I would rather optimize for readability than for performance. Even with the mutation you're doing a lot of cloning. It's unclear to me as a reader how all the mutations interact, intermediate immutable steps would help here.

robertbastian · 2024-04-29T15:58:09Z

provider/datagen/src/driver.rs

+
+    // Fill in missing extensions and aux keys from parent locales,
+    // and calculate which langids are ancestors and descendants.
+    for current_langid in locales_map.keys().cloned().collect::<Vec<_>>() {


It might be clearer to build a new one instead of mutating

robertbastian · 2024-04-29T16:05:09Z

provider/datagen/src/driver.rs

+            .map(|family| family.include_ancestors)
+            .unwrap_or(false);


Is is really worth, performance wise, to have family be an optional borrow, instead of just creating the families for all LanguageIdentifiers? This false is the same as single's, but here it's just a false literal.

Same below for include_descendants.

The presence of the family is how I indicate that the langid was explicit. If it is None, the langid is supported but not explicit.

sffc · 2024-04-30T00:46:29Z

I did a bunch more refactoring to reduce the amount of mutation. I agree it makes it easier to follow. Hope you like it :)

provider/datagen/src/driver.rs

Co-authored-by: Robert Bastian <[email protected]>

robertbastian · 2024-04-30T16:55:44Z

provider/datagen/src/driver.rs

-            LocalesWithOrWithoutFallback::WithoutFallback { langids: locales } => {
-                let mut sorted_locales = locales
+            LocalesWithOrWithoutFallback::WithoutFallback { langids } => {
+                let mut sorted_locales = langids


sorted_langids?

sffc added 13 commits April 24, 2024 10:27

Start adding without_ancestors (unicode-org#4629)

c18bb78

Mostly working

5c5b990

Add test for without_ancestors

8cb704c

fmt

1c99e75

clippy

c0d9149

Add collation special case for backwards compatibility

9e76071

Special case for --locales full; fixes

78c923b

Generalize special case (also impacts transliterator)

a36b095

Return to the old way of handling --locales full: needs to work with …

4b2362e

…region fallback

Always include variants of und for region fallback

fc27d6c

Inline langid_families

ad6f135

de-AT-u-co-phonebk is now included to prevent fallback to de-AT

ac0b97d

fmt/rename

5eb0f78

sffc marked this pull request as ready for review April 25, 2024 02:08

sffc requested review from robertbastian, Manishearth and a team as code owners April 25, 2024 02:08

sffc removed request for a team and Manishearth April 25, 2024 02:08

Undo addition of either dep

0600a81

sffc commented Apr 25, 2024

View reviewed changes

robertbastian reviewed Apr 29, 2024

View reviewed changes

sffc added 6 commits April 29, 2024 13:24

Merge branch 'main' into without-ancestors

dd78d71

Define precedence ordering for LocaleFamily; refactor to support it

d980270

Switch data structures to use less mutation

8d18020

Even less mutation

04b22d0

Even less mutation

23810a7

docs, clippy

f955ef1

sffc requested a review from robertbastian April 30, 2024 00:45

sffc added 4 commits April 29, 2024 17:49

Cleanup

5a4b115

Fix

2960378

Clippy

85380a0

Fix

48a1355

robertbastian previously approved these changes Apr 30, 2024

View reviewed changes

Update provider/datagen/src/driver.rs

3f94341

Co-authored-by: Robert Bastian <[email protected]>

sffc dismissed robertbastian’s stale review via 3f94341 April 30, 2024 16:22

sffc added 4 commits April 30, 2024 09:23

inner -> parts

c7b7924

langids

7a6ed81

comment about false

bddaa9b

Deduplicate candidate langids

2b9638b

sffc requested a review from robertbastian April 30, 2024 16:28

robertbastian approved these changes Apr 30, 2024

View reviewed changes

sffc merged commit 006e2c2 into unicode-org:main Apr 30, 2024
30 checks passed

sffc deleted the without-ancestors branch April 30, 2024 23:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement without_descendants and without_ancestors in datagen #4846

Implement without_descendants and without_ancestors in datagen #4846

sffc commented Apr 25, 2024 •

edited

Loading

sffc Apr 25, 2024

sffc Apr 25, 2024 •

edited

Loading

sffc Apr 25, 2024

sffc Apr 25, 2024

robertbastian Apr 29, 2024

robertbastian left a comment

robertbastian Apr 29, 2024

robertbastian Apr 29, 2024

robertbastian Apr 29, 2024

robertbastian Apr 29, 2024

sffc Apr 29, 2024

robertbastian Apr 29, 2024

robertbastian Apr 29, 2024

robertbastian Apr 29, 2024

sffc Apr 29, 2024

sffc commented Apr 30, 2024

robertbastian Apr 30, 2024

	let mut locales_map: HashMap<LanguageIdentifier, LocalesMapValue> = Default::default();
	let mut locales_map = HashMap::<LanguageIdentifier, LocalesMapValue>::default();

Implement without_descendants and without_ancestors in datagen #4846

Implement without_descendants and without_ancestors in datagen #4846

Conversation

sffc commented Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

sffc Apr 25, 2024 • 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

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

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 commented Apr 30, 2024

Choose a reason for hiding this comment

sffc commented Apr 25, 2024 •

edited

Loading

sffc Apr 25, 2024 •

edited

Loading