Implement Grapheme_Cluster_Break, Word_Break, and Sentence_Break Unicode properties #1233

aethanyc · 2021-10-28T19:05:17Z

…ode properties required by UAX 29

The TOML file was obtained from Azure artifact archive from commit 2921a81ee4c67459ff455e31c599e7d7a09086ab titled "ICU-21811 TZ update 2021a (2021e)" on maint/maint-70 branch. This commit imports TrieType::Small flavor of the uprops files.

aethanyc · 2021-10-28T19:20:25Z

Some thoughts:

This PR is ready for review, but it will conflict with Connect properties provider to the icu4x_datagen exporter tool #1204 and need some tweak once 1204 merged.
To test codepointtrie getter API such as get_word_break, I need to import TOML files.
We need another PR to connect these properties to icu4x_datagen to convert the TOML files to a binary format to serve them in a production-ready segmenter.

markusicu · 2021-10-29T00:08:45Z

components/properties/src/props.rs

+    pub const RegionalIndicator: GraphemeClusterBreak = GraphemeClusterBreak(12); // name="RI"
+    pub const ZWJ: GraphemeClusterBreak = GraphemeClusterBreak(17); // name="ZWJ"


What about values 13..16?
https://unicode-org.github.io/icu-docs/apidoc/released/icu4c/uchar_8h.html#abb9bae7d2a1c80ce342be4647661fde1a1654351c7ced7d34223bbbfacf4853aa

13 .. 16 are marked as unused in the spec, and no v=13 .. v=16 in the TOML, so I left them undefined. Should I add them for completeness?

Good question. ICU of course has constants for these values because at some point some characters had them, the constants are stable API, and we also have API that lets people map between value names and enum constant values.

You don't need to support data from old Unicode versions, so maybe you don't need now-unused values.

However, another question is how \p{gcb=E_Base} would be supported. It should yield an empty set rather than an unknown-value error. Is it easier to do so if unused values are defined?

So... my hunch is that it's cleaner / more user-friendly to define all values, even ones that currently aren't used.

Question: is this set of 18 values stable, or will it grow over time?

If the set is stable, I think we should use a numeric enum rather than a newtype. (If the set will grow, I think the newtype is correct.)

So... my hunch is that it's cleaner / more user-friendly to define all values, even ones that currently aren't used.

Makes sense. I add them to avoid the confusion of the gaps in the enum values.

Question: is this set of 18 values stable, or will it grow over time?

From the spec's evolution and ICUC's doc UGraphemeClusterBreak, the set of value can grow overtime. For example, ZWJ and now unused value 13 .. 16 are introduced in Unicode 9, so it's hard to predict we won't ever introduce a new value.

We probably want all break properties, include LineBreak to be added in the future, in one format so that segmenter can access them uniformly.

Most enumerated properties can get new values added in future versions of Unicode. The ...Break properties frequently do.

markusicu · 2021-10-29T00:09:48Z

components/properties/src/props.rs

+    pub const DoubleQuote: WordBreak = WordBreak(16); // name=DQ
+    pub const ZWJ: WordBreak = WordBreak(21); // name="ZWJ"


17..20?
https://unicode-org.github.io/icu-docs/apidoc/released/icu4c/uchar_8h.html#af70ee907368e663f8dd4b90c7196e15caac5eadf5e27eca96b19f85ad3c786fcd

Same. 17.. 20 are obsoleted and used in the spec https://www.unicode.org/reports/tr29/#Default_Word_Boundaries

Same question as above w.r.t. stability (also below).

See my reply in grapheme cluster breaker. Also added the obsolete values.

markusicu · 2021-10-29T00:16:23Z

components/properties/src/trievalue.rs

@@ -27,3 +27,30 @@ impl TrieValue for Script {
        u16::try_from(i).map(Script)
    }
 }
+
+impl TrieValue for GraphemeClusterBreak {
+    const DATA_GET_ERROR_VALUE: GraphemeClusterBreak = GraphemeClusterBreak::Other;


FYI I know this is pre-existing, just sayin' --

The trie itself stores an error value. It would be nice to fetch that (and map it to the return type) rather than having to hardcode it for each type.
@echeran @sffc

This DATA_GET_ERROR_VALUE is a "last resort fallback" if the data is malformed and we can't get the error value out of it. We already have a ticket #1183 to discuss whether this is the best approach.

sffc · 2021-11-01T17:10:47Z

components/properties/src/maps.rs

+/// let root_dir = icu_testdata::paths::uprops_toml_root();
+/// let provider = EnumeratedPropertyCodePointTrieProvider::new(root_dir);


Nit, here and elsewhere: as of Friday, you can (and should) replace this with icu_testdata::get_provider(), which will work after you update your branch and run cargo make testdata

Added necessary glue code to run cargo make testdata, and regenerated the data.

sffc · 2021-11-01T17:11:48Z

components/properties/Cargo.toml

@@ -42,6 +42,8 @@ zerovec = { version = "0.4", path = "../../utils/zerovec", features = ["serde"]

 [dev-dependencies]
 icu = { path = "../../components/icu", default-features = false }
+icu_testdata = { version = "0.3", path = "../../provider/testdata" }
+icu_provider_uprops = { version = "0.3", path = "../../provider/uprops" }


Nit: As of Friday, you can (and should) remove this dependency

components/properties/src/props.rs

The json and postcard files are generated via `cargo make testdata`.

…compatible with ICU. These are obsolete values, so the generated testdata shouldn't change. Running `cargo make testdata` won't change the testdata.

sffc

LGTM! Nice job!

aethanyc · 2021-11-02T16:57:36Z

@markusicu @sffc Thank you for the review.

…ode properties (unicode-org#1233) The obsolete enum values in GraphemeClusterBreak and WordBreak are added to retain the compatibility with ICU. The TOML file was obtained from Azure artifact archive built on unicode-org/icu, commit 2921a81ee4c67459ff455e31c599e7d7a09086ab titled "ICU-21811 TZ update 2021a (2021e)" on maint/maint-70 branch. This commit imports TrieType::Small flavor of the uprops files. The json and postcard files are generated via `cargo make testdata`.

aethanyc added 2 commits October 28, 2021 11:08

Implement Grapheme_Cluster_Break, Word_Break, and Sentence_Break Unic…

b5ff95b

…ode properties required by UAX 29

aethanyc requested review from echeran and sffc October 28, 2021 19:05

aethanyc requested a review from a team as a code owner October 28, 2021 19:05

markusicu reviewed Oct 29, 2021

View reviewed changes

sffc reviewed Nov 1, 2021

View reviewed changes

aethanyc added 5 commits November 1, 2021 15:51

Merge remote-tracking branch 'origin/main' into uax29-props

ff4ec6b

Fix cargo build and test bustages due to changes in 'main' branch

9683ee3

Add necessary glue code to generate testdata for these break properties

88bff64

The json and postcard files are generated via `cargo make testdata`.

Remove icu_provider_uprops dependency and update doctest

387151f

Add obsolete enum values to GraphemeClusterBreak and WordBreak to be …

3e48bce

…compatible with ICU. These are obsolete values, so the generated testdata shouldn't change. Running `cargo make testdata` won't change the testdata.

markusicu mentioned this pull request Nov 2, 2021

Set CPT ValueWidth::DATA_GET_ERROR_VALUE to 0 #1183

Closed

sffc approved these changes Nov 2, 2021

View reviewed changes

aethanyc merged commit 0c855b1 into unicode-org:main Nov 2, 2021

aethanyc deleted the uax29-props branch November 2, 2021 17:02

echeran mentioned this pull request Mar 22, 2022

Add Bidi class data. #1716

Merged

echeran mentioned this pull request Aug 18, 2022

Support more than just Script in CodePointMap ffi (ICU4XCodePointMapData16) #2379

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Grapheme_Cluster_Break, Word_Break, and Sentence_Break Unicode properties #1233

Implement Grapheme_Cluster_Break, Word_Break, and Sentence_Break Unicode properties #1233

aethanyc commented Oct 28, 2021

aethanyc commented Oct 28, 2021

markusicu Oct 29, 2021

aethanyc Oct 29, 2021

markusicu Oct 29, 2021

sffc Nov 1, 2021

aethanyc Nov 1, 2021

markusicu Nov 2, 2021

markusicu Oct 29, 2021

aethanyc Oct 29, 2021

sffc Nov 1, 2021

aethanyc Nov 1, 2021

markusicu Oct 29, 2021

sffc Nov 1, 2021

sffc Nov 1, 2021

aethanyc Nov 1, 2021

sffc Nov 1, 2021

aethanyc Nov 1, 2021

sffc left a comment

aethanyc commented Nov 2, 2021

		pub const RegionalIndicator: GraphemeClusterBreak = GraphemeClusterBreak(12); // name="RI"
		pub const ZWJ: GraphemeClusterBreak = GraphemeClusterBreak(17); // name="ZWJ"

		pub const DoubleQuote: WordBreak = WordBreak(16); // name=DQ
		pub const ZWJ: WordBreak = WordBreak(21); // name="ZWJ"

		/// let root_dir = icu_testdata::paths::uprops_toml_root();
		/// let provider = EnumeratedPropertyCodePointTrieProvider::new(root_dir);

Implement Grapheme_Cluster_Break, Word_Break, and Sentence_Break Unicode properties #1233

Implement Grapheme_Cluster_Break, Word_Break, and Sentence_Break Unicode properties #1233

Conversation

aethanyc commented Oct 28, 2021

aethanyc commented Oct 28, 2021

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

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 left a comment

Choose a reason for hiding this comment

aethanyc commented Nov 2, 2021