Repository navigation
Conversation
grhoten
left a comment
grhoten
left a comment
There was a problem hiding this comment.
It's going in the right direction. I'm getting hung up on some of the details. Some of the details I've gotten hung up on for years, especially the layering/overriding of data.
| /** | ||
| * Returns {@code true} when multiple input files are provided and {@code --merge-conflicts} is {@code false} | ||
| * (the default). In this mode, later files on the CLI override conflicting entries of the same | ||
| * {@link PartOfSpeech} from earlier files via a two-tier strategy: | ||
| * <ul> | ||
| * <li><b>Tier 1 (Lemma/Paradigm level)</b>: {@link #recordOrAnalyzeLemma} stages lemmas and | ||
| * {@link #processStagedLemmas} strips any {@code (lemma.value, PartOfSpeech)} superseded by a later | ||
| * source before {@link #analyzeInflections} runs. This prevents removed inflected surface forms | ||
| * (e.g., an obsolete plural dropped in a patch file) from lingering as orphans in {@code dictionary.lst} | ||
| * and prevents superseded paradigms from inflating {@link InflectionPattern} counts in {@code inflectional.xml}.</li> | ||
| * <li><b>Tier 2 (Surface-form level)</b>: {@link DictionaryEntry#merge} resolves remaining surface-form | ||
| * collisions per {@link PartOfSpeech} using {@code sourceIndex}.</li> | ||
| * </ul> | ||
| */ | ||
| private boolean isMultiSourceOverwriteMode() { | ||
| return !parserOptions.mergeConflicts && parserOptions.sourceInputs.size() > 1; | ||
| } |
There was a problem hiding this comment.
I'm not quite understanding this logic. What does merge mean? Here are the options that I see when there is a conflict.
- Pattern is mixed in with the words with normal priority sorting.
- Pattern is mixed in, and the conflict comes first.
- New pattern and grammemes overrides all previous data for a surface form.
- Normal processing happens, until you hit the overrides, and then one of the above options happens.
There was a problem hiding this comment.
If merging continues being an issue, we can remove it from this round and just do overrides for now.
Option 4 makes the design much cleaner. I removed the two-tier lemma staging (stagedLemmas) in ParseWikidata and the per-POS PosSlice splitting in DictionaryEntry, and replaced them with Option 4:
- All files now stream directly into
analyzeLemma(...)with normal intra-file merging. - When a surface form from a later source file (
entry.sourceIndex > this.sourceIndex) collides with an existingDictionaryEntry:- By default (
--merge-conflicts=false— Option 4 + Option 3): The new entry's pattern and grammemes completely override the previous data for that surface form (matching how--supplementalfileworks inbuildDictionary). If the same override file itself has multiple entries for that surface form (samesourceIndex), they merge normally with each other. Before writinginflectional.xml,DocumentState.printDocument()prunes anyInflectionPatternwhose surface forms were all overridden soinflectional.xmlstays in sync withdictionary.lst. - When
--merge-conflicts=true(Option 4 + Option 2): The new entry's grammemes and pattern are mixed in, and the later source's conflicting pattern is placed first in priority ahead of earlier sources' patterns.
- By default (
| return TYPEMAP.get(grammeme); | ||
| if (grammeme == null) { | ||
| return null; | ||
| } | ||
| Set<? extends Enum<?>> mapped = TYPEMAP.get(grammeme); | ||
| if (mapped != null) { | ||
| return mapped; | ||
| } | ||
| return TYPEMAP.get(normalizeGrammemeKey(grammeme)); |
There was a problem hiding this comment.
This is hit constantly, and it can add to the parse time. Is there a way to optimize this. Perhaps there should be 2 maps? If that complicates the logic elsewhere too much, then maybe not.
Also if grammeme is null, that't an error condition. You need something! Even a part of speech helps.
There was a problem hiding this comment.
- Removed
normalizeGrammemeKey()completely;getMappedGrammemes()is back to a singleTYPEMAP.get(grammeme)lookup with zero string allocations, and DMLex entries use canonical grammeme names fromDEFAULTMAP(seeded once intoTYPEMAPat static initialization). - Added an explicit
if (grammeme == null) throw new IllegalArgumentException("Grammeme cannot be null");check so anullgrammeme fails fast.
| // Per-run string-to-grammeme overrides configured via --map-grammeme or Q-ID arguments to | ||
| // --ignore-property / --ignore-entries-with-grammemes. Stored per ParserOptions instance so | ||
| // static Grammar.TYPEMAP is never mutated in place. | ||
| private final Map<String, Set<? extends Enum<?>>> customTypeMap = new HashMap<>(); | ||
| // Per-run enum overrides configured via --ignore-property or --ignore-entries-with-grammemes | ||
| // when specified by grammeme name (e.g. "countable", "shortForm"). Any resolved grammeme enum | ||
| // present in this map is replaced with its configured Grammar.Ignorable token during lookup. | ||
| private final Map<Enum<?>, Grammar.Ignorable> ignoredGrammemes = new HashMap<>(); |
There was a problem hiding this comment.
Can you elaborate on the purpose of this change? What issue was encountered when mutating Grammar.TYPEMAP?
There was a problem hiding this comment.
This was originally added out of concern that mutating static Grammar.TYPEMAP via --map-grammeme or --ignore-property across multiple ParseWikidata.main(...) calls in ParseWikidataTest could affect subsequent tests in the same JVM.
In practice, the test cases use distinct test-only grammemes and keeping separate customTypeMap and ignoredGrammemes fields on ParserOptions made both ParserOptions and every grammeme lookup unnecessarily complicated. I removed customTypeMap and ignoredGrammemes and reverted ParserOptions back to mutating Grammar.TYPEMAP directly as before.
| var grammemeEnum = DEFAULTMAP.get(grammeme); | ||
| if (grammemeEnum == null) { | ||
| grammemeEnum = DEFAULTMAP.get(Grammar.normalizeGrammemeKey(grammeme)); | ||
| } | ||
| if (grammemeEnum == null && !grammeme.matches("Q\\d*")) { | ||
| Set<? extends Enum<?>> mapped = Grammar.getMappedGrammemes(grammeme); | ||
| if (mapped != null && mapped.size() == 1) { | ||
| grammemeEnum = mapped.iterator().next(); | ||
| } | ||
| } |
There was a problem hiding this comment.
This is getting complicated. Is there a way to simplify this?
There was a problem hiding this comment.
Simplified—by removing customTypeMap, ignoredGrammemes, ParserOptions.getMappedGrammemes(), and normalizeGrammemeKey(), setIgnoreProperty() and toEnumSet() are back to their original logic from main.
| void reset() { | ||
| value = null; | ||
| isRare = false; | ||
| grammemes.clear(); | ||
| inflections.clear(); | ||
| } | ||
|
|
There was a problem hiding this comment.
This was done to improve the performance. It's not stored long term. Can you elaborate on why you made this change?
There was a problem hiding this comment.
Lemma.reset() was only removed because the earlier two-tier override logic buffered Lemma instances in stagedLemmas until all files were read. Now that we use streaming (Option 4) without stagedLemmas, Lemma is short-lived again and I have restored Lemma.reset().
| * unusual case where {@code "entries"} appears before {@code "langCode"} in the JSON key order, | ||
| * entries are buffered in {@code entriesBeforeLangCode} until {@code "langCode"} is read. | ||
| */ | ||
| private void parseDMLexStream( |
There was a problem hiding this comment.
Is there a way to leverage a JsonDeserializer to simplify this logic? This seems to be going fairly low level with the parsing.
There was a problem hiding this comment.
Done—replaced the manual JsonToken loop in parseDMLexStream with objectMapper.readValue(parser, DMLexEntry.Lexicon.class) and simplified DMLexStringListDeserializer to use JsonNode.
This branch has not been deployed
2 participants
Fixes #208
Summary
Adds support for parsing OASIS DMLex v1.0 JSON dictionaries alongside (or instead of) Wikidata lexeme dumps in
tools/dictionary-parser, with configurable multi-source conflict resolution.Key Changes
ParserOptions):--wikidata <file1> [<file2> ...]and--dmlex <file1> [<file2> ...](supporting multiple space- or comma-separated files as well as--wikidata=<files>/--dmlex=<files>, including.bz2compressed inputs).--merge-conflicts[={true|false}](defaultfalse): controls whether same-part-of-speech entries across multiple input files are overridden by later files (false) or merged (true).Grammar.TYPEMAPwith per-instance overrides (customTypeMapandignoredGrammemes) onParserOptions.DMLexEntry,DMLexInflectedForm,DMLexStringListDeserializer,ParseWikidata):parseDMLexStream) validating the top-level DMLex JSON object (langCodeandentries) and converting entries into the sharedLemma/Inflectionpipeline.headwordwheninflectedFormsis omitted.Grammarlookup (getMappedGrammemes) to normalize camelCase, snake_case, and hyphenated DMLex/Universal Dependencies grammatical tags in addition to Wikidata Q-IDs.ParseWikidata,DictionaryEntry):--merge-conflicts=false(default) and multiple source files are provided, stages lemmas and preempts any earlier(lemma, PartOfSpeech)paradigm superseded by a later source file before generating inflection patterns (inflectional.xml) or surface forms (dictionary.lst), preventing orphaned surface forms and inflated pattern counts.PartOfSpeechprovenance (sourceIndex) insideDictionaryEntryso same-POS surface-form collisions from a later source replace earlier ones while preserving non-conflicting parts of speech (e.g., keepinginterjectionfrom an earlier source when a later source addsnoun) and intra-file homographs.ParseWikidataTest):--merge-conflicts=false), cross-source union merging (--merge-conflicts=true), partial multi-POS overrides, space/comma-separated file lists, CLI option compatibility (--map-grammeme,--expand-grammemes,--ignore-property,--ignore-entries-with-grammemes,--exclude-language), and schema validation errors.