Reduce usage of DataSourceAnalysis in interfaces #17724
Conversation
github-actions
bot
added
Area - Batch Ingestion
Area - Querying
Area - Ingestion
Area - MSQ
| * datasource is a {@link UnionDataSource} of {@link TableDataSource}. | ||
| */ | ||
| public Optional<TableDataSource> getBaseTableDataSource() | ||
| public TableDataSource getBaseTableDataSource() |
There was a problem hiding this comment.
Maybe we should also add a method hasBaseTableDatasource(), otherwise some callers need to do an instanceof check.
There was a problem hiding this comment.
I've removed the instanceof from Queries ; as I think that was aiming as well on an exception which could be the one this method produces as well.
| @@ -182,7 +182,7 @@ public <T> QueryRunner<T> getQueryRunnerForSegments(final Query<T> query, final | |||
| final DataSourceAnalysis analysis = dataSourceFromQuery.getAnalysis(); | |||
|
|
|||
| // Sanity check: make sure the query is based on the table we're meant to handle. | |||
| if (!analysis.getBaseTableDataSource().filter(ds -> dataSource.equals(ds.getName())).isPresent()) { | |||
| if (!analysis.getBaseTableDataSource().getName().equals(dataSource)) { | |||
There was a problem hiding this comment.
| if (!analysis.getBaseTableDataSource().getName().equals(dataSource)) { | |
| if (!analysis.hasBaseTableDataSource() || !analysis.getBaseTableDataSource().getName().equals(dataSource)) { |
There was a problem hiding this comment.
I feel like that won't be much different:
I think:
- earlier the return type for
getBaseTableDataSourcewas anOptional; if it returnedempty- it returned the below error; the newgetBaseTableDataSourceis not anOptionaland has a check inside it; this leaves that case to that. - if there were no match to the filter it become empty that way and returned the next error - now this become the equals check
| final Task task = runningItem.getTask(); | ||
| final TableDataSource queryTable = query.getDataSourceAnalysis().getBaseTableDataSource(); |
There was a problem hiding this comment.
The original code was also checking if getBaseTableDataSource() is present.
If it is absent, the original code was no-op but now we will throw an exception.
Is this okay?
There was a problem hiding this comment.
I think this runner was tasked with running that particular query in this case...I don't think it may back-out silently with a noop if it doesn't like the task... I think that may possibly leading to missing results
The part submitting these tasks should have been able to decide if this a good idea or not...but there are no test failures - so I think it was doing the right thing.
Could you imagine a valid usecase for it?
There was a problem hiding this comment.
No, I am not sure myself. But I guess if there are no test failures, we should be good.
processing/src/main/java/org/apache/druid/query/planning/DataSourceAnalysis.java
Outdated
Show resolved
Hide resolved
…ourceAnalysis.java Co-authored-by: Kashif Faraz <[email protected]>
|
Thank you for reviewing the changes @kfaraz! |
TableDataSourceinTimelineServerView#getTimelineandSegmentManager#getTimelineanalysis.getBaseDataSource()throws instead of returning anOptional