Copilot commented on code in PR #17844:
URL: https://github.com/apache/pinot/pull/17844#discussion_r2922434271
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/ForwardIndexReaderContext.java:
##########
@@ -29,13 +27,6 @@
*/
public interface ForwardIndexReaderContext extends AutoCloseable {
- /**
- * Applies query-level options to this context so that reader
implementations can adjust behavior per-query
- * (e.g., bypassing caches). The default implementation is a no-op for
backward compatibility.
- */
- default void applyQueryOptions(Map<String, String> queryOptions) {
- }
-
@Override
void close();
Review Comment:
`ForwardIndexReaderContext` drops the `applyQueryOptions(Map<String,
String>)` default method. Even though it was a no-op here, removing a method
from a public SPI interface is a breaking API/binary change for any external
ForwardIndexReaderContext implementations/consumers compiled against the
previous version. Consider keeping `applyQueryOptions` (possibly deprecated) or
providing a deprecation/migration path (e.g., retain it and have
`ForwardIndexReader#createContext(Map)` call it on the returned context for
backward compatibility).
##########
pinot-core/src/main/java/org/apache/pinot/core/common/DataFetcher.java:
##########
@@ -325,10 +325,7 @@ private class ColumnValueReader implements AutoCloseable {
private ForwardIndexReaderContext getReaderContext() {
if (!_readerContextCreated) {
- _readerContext = _reader.createContext();
- if (_readerContext != null) {
- _readerContext.applyQueryOptions(_queryOptions);
- }
+ _readerContext = _reader.createContext(_queryOptions);
_readerContextCreated = true;
}
Review Comment:
Query options are now threaded into forward-index context creation via
`createContext(_queryOptions)`, but there’s no unit test ensuring the query
options passed into `DataFetcher` actually reach
`ForwardIndexReader#createContext(Map)` (and not the no-arg overload). Adding a
focused Mockito-based test (mock DataSource/ForwardIndexReader and verify the
Map is forwarded) would prevent regressions in this propagation path.
--
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]
