kgyrtkirk commented on code in PR #17724:
URL: https://github.com/apache/druid/pull/17724#discussion_r1957971281
##########
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:
I feel like that won't be much different:
I think:
* earlier the return type for `getBaseTableDataSource` was an `Optional` ;
if it returned `empty` - it returned the below error; the new
`getBaseTableDataSource` is not an `Optional` and 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
##########
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:
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.
##########
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:
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?
--
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]
