Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-10472

Fix uninversioning of (single valued) PointFields & clean up some related code

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.6, 7.0
    • Component/s: None
    • Security Level: Public (Default Security Level. Issues are Public)
    • Labels:
      None

      Description

      In getting caught up with the new PointsField work in SOLR-8396 & SOLR-9987 I realized:

      • There's some inconsistencies/contridictions in how the new field types implement FieldType.getUninversionType vs some of the error handling added to methods like SchemaField.checkSortability.
      • as part of SOLR-9987, SORTED_foo constants were added to UninvertingReader and used in the getUninversionType methods for PointFields – even though those constants are useless (because UninvertingReader doesn't support uninverting multivalued points: SOLR-9202)
      • the changes made to methods like SchemaField.checkSortability to explicitly check isPointsField should have never been needed if those methods were paying attention to getUninversionType – but those types of checks weren't added when getUninversionType was introduced (the existing checks pre-date the DocValues API, back where any single-valued indexed field was implicitily "uninvertable")
      • If all of the above is corrected, the only thing preventing UninvertedReader from working properly with Solr's new PointField types (in the single valued case) is a trivial bug in IndexSchema (being presumptutious about what is uninvertable)

      I'm opening this issue to track fixing all of this, such that the end result will be:

      • single valued PointFields will be uninvertable (ie: sortable even if they don't have docvalues)
      • error handling code will be simplified
      • unused/unsupported/misleading constants in UninvertingReader will be removed.
      1. SOLR-10472.patch
        52 kB
        Hoss Man
      2. SOLR-10472.patch
        20 kB
        Hoss Man
      3. SOLR-10472.patch
        20 kB
        Hoss Man

        Issue Links

          Activity

          Hide
          hossman Hoss Man added a comment -

          Here's my starting point of a patch, showing:

          • the logic fixes for all IndexSchema & all FooPointField classes
          • corrected check* methods in SchemaField (and simplifying the logic so it's more future proof for other similar classes down the road)
          • removal of usless/missleading constants from UninvertingReader
          • TestPointFields changes demonstrating this new logic for sorting IntPointField
            • most of the remaining work needed is here, changing the tests for other types, fixing the FunctionQuery tests, and beefing up the test coverage

          Note: i'll be offline for (at least) a week (vacation, then possible jury duty) so if there are comments/questions about this i won't be around to answer them for a while ... i'll plan on picking this back up when i return unless someone else beats me to it.

          Show
          hossman Hoss Man added a comment - Here's my starting point of a patch, showing: the logic fixes for all IndexSchema & all FooPointField classes corrected check* methods in SchemaField (and simplifying the logic so it's more future proof for other similar classes down the road) removal of usless/missleading constants from UninvertingReader TestPointFields changes demonstrating this new logic for sorting IntPointField most of the remaining work needed is here, changing the tests for other types, fixing the FunctionQuery tests, and beefing up the test coverage Note: i'll be offline for (at least) a week (vacation, then possible jury duty) so if there are comments/questions about this i won't be around to answer them for a while ... i'll plan on picking this back up when i return unless someone else beats me to it.
          Hide
          hossman Hoss Man added a comment -

          patch updated to master.

          LukeRequestHandlerTest.testLuke is currently failing (reliably) in a weird way for reason i haven't wrapped by head around yet – not sure if that's a new consequence of other changes on master, or something i overlooked before.

          since i haven't seen any objections/concerns to the overall idea of this jira, i'll plan on moving forward with fixing up the remaining nocommits and beefing up the tests – starting with a dive into what's going wrong in testLuke

          Show
          hossman Hoss Man added a comment - patch updated to master. LukeRequestHandlerTest.testLuke is currently failing (reliably) in a weird way for reason i haven't wrapped by head around yet – not sure if that's a new consequence of other changes on master, or something i overlooked before. since i haven't seen any objections/concerns to the overall idea of this jira, i'll plan on moving forward with fixing up the remaining nocommits and beefing up the tests – starting with a dive into what's going wrong in testLuke
          Hide
          hossman Hoss Man added a comment -
          • the root cause of the testLuke failure was (evidently) because the IndexSchema changes i had made for got to check if a field was indexed=true before including it in the uninversion map - that seemed to trigger some unexpected code path in the LukeRequestHandler which caused no response for non-indexed (non-docvalue) fields.
          • I resolved all the nocommits & beefed up the Points Sorting & ValueSource testing
            • i also fleshed out some more TODO items relating to sort missing first/last (see SOLR-10501: there were already TODO items about this, improving the test coverage in this way shouldn't block the uninverting improvements)
          • Changing SchemaField.checkFieldCacheSource() to check getUninversionType led me to discover bugs in CurrencyField – it's getValueSource(field) method was calling field.checkFieldCacheSource() even though there is no reason why the field itself needs to be indexed/docvalues – that's the job of the underlying poly-fields, which were NOT being checked, so i fixed that as well.
            • see also: SOLR-10502 & SOLR-10503, both other CurrencyField improvements for the future i noticed while working looking into this.
          Show
          hossman Hoss Man added a comment - the root cause of the testLuke failure was (evidently) because the IndexSchema changes i had made for got to check if a field was indexed=true before including it in the uninversion map - that seemed to trigger some unexpected code path in the LukeRequestHandler which caused no response for non-indexed (non-docvalue) fields. I resolved all the nocommits & beefed up the Points Sorting & ValueSource testing i also fleshed out some more TODO items relating to sort missing first/last (see SOLR-10501 : there were already TODO items about this, improving the test coverage in this way shouldn't block the uninverting improvements) Changing SchemaField.checkFieldCacheSource() to check getUninversionType led me to discover bugs in CurrencyField – it's getValueSource(field) method was calling field.checkFieldCacheSource() even though there is no reason why the field itself needs to be indexed/docvalues – that's the job of the underlying poly-fields, which were NOT being checked, so i fixed that as well. see also: SOLR-10502 & SOLR-10503 , both other CurrencyField improvements for the future i noticed while working looking into this.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 10772121eee97023aed415751e49a06d116b26ad in lucene-solr's branch refs/heads/master from Chris Hostetter
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=1077212 ]

          SOLR-10472: Fixed uninversion (aka: FieldCache) bugs with the numeric PointField classes, and CurrencyField

          Show
          jira-bot ASF subversion and git services added a comment - Commit 10772121eee97023aed415751e49a06d116b26ad in lucene-solr's branch refs/heads/master from Chris Hostetter [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=1077212 ] SOLR-10472 : Fixed uninversion (aka: FieldCache) bugs with the numeric PointField classes, and CurrencyField
          Hide
          hossman Hoss Man added a comment -

          on master, planning to backport to 6x this afternoon/tomorrow.

          Show
          hossman Hoss Man added a comment - on master, planning to backport to 6x this afternoon/tomorrow.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit dedb0255947ed936e001b244ff1859075398dc40 in lucene-solr's branch refs/heads/branch_6x from Chris Hostetter
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=dedb025 ]

          SOLR-10472: Fixed uninversion (aka: FieldCache) bugs with the numeric PointField classes, and CurrencyField

          (cherry picked from commit 10772121eee97023aed415751e49a06d116b26ad)

          Conflicts in test due to slightly diff internal DocValues APIs on master vs 6x
          solr/core/src/test/org/apache/solr/schema/TestPointFields.java

          Show
          jira-bot ASF subversion and git services added a comment - Commit dedb0255947ed936e001b244ff1859075398dc40 in lucene-solr's branch refs/heads/branch_6x from Chris Hostetter [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=dedb025 ] SOLR-10472 : Fixed uninversion (aka: FieldCache) bugs with the numeric PointField classes, and CurrencyField (cherry picked from commit 10772121eee97023aed415751e49a06d116b26ad) Conflicts in test due to slightly diff internal DocValues APIs on master vs 6x solr/core/src/test/org/apache/solr/schema/TestPointFields.java

            People

            • Assignee:
              hossman Hoss Man
              Reporter:
              hossman Hoss Man
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development