CLDR-19641 Add JSON representation of XPath coverage levels to cldr-json - #5909
CLDR-19641 Add JSON representation of XPath coverage levels to cldr-json#5909sffc wants to merge 8 commits into
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
srl295
left a comment
There was a problem hiding this comment.
see comments in ticket:
need clear description of what data is requested and how it is to be used.
xpath doesn't make sense in a non-xml environment
9809d17 to
bce94de
Compare
This comment was marked as outdated.
This comment was marked as outdated.
bce94de to
f2a8383
Compare
This comment was marked as outdated.
This comment was marked as outdated.
f2a8383 to
87407f8
Compare
This comment was marked as outdated.
This comment was marked as outdated.
87407f8 to
ebf73b3
Compare
This comment was marked as outdated.
This comment was marked as outdated.
| } | ||
| } | ||
|
|
||
| public void writeCoverageLevelsByXPath(String outputDir) throws IOException { |
There was a problem hiding this comment.
+1 this is the right way to hook this in
ebf73b3 to
08120ac
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
🤖 This comment is written by Antigravity, an AI agent working with @sffc. Size Analysis: Including
|
| Baseline Configuration | Total Root XPaths | Uncompressed Size | Gzipped Size |
|---|---|---|---|
Excluding comprehensive (Current) |
8,545 | 693 KB | 38.5 KB |
Including comprehensive |
25,062 | 2.9 MB | 145 KB |
Note: Per-locale override files (e.g. coverageByXPath/it-CH.json) are unaffected and remain small (~2.3 KB).
Should comprehensive coverage level XPaths be included in the root coverageByXPath.json output file?
08120ac to
d5f63c7
Compare
This comment was marked as outdated.
This comment was marked as outdated.
d5f63c7 to
f28aa9e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
f28aa9e to
c56c6a3
Compare
This comment was marked as outdated.
This comment was marked as outdated.
c56c6a3 to
58a8260
Compare
This comment was marked as outdated.
This comment was marked as outdated.
… cldr-misc-full Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>
58a8260 to
060c6d4
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
Shouldn't this generate JSON paths instead of XML paths? |
|
XPaths are well-documented in the spec, and they are more stable. JSON paths are the opposite: not well-documented, and not as stable (they are subject to change when we find bugs). Maybe we want to have a JSON version of this in the future, though, which is why I named these files explicitly |
…rce files Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>
… against root coverage Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…aluated against root coverage Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
Updated PR #5909: Harvest baseline XPaths across all available locales rather than relying solely on en.xml. This ensures XPaths present in non-English locales (such as |
… root coverageByXPath Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>
d9348f7 to
ee629fc
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>
| } catch (NoSourceDirectoryException e) { | ||
| // Skip legacy metadata locale aliases that lack XML source files. | ||
| } |
There was a problem hiding this comment.
this sounds like it's some other bug, can you give an example?
| // Get a union of all XPaths in all locales of CLDR in main. | ||
| // Then, use root to determine the baseline coverage level for each XPath. | ||
| // We can't pull XPaths from only root.xml since that file contains only a | ||
| // subset of all data. |
There was a problem hiding this comment.
this isn't quite optimal, as fullIterable is the extra xpaths. Can be optimized later.
| throw new IllegalStateException( | ||
| "Unexpected coverage level " + l + " for XPath: " + x); | ||
| } | ||
| // Skip COMPREHENSIVE level as it represents the catch-all max coverage tier. |
There was a problem hiding this comment.
Not sure about skipping comprehensive.
| + miscDir.getAbsolutePath() | ||
| + File.separator | ||
| + "coverageByXPath.json using CoverageLevel2 API"); | ||
| outf.println(gson.toJson(rootCoverageObj)); |
There was a problem hiding this comment.
coverageByXPath.json is the root coverage. I still think it ought to be the und file alongside the others.
There was a problem hiding this comment.
I still disagree about putting the root coverage in und.json. CLDR JSON has no inheritance structure. All the files are fully resolved. Since this is infeasible for these coverage levels, I am defining this structure to be "the diff between 'that coverage file over there' and this locale's coverage". That "coverage file over there" does not necessarily need to be root, and the algorithm for generating it is different from the algorithm that would be used for und.json: for example, it includes XPaths that are not in root.xml.
If we were to put the coverage levels in und.json, something needs to break:
- There is no inheritance in CLDR JSON (but now there would be: but only for coverage levels)
- The
undlocale is exported just like any other locale (but would not be: it would contain xpaths that don't exist in und.xml, for example) - The locale-specific coverage level files contain only locale-specific overrides (but if we make
undcontain only the und.xml levels, locale files will contain a lot more coverage levels for XPaths they have that aren't in und.xml)
There was a problem hiding this comment.
Then add a: "basedOn": "und" to all files
There was a problem hiding this comment.
Have you checked difference between en-MO, en-CA etc?
we might want the possibility of "basedOn": "en" to further reduce the data (even if we don't do that at first)
There was a problem hiding this comment.
Since this is infeasible for these coverage levels
Why? CLDR-JSON contains mainly redundant data already. ICU4X-style data-compression techniques are not required for CLDR-JSON.
There was a problem hiding this comment.
different from the algorithm that would be used for und.json: for example, it includes XPaths that are not in root.xml.
It might include xpaths that aren't in root.xml, but the root file (I'll call it that) has the coverage levels that pertain to root aka und. So we should call it und. I'm not sure why you want to have a separate not-und file in a parent directory, and then an empty und file.
There was a problem hiding this comment.
Then add a:
"basedOn": "und"to all files
...Since this is infeasible for these coverage levels
Why? CLDR-JSON contains mainly redundant data already. ICU4X-style data-compression techniques are not required for CLDR-JSON.
The root coverage file is 1.01 MB. I don't want to copy the 1.01 MB into all 780 locales when there is an easy alternative. At the same time, I do not want to add full-fledged inheritance. I just want to take care of the lowest hanging fruit to reduce the unzipped json size by nearly 800 MB.
I would agree with you both if CLDR JSON had inheritance, but it doesn't. und is a locale just like any other locale. You can remove it from your archive and nothing breaks. No locale depends on any other locale. You can pull a locale based on directory names. Putting the root coverage XPaths in a metadata-like JSON file maintains this property. I am really quite surprised that the CLDR JSON maintainers are suggesting to break this invariant.
If adding the 800 MB of data will unblock this PR, I would rather do that than add inheritance, including inheritance to the locale named und.
There was a problem hiding this comment.
What I don't understand is how you're taking the single coverageLevels.xml file, which is 210kB, and produce 800MB of data out of that in the first place. Yes if you do that you need to come up with unprecedented ways of data modelling to reduce that size; but do you need to do that?
| // Emit und.json with empty overrides map for undetermined locale | ||
| JsonObject undObj = new JsonObject(); | ||
| JsonObject undInnerObj = new JsonObject(); | ||
| undObj.add("coverageByXPath", undInnerObj); | ||
| undInnerObj.add("und", new JsonObject()); |
| "languages:language:menu", | ||
| "monthWidth:month:yeartype", | ||
| "characters:parseLenients:scope", | ||
| "characters:placeholderBoundarySpacing:type", |
robertbastian
left a comment
There was a problem hiding this comment.
- The format for
cldr-misc-fulliscldr-misc-full/<bcp47loc>/<dataType>.json, notcldr-misc-full/<dataType>/<bcp47loc>.json - Inside the files the top levels are
{
"main": {
"<bcp47loc>": {
}
}
}- The root data file should be in a directory called
und–rootis not used in CLDR-JSON
Almost right. It's
Also close but not exactly right: {
"main": {
"<bcp47loc>": {
"<datatype>": {
}
}
}
}Maybe we should keep the same. So what about this: cldr-misc-full/main/fr/coverageByXPath.json {
"main": {
"fr": {
"coverageByXpath": {
}
}
}
}And have it strictly limited to the 'main' tree.
Yeah, I'd prefer this to having the root data file with a special name. |
|
Ah yes I forgot the |
|
I'm very happy to put these wherever CLDR INFRA wants me to put them. Last week when I asked @srl295 on Slack, we came up with the structure currently in this PR. We discussed that since this is not actual locale data, it doesn't really make sense to put in the locale main directories alongside the actual locale data. But, again, I will put this wherever you think it should go. I assume I will go with #5909 (comment) unless I hear otherwise. The only structural comment where I have an opinion is putting the reference coverage levels in |
|
It should go in the locale specific subdirectories with other files. And, the |
…cale> directories Put locale coverage level override files into cldr-misc-full/main/<locale>/coverageByXPath.json following standard CLDR JSON package layout without identity section. Add TODO comments for NoSourceDirectoryException and und locale handling. Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
🤖 This comment is written by an AI agent working with @sffc. Updated JSON File Layout for
|
Coming back to this: if we change a JSON path, consumers have to update their code anyway, so it's not a problem if the coverage path also changes. |
|
@robertbastian Do you have a suggestion on how to generate the files with JSON paths? Or are you suggesting that as potential future work? |
|
I believe the code for transforming an XML path to a JSON path is in https://github.com/robertbastian/cldr/blob/main/tools/cldr-code/src/main/java/org/unicode/cldr/json/LdmlConvertRules.java. Given that you're planning on hardcoding this data in ICU4X anyway, why don't you just hardcode the XML file in the short term, and we can actually take time to design this? |
I'm +1 on "actually take time to design this" I'm -1 on the XML file as an interim solution, because the XML file is not very portable or easy for ICU4X to consume. It has weird variables that are only resolvable by using the CoverageLevels2 Java class. |
|
Given that you only need this for a handful of paths, you could literally just have a Rust file with the data you need. |
My branch in ICU4X currently contains a filtered-down version of the files generated by the previous version of this PR, and they are linked into Rust code using the Abstract Memory FS. |
|
With my CLDR hat: I don't think this can be merged, there are a lot of design decisions that you took that I don't agree with and that we should discuss:
With my ICU4X hat: Your branch contains a good amount of code that is very specific to all of these decisions, which is why I don't want to merge the harcoded data from this PR, but simpler data |
CLDR-19641
This PR adds a JSON representation of API-computed path-specific coverage levels to
cldr-jsontool output ascoverageByXPath.jsonincldr-misc-full.As explained in the issue, this allows ICU4X and other consumers to inspect path-specific coverage requirements by region/locale for improved data slicing.
Here is
coverageByXPath.jsonsample output generated from this PR: https://gist.github.com/sffc-bot/e1e2415c9ce1a35084cea1c0aed2d937Implementation
Instead of raw XML translation, this implementation uses the CLDR Java
CoverageLevel2/SupplementalDataInfoAPIs (cov.getLevel(xpath)) across locales to compute and serialize effective coverage levels (core,basic,moderate,modern) intocldr-misc-full.Output layout:
cldr-misc-full/coverageByXPath.json({ "coverageByXPath": { "root": { ... } } })cldr-misc-full/coverageByXPath/<bcp47loc>.json(e.g.,it-CH.json,und.jsoncontaining{ "coverageByXPath": { "<bcp47loc>": { ... } } })🤖 Antigravity helped generate this change.