Moving `AuxiliaryKeys` to `DataRequest` as `DataKeyAttributes` #4981

robertbastian · 2024-05-31T15:05:16Z

sffc

I reviewed the changes in icu_provider, icu_provider_blob, and parts of icu_datagen. I assume the rest of the changes are mechanical.

Praise: nice work! Thanks for keeping the output data the same in order to make this a more standalone change.

provider/core/src/request.rs

sffc · 2024-05-31T18:33:06Z

provider/core/src/request.rs

+/// TODO
+#[derive(Clone, Default)]
+pub struct DataKeyAttributes {


TODO (here and elsewhere)

provider/core/src/marker.rs

provider/core/src/datagen/mod.rs

sffc · 2024-05-31T18:54:11Z

provider/core/src/datagen/iter.rs

+        key_attributes: &DataKeyAttributes,
+    ) -> Result<bool, DataError> {
+        self.supported_requests()
+            .map(|v| v.contains(&(locale.clone(), key_attributes.clone())))


Thought/Issue: This is a hot path and it's a pity you need to clone; there should be a way to do the lookup without cloning.

yes there should be...

Could work with HashMap<DataKeyAttributes, HashSet<DataLocale>> instead?

There's some discussion of options in https://stackoverflow.com/questions/45786717/how-to-implement-hashmap-with-two-keys

Doubly-nested HashMap is interesting, but that's a helluva lot of hashmaps...

Or, since you need this tuple everywhere, maybe just export a helper struct called LocaleWithAttributes (a standalone helper used in IterableDataProvider but not DataRequest) with all the impls you need.

Can I do this in a follow-up? I don't want this to get stale

Follow-up is okay

provider/blob/src/export/blob_exporter.rs

provider/blob/src/blob_schema.rs

robertbastian · 2024-05-31T19:24:22Z

Please also look at icu_datetime, and have a really close look at icu_datagen. I cannot figure out why the locale/aux set for datetime keys is changing.

components/experimental/src/transliterate/compile/mod.rs

components/experimental/tests/transliterate/data/baked/macros.rs

components/experimental/tests/transliterate/data/baked/macros/normalizer_comp_v1.rs.data

provider/datagen/src/transform/cldr/datetime/neo.rs

provider/datagen/src/transform/cldr/datetime/neo_skeleton.rs

sffc · 2024-05-31T20:44:19Z

provider/fs/src/export/fs_exporter.rs

+        write!(&mut path_buf, "/{key}").expect("infallible");
+        write!(&mut path_buf, "/{locale}").expect("infallible");
+        if !key_attributes.is_empty() {
+            write!(&mut path_buf, "-x-{}", key_attributes as &str).expect("infallible");
+        }
+        write!(&mut path_buf, ".{}", self.manifest.file_extension).expect("infallible");


Thought for later: we probably want to have fs provider create another directory layer instead of using -x-

yep, we can improve the representation for all providers

sffc · 2024-05-31T20:56:27Z

provider/core/src/datagen/iter.rs

+        key_attributes: &DataKeyAttributes,
+    ) -> Result<bool, DataError> {
+        self.supported_requests()
+            .map(|v| v.contains(&(locale.clone(), key_attributes.clone())))


There's some discussion of options in https://stackoverflow.com/questions/45786717/how-to-implement-hashmap-with-two-keys

Doubly-nested HashMap is interesting, but that's a helluva lot of hashmaps...

provider/core/src/request.rs

provider/blob/src/export/blob_exporter.rs

sffc · 2024-05-31T22:25:16Z

Please also look at icu_datetime, and have a really close look at icu_datagen. I cannot figure out why the locale/aux set for datetime keys is changing.

The difference is in the deduplicate_payloads function.

The new code deduplicates more aggressively; for example, it finds the following match that the old code did not find:

2024-05-31T22:05:21.385Z TRACE [icu_datagen::driver] Deduplicating datetime/patterns/buddhist/date@1/"l"/en-GB (inherits from en-001)

I'm trying to investigate why.

sffc · 2024-05-31T22:27:12Z

I added more printing to the old code, and I get this message

2024-05-31T22:25:00.908Z TRACE [icu_datagen::driver] Not a patch: datetime/patterns/buddhist/date@1/en-GB-x-l (does not match en-x-l)

I don't know why it thinks en-GB falls back to en...

sffc · 2024-05-31T22:32:45Z

Ok, it appears to be a bug in the LocaleFallbacker where it doesn't ignore the aux keys when it should, which is effectively fixed by removing aux keys from DataLocale. #4983

robertbastian · 2024-05-31T22:54:23Z

Thanks for investigating.

Co-authored-by: Shane F. Carr <[email protected]>

provider/blob/benches/auxkey_bench.rs

sffc · 2024-05-31T23:24:18Z

components/calendar/src/week_of.rs

@@ -66,7 +66,7 @@ impl WeekCalculator {
                &provider.as_downcasting(),
                DataRequest {
                    locale,
-                    metadata: Default::default(),
+                    ..Default::default()


Thought: using ..Default::default() means that if we ever refactor DataRequest again, we won't get compiler errors for missing fields, which I think was an important part of building this PR.

Didn't really need or use that. Searching for DataRequest { is exhaustive because there are no constructors.

robertbastian added 19 commits May 31, 2024 13:37

-experimental

ce364a9

s/AuxiliaryKeys/DataKeyAttributes

37025a0

checkpoint

58cd08c

exporter

307d3e1

cehck

acfb229

testdata

4494de2

baked

b1c26b6

fix

b2a454d

f

663a9d4

f

8a2bbec

ugh

881c603

d

e60b39e

d

7f9b406

f

8a63e2f

f

3864bcd

revert baked?

d5055a0

blob

23a5633

more bake

1dfaf56

h

8322606

robertbastian marked this pull request as ready for review May 31, 2024 18:25

robertbastian requested review from sffc, Manishearth, aethanyc, makotokato, zbraniecki, dminor, pdogr, snktd, eggrobin and nordzilla as code owners May 31, 2024 18:26

robertbastian removed request for echeran, makotokato, eggrobin, pdogr, nordzilla and snktd May 31, 2024 18:26

sffc mentioned this pull request May 31, 2024

IterableDataProvider::supported_locales is a misnomer #4872

Closed

sffc reviewed May 31, 2024

View reviewed changes

baked

17c3d85

robertbastian requested a review from sffc May 31, 2024 19:24

s

cd4178d

sffc reviewed May 31, 2024

View reviewed changes

sffc mentioned this pull request May 31, 2024

Fix fallback algorithm to ignore aux keys when looking up parents #4983

Closed

robertbastian and others added 5 commits June 1, 2024 00:57

sizes

a2bdbd1

aux fallback diff (unicode-org#4983)

0d08e97

Update provider/datagen/src/transform/cldr/datetime/neo.rs

a613bba

Co-authored-by: Shane F. Carr <[email protected]>

fmt

490147e

rev fwd

cabd703

sffc reviewed May 31, 2024

View reviewed changes

provider/blob/benches/auxkey_bench.rs Show resolved Hide resolved

more unicode-org#4983

69a71ef

robertbastian requested a review from sffc May 31, 2024 23:18

sffc approved these changes May 31, 2024

View reviewed changes

sffc reviewed May 31, 2024

View reviewed changes

robertbastian merged commit 0ab2630 into unicode-org:main May 31, 2024
28 checks passed

robertbastian deleted the attrs branch June 6, 2024 17:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moving `AuxiliaryKeys` to `DataRequest` as `DataKeyAttributes` #4981

Moving `AuxiliaryKeys` to `DataRequest` as `DataKeyAttributes` #4981

robertbastian commented May 31, 2024 •

edited

Loading

sffc left a comment

sffc May 31, 2024

sffc May 31, 2024

robertbastian May 31, 2024

robertbastian May 31, 2024

sffc May 31, 2024

sffc May 31, 2024

robertbastian May 31, 2024

sffc May 31, 2024

sffc May 31, 2024

robertbastian commented May 31, 2024

sffc May 31, 2024

robertbastian May 31, 2024

sffc May 31, 2024

sffc commented May 31, 2024

sffc commented May 31, 2024

sffc commented May 31, 2024

robertbastian commented May 31, 2024

sffc May 31, 2024

robertbastian May 31, 2024 •

edited

Loading

Moving AuxiliaryKeys to DataRequest as DataKeyAttributes #4981

Moving AuxiliaryKeys to DataRequest as DataKeyAttributes #4981

Conversation

robertbastian commented May 31, 2024 • 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

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

robertbastian commented May 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc commented May 31, 2024

sffc commented May 31, 2024

sffc commented May 31, 2024

robertbastian commented May 31, 2024

Choose a reason for hiding this comment

robertbastian May 31, 2024 • edited Loading

Choose a reason for hiding this comment

Moving `AuxiliaryKeys` to `DataRequest` as `DataKeyAttributes` #4981

Moving `AuxiliaryKeys` to `DataRequest` as `DataKeyAttributes` #4981

robertbastian commented May 31, 2024 •

edited

Loading

robertbastian May 31, 2024 •

edited

Loading