dweiss commented on issue #13820:
URL: https://github.com/apache/lucene/issues/13820#issuecomment-2375077701
I linked the suggested minimal PR above. I am not convinced the test in
TestFilterIndexInput.testOverrides is correct. The problem with delegation
pattern ("FilterXyz" classes) is that all public/API methods should be
delegated, otherwise there is a risk of using the default, possibly slower
implementation. java.io.FilterInputStream is an example of when it can go wrong:
```
public int read(byte[] b) throws IOException {
return read(b, 0, b.length);
}
public int read(byte[] b, int off, int len) throws IOException {
return in.read(b, off, len);
}
```
This is correct implementation. But when your "in" (delegate) has an
accelerated implementation of ```read(byte[])``` and, for some obscure reason,
a slow implementation of ```read(byte[], int, int)```, wrapping it with a
filter will cause all ```read(byte[])``` to be slow.
There are many, many examples like this. In DataInput's case, the
FilterIndexInput should also override all public methods and call the provided
delegate. An example of when it can go wrong: an IndexInput with a
super-accelerated ```readGroupVInt(long[] dst, int offset)``` (the protected
method called by ```readGroupVInts```), wrapped in a FilterIndexInput is no
longer reaching the accelerated code because any class subclassing
FilterIndexInput is effectively using the default implementation from
DataInput...
I think I'll leave correcting this until later - keeping this PR simple. It
is a real issue and a potential problem though.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
