[GitHub] [lucene] mikemccand commented on a change in pull request #220: LUCENE-9450: Use BinaryDocValue fields in the taxonomy index based on the existing index version

[GitBox <gi...@apache.org>]

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-unsubscribe@lucene.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org