You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "dweiss (via GitHub)" <gi...@apache.org> on 2024/09/25 08:55:22 UTC
[I] DataInput class can't be used with delegation pattern [lucene]
dweiss opened a new issue, #13820:
URL: https://github.com/apache/lucene/issues/13820
### Description
The change in #12841 brought this pair of methods to DataInput:
```
public final void readGroupVInts(long[] dst, int limit) throws IOException { ... }
protected void readGroupVInt(long[] dst, int offset) throws IOException { ... }
```
If one wishes to write a DataInput delegating to another, existing DataInput, it's no longer possible because the public method is final and the protected method cannot be invoked on the delegate (cross-package access). A corresponding method in DataOutput is not final.
I would remove the final modifier from the API for symmetry. Thoughts?
### Version and environment details
_No response_
--
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.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
Re: [I] DataInput class can't be used with delegation pattern [lucene]
Posted by "dweiss (via GitHub)" <gi...@apache.org>.
dweiss commented on issue #13820:
URL: https://github.com/apache/lucene/issues/13820#issuecomment-2373976921
The test added as part of #11868 is checking whether only abstract methods are overridden in filter classes. It's a different case here.
--
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
Re: [I] DataInput class can't be used with delegation pattern [lucene]
Posted by "dweiss (via GitHub)" <gi...@apache.org>.
dweiss closed issue #13820: DataInput class can't be used with delegation pattern
URL: https://github.com/apache/lucene/issues/13820
--
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
Re: [I] DataInput class can't be used with delegation pattern [lucene]
Posted by "dweiss (via GitHub)" <gi...@apache.org>.
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: 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
Re: [I] DataInput class can't be used with delegation pattern [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on issue #13820:
URL: https://github.com/apache/lucene/issues/13820#issuecomment-2373932348
+1 for API symmetry.
I thought we had reflection based unit tests to validate that our delegating `FilterXX` classes delegate all methods? Maybe we don't have a `FilterDataInput` so we don't test that ...
--
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
Re: [I] DataInput class can't be used with delegation pattern [lucene]
Posted by "dweiss (via GitHub)" <gi...@apache.org>.
dweiss commented on issue #13820:
URL: https://github.com/apache/lucene/issues/13820#issuecomment-2373937582
There is FilterIndexInput which extends IndexInput (extends DataInput). Interesting this hasn't been caught.
--
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