Change uses of withReadAdvice to use hints instead by thecoop · Pull Request #14510 · apache/lucene

thecoop · 2025-04-16T15:51:43Z

Followon from #14482.

ReadAdvice is now only really used by MMapDirectory, no longer part of Directory.

This PR doesn't change any behaviour, it just replicates the ReadAdvice that would be used using hints + MMapDirectory.toReadAdvice.

lucene/test-framework/src/java/org/apache/lucene/tests/store/SerialIOCountingDirectory.java

thecoop · 2025-04-30T13:23:48Z

lucene/core/src/java/org/apache/lucene/store/MemorySegmentIndexInput.java

      int chunkSizePower,
-      boolean confined) {
+      boolean confined,
+      Function<IOContext, ReadAdvice> toReadAdvice) {


A Function representing the mapping keeps the IOContext -> ReadAdvice mapping entirely within MMapDirectory

lucene/core/src/java/org/apache/lucene/store/MMapDirectory.java

jpountz · 2025-05-02T13:13:06Z

...ard-codecs/src/java/org/apache/lucene/backward_codecs/lucene101/Lucene101PostingsReader.java

-      docIn = state.directory.openInput(docName, state.context.withReadAdvice(ReadAdvice.NORMAL));
+      docIn =
+          state.directory.openInput(
+              docName, state.context.withHints(FileTypeHint.DATA, FileDataHint.POSTINGS));


Should this call also pass a DataAccessHint? The fact that it's sometimes provided and sometimes not is a bit confusing to me.

This doesn't set a DataAccessHint as it's not RANDOM or SEQUENTIAL, just NORMAL.

I've tried to keep the initial hints as simple as possible here, without changing behaviour, so we can normalise and possibly modify behaviour in a subsequent PR

Ohhhh I had assumed until now that DataAccessHint.SEQUENTIAL effectively meant ReadAdvice.NORMAL.

It maps onto ReadAdvice.SEQUENTIAL

Hi @thecoop @jpountz ,

This doesn't set a DataAccessHint as it's not RANDOM or SEQUENTIAL, just NORMAL.

#14635 - with this PR - we have removed the change in MMAPDirectory to apply read advice as normal. So if no data access hint is provided, it falls back to default read advice which is random. So read advice has changed from default to random ? Can you please confirm if I'm missing something and if not, then should we revert back the read advice to normal ?

See reply in #14635 (comment)

...kward-codecs/src/java/org/apache/lucene/backward_codecs/lucene50/Lucene50CompoundReader.java

jpountz · 2025-05-02T13:20:40Z

lucene/core/src/java/org/apache/lucene/store/MMapDirectory.java

+    // readahead.
+    if (context.hints().contains(FileDataHint.POSTINGS)) {
+      return ReadAdvice.NORMAL;
+    }


I feel like this default implementation should be dumb and trust the DataAccessHint, and the FileDataHint hint should only be used by users to override defaults when they know better about their data / access pattern?

Currently, ReadAdvice.NORMAL doesn't have a corresponding DataAccessHint, so you get that by not specifying the DataAccessHint but then as the default is now ReadAdvice.RANDOM (or configurable), you need some way to explicitly specify 'I want the OS default behaviour here'.

Maybe we do have a DataAccessHint.DEFAULT, but then doesn't DataAccessHint just mirror ReadAdvice?

I think it's natural for DataAccessHint to be closely related to ReadAdvice.

My suggestion would be to do the following:

DataAccessHint has a single enum constant: RANDOM

the lack of a DataAccessHint means that the access pattern is Lucene's standard access pattern: a mix of forward seeks and reads of data in the order of 1kB-1MB (what terms, postings, points and doc values typically do)

Vectors pass a DataAccessHint.RANDOM at open time, they are the only ones to pass a DataAccessHint to the IOContext, because they're the only one to seek backwards to evaluate a single query

To mimic today's behavior MMapDirectory's default implementation uses Constants.DEFAULT_READADVICE on all files that have FileTypeHint.DATA.

In the future (different PR), we can discuss whether MMapDirectory's default implementation should force ReadAdvice.RANDOM for DataAccessHint.RANDOM, and whether Constants.DEFAULT_READADVICE should move back to NORMAL.

There's still ReadAdvice.SEQUENTIAL, with corresponding DataAccessHint, which is used for a few things - whilst we need to consolidate uses of NORMAL vs SEQUENTIAL, I want to keep existing behaviour in this PR as much as as sensible, and modify it in subsequent PRs

I've taken another look at this, and reduced MMapDirectory down to only check DataAccessHint and FileTypeHint, without changing behavior.

lucene/test-framework/src/java/org/apache/lucene/tests/store/SerialIOCountingDirectory.java

ChrisHegarty

Thanks @thecoop - LGTM

Backport of apache#14510 to 10.x

…14510) Backport of #14510

Apply changes made between 10.2.2 and 10.3.1 versions: ``` $ g log --oneline releases/lucene/10.2.2..releases/lucene/10.3.1 lucene/core/src/java/org/apache/lucene/codecs/lucene90/{Lucene90DocValuesProducer.java,Lucene90DocValuesConsumer.java,Lucene90DocValuesFormat.java} d176a42b659 Implement IndexedDISI#docIDRunEnd (#14753) 223d7a41e3c [10.x] Change uses of withReadAdvice to use hints instead (#14629) (#14510) bb3167e57c6 Impl intoBitset for IndexedDISI and Docvalues (#14529) ``` Relevant PRs: apache/lucene#14753 apache/lucene#14510 apache/lucene#14529 Follows: #18545

github-project-automation bot added this to OpenSearch Lucene & Core Performance Tracking Apr 16, 2025

github-project-automation bot moved this to Open in OpenSearch Lucene & Core Performance Tracking Apr 16, 2025

github-actions bot added module:core/store module:core/index module:core/codecs module:test-framework module:misc labels Apr 16, 2025

thecoop force-pushed the read-advice-to-hints branch from fee3d2c to 4e1356b Compare April 17, 2025 09:55

jpountz mentioned this pull request Apr 28, 2025

Create file open hints on IOContext to replace ReadAdvice #14482

Merged

Remove ReadAdvice from IOContext

020aa4c

thecoop force-pushed the read-advice-to-hints branch from 6154e9b to 020aa4c Compare April 30, 2025 13:20

github-actions bot removed module:core/index module:misc labels Apr 30, 2025

thecoop commented Apr 30, 2025

View reviewed changes

lucene/test-framework/src/java/org/apache/lucene/tests/store/SerialIOCountingDirectory.java Outdated Show resolved Hide resolved

thecoop commented Apr 30, 2025

View reviewed changes

thecoop marked this pull request as ready for review April 30, 2025 13:45

Merge branch 'main' into read-advice-to-hints

2113edf

ChrisHegarty reviewed May 2, 2025

View reviewed changes

lucene/core/src/java/org/apache/lucene/store/MMapDirectory.java Outdated Show resolved Hide resolved

benwtrent mentioned this pull request May 2, 2025

[Bug] Fix for stored fields force merge regression #14512

Open

jpountz reviewed May 2, 2025

View reviewed changes

PR comments

62cb261

thecoop requested review from ChrisHegarty and jpountz May 6, 2025 13:57

These change behavior - revert

4a1164f

ChrisHegarty approved these changes May 8, 2025

View reviewed changes

thecoop mentioned this pull request May 8, 2025

Backport migration from ReadAdvice to hints to 10.x #14629

Merged

thecoop added a commit to thecoop/lucene that referenced this pull request May 8, 2025

Remove ReadAdvice from IOContext

890754d

Backport of apache#14510 to 10.x

thecoop added a commit to thecoop/lucene that referenced this pull request May 8, 2025

Remove ReadAdvice from IOContext

713587a

Backport of apache#14510 to 10.x

ChrisHegarty merged commit bec671e into apache:main May 9, 2025
7 checks passed

github-project-automation bot moved this from Open to Merged in OpenSearch Lucene & Core Performance Tracking May 9, 2025

thecoop deleted the read-advice-to-hints branch May 9, 2025 08:37

ChrisHegarty pushed a commit that referenced this pull request May 9, 2025

[10.x] Change uses of withReadAdvice to use hints instead (#14629) (#…

223d7a4

…14510) Backport of #14510

matriv mentioned this pull request Oct 21, 2025

Apply Lucene changes to CrateDB's CustomLucene90DocValuesProducer crate/crate#18554

Merged

Conversation

thecoop commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

thecoop Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thecoop May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thecoop May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ChrisHegarty left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thecoop commented Apr 16, 2025 •

edited

Loading

thecoop Apr 30, 2025 •

edited

Loading

thecoop May 2, 2025 •

edited

Loading

thecoop May 6, 2025 •

edited

Loading