mikemccand commented on a change in pull request #220: URL: https://github.com/apache/lucene/pull/220#discussion_r677406777
########## File path: lucene/facet/src/test/org/apache/lucene/facet/taxonomy/directory/TestBackwardsCompatibility.java ########## @@ -49,13 +49,8 @@ // // Then move the zip file to your trunk checkout and use it in your test cases - public static final String oldTaxonomyIndexName = "taxonomy.8.6.3-cfs"; + public static final String oldTaxonomyIndexName = "taxonomy.8.10.0-cfs"; - // LUCENE-9334 requires consistency of field data structures between documents. - // Old taxonomy index had $full_path$ field indexed only with postings, - // It is not allowed to add the same field $full_path$ indexed with BinaryDocValues - // for a new segment, that this test is trying to do. - @AwaitsFix(bugUrl = "https://issues.apache.org/jira/browse/LUCENE-9334";) Review comment: Woot! ########## File path: lucene/facet/src/java/org/apache/lucene/facet/taxonomy/directory/DirectoryTaxonomyWriter.java ########## @@ -475,8 +477,15 @@ private int addCategoryDocument(FacetLabel categoryPath, int parent) throws IOEx String fieldPath = FacetsConfig.pathToString(categoryPath.components, categoryPath.length); fullPathField.setStringValue(fieldPath); + + if (useOlderStoredFieldIndex) { + fullPathField = new StringField(Consts.FULL, fieldPath, Field.Store.YES); Review comment: Hmm this is odd -- up above this `if` we are already calling `setStringValue` on the reused `fullPathField`, but then inside this `if` we override `fullPathField` with a new `StringField` instance? Maybe just move the above line inside the `if`? Also, why don't we also re-use the `BinaryDocValues` field? ########## File path: lucene/facet/src/java/org/apache/lucene/facet/taxonomy/directory/DirectoryTaxonomyWriter.java ########## @@ -164,9 +159,16 @@ public DirectoryTaxonomyWriter(Directory directory, OpenMode openMode, TaxonomyW openMode = config.getOpenMode(); if (!DirectoryReader.indexExists(directory)) { Review comment: Maybe take the opportunity to repair this horrible `!` by switching it to `== false`? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
