kfaraz commented on code in PR #17724:
URL: https://github.com/apache/druid/pull/17724#discussion_r1957027124
##########
server/src/main/java/org/apache/druid/segment/realtime/appenderator/SinkQuerySegmentWalker.java:
##########
@@ -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)) {
Review Comment:
```suggestion
if (!analysis.hasBaseTableDataSource() ||
!analysis.getBaseTableDataSource().getName().equals(dataSource)) {
```
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/SingleTaskBackgroundRunner.java:
##########
@@ -388,11 +388,10 @@ private <T> QueryRunner<T> getQueryRunnerImpl(Query<T>
query)
QueryRunner<T> queryRunner = null;
if (runningItem != null) {
- final DataSourceAnalysis analysis = query.getDataSource().getAnalysis();
final Task task = runningItem.getTask();
+ final TableDataSource queryTable =
query.getDataSourceAnalysis().getBaseTableDataSource();
Review 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?
##########
processing/src/main/java/org/apache/druid/query/planning/DataSourceAnalysis.java:
##########
@@ -110,17 +111,19 @@ public DataSource getBaseDataSource()
}
/**
- * If {@link #getBaseDataSource()} is a {@link TableDataSource}, returns it.
Otherwise, returns an empty Optional.
+ * Unwraps the {@link #getBaseDataSource()} if its a {@link TableDataSource}.
*
- * Note that this can return empty even if {@link
#isConcreteAndTableBased()} is true. This happens if the base
+ * @throws An error of type {@link DruidException.Category#DEFENSIVE} if the
{@link BaseDataSource} is not a table.
+ *
+ * note that this may not be true even {@link #isConcreteAndTableBased()} is
true - in cases when the base
* datasource is a {@link UnionDataSource} of {@link TableDataSource}.
*/
- public Optional<TableDataSource> getBaseTableDataSource()
+ public TableDataSource getBaseTableDataSource()
Review Comment:
Maybe we should also add a method `hasBaseTableDatasource()`, otherwise some
callers need to do an `instanceof` check.
--
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]
