[Draft] Pluggable query engine interfaces + searcher and reader interfaces #19707
Conversation
bharath-techie
force-pushed
the
search-df-main-interface
branch
2 times, most recently
from
478b56d to
94237cc
Compare
Signed-off-by: bharath-techie <[email protected]>
bharath-techie
force-pushed
the
search-df-main-interface
branch
from
94237cc to
dc79a65
Compare
|
❌ Gradle check result for dc79a65: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
| */ | ||
| DataFormat getDataFormat(); | ||
|
|
||
| CompletableFuture<Void> closeSessionContext(long sessionContextId); |
There was a problem hiding this comment.
the API here seems off - why is dataformatcodec managing sessioncontext lifecycle?
There was a problem hiding this comment.
Yeah for this PR - there are no good examples, mainly today codec is only used for configuration and optimizers.
https://github.com/bharath-techie/OpenSearch/pull/47/files#diff-48735da23f1ac176775a474bd04cd70ec100c2e487d72fb9285e9146897ce043R153 - here is an example of how codec gets used.
There was a problem hiding this comment.
The close session context is not used , will remove it
| * Represents a data format. | ||
| */ | ||
| @ExperimentalApi | ||
| public interface DataFormat { |
There was a problem hiding this comment.
we've got two DataFormat interfaces here and in libs
There was a problem hiding this comment.
#19475 (comment) - facing this issue man - we might need to move dataFormatCodec to core.
There was a problem hiding this comment.
This i think is a bit tricky , if i move the codec to core, then pluggable engine specific stuff like session config etc also needs to be in core.
Do you have any suggestions ?
https://github.com/opensearch-project/OpenSearch/pull/19760/files - Kindly refer to this for what configuration details might be present for datafusion and parquet.
| * Represents a stream of record batches from a DataFusion query execution. | ||
| * This interface provides access to query results in a streaming fashion. | ||
| */ | ||
| public interface RecordBatchStream extends AutoCloseable { |
There was a problem hiding this comment.
I think we should move these components in libs under the DF plugin as thats the only place they are used?
There was a problem hiding this comment.
Yes, initially I had usage in parquet plugin but moved everything out and somehow this stayed in libs, so will move this out too.
CompletableFuture<RecordBatchStream> executeSubstraitQuery(long sessionContextId, byte[] substraitPlanBytes);
| /** | ||
| * OpenSearch plugin that provides Parquet data format support for indexing and query operations. | ||
| */ | ||
| public class ParquetDataFormatPlugin extends Plugin implements DataFormatPlugin { |
There was a problem hiding this comment.
i'm still struggling with the need for this plugin at a top level - can we not have dataformat pieces as a lib inside of engine specific plugins, ie engines provide indexer & searcher functionality directly.
There was a problem hiding this comment.
You're right searcher and reader are completely in fact in engine-datafusion , so we are already not using the format plugins for the same. As commented above, only configuration and optimizers , i expect to be in data format plugins.
But indexer / writer is a complete piece that sits in parquet plugin as its specific to parquet and agnostic of read engines.
2 participants
Description
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.