imply-cheddar commented on code in PR #13501:
URL: https://github.com/apache/druid/pull/13501#discussion_r1041521020
##########
processing/src/main/java/org/apache/druid/query/DataSource.java:
##########
@@ -124,4 +124,7 @@
*/
byte[] getCacheKey();
+ DataSourceAnalysis getAnalysisForDataSource();
+
+ DataSourceAnalysis getAnalysisForDataSource(Query<?> query);
Review Comment:
I don't believe we need two methods. Just a single one that takes no
argument should be fine. My read of the old DataSourceAnalysis code is that it
just wants the final query. That means that we need the QueryDataSource to
just check the return object and attach the Query only if a query hasn't been
attached yet.
##########
processing/src/main/java/org/apache/druid/query/planning/DataSourceAnalysis.java:
##########
@@ -97,99 +90,15 @@ private DataSourceAnalysis(
if (baseDataSource instanceof JoinDataSource) {
// The base cannot be a join (this is a class invariant).
// If it happens, it's a bug in the datasource analyzer.
- throw new IAE("Base dataSource cannot be a join! Original dataSource
was: %s", dataSource);
+ throw new IAE("Base dataSource cannot be a join! ");
Review Comment:
Maybe interpolate the baseDataSource into the message instead?
##########
server/src/main/java/org/apache/druid/server/LocalQuerySegmentWalker.java:
##########
@@ -79,10 +79,10 @@ public LocalQuerySegmentWalker(
@Override
public <T> QueryRunner<T> getQueryRunnerForIntervals(final Query<T> query,
final Iterable<Interval> intervals)
{
- final DataSourceAnalysis analysis =
DataSourceAnalysis.forDataSource(query.getDataSource());
+ final DataSourceAnalysis analysis =
query.getDataSource().getAnalysisForDataSource();
- if (!analysis.isConcreteBased() || !analysis.isGlobal()) {
- throw new IAE("Cannot query dataSource locally: %s",
analysis.getDataSource());
+ if (!analysis.isConcreteBased() || !query.getDataSource().isGlobal()) {
+ throw new IAE("Cannot query dataSource locally: %s",
query.getDataSource());
Review Comment:
There are 3 calls to `query.getDataSource()` in quick succession here.
Might as well do it once and store the reference.
##########
server/src/main/java/org/apache/druid/client/BrokerServerView.java:
##########
@@ -353,7 +353,7 @@ public Optional<VersionedIntervalTimeline<String,
ServerSelector>> getTimeline(f
{
final TableDataSource table =
analysis.getBaseTableDataSource()
- .orElseThrow(() -> new ISE("Cannot handle datasource: %s",
analysis.getDataSource()));
+ .orElseThrow(() -> new ISE("Cannot handle datasource: %s",
analysis.getBaseDataSource()));
Review Comment:
Is the `BaseDataSource` now the old `DataSource`? Changing names like that
can be confusing.
##########
processing/src/main/java/org/apache/druid/query/QueryDataSource.java:
##########
@@ -112,6 +113,25 @@ public byte[] getCacheKey()
return null;
}
+ @Override
+ public DataSourceAnalysis getAnalysisForDataSource()
+ {
+ return getAnalysisForDataSource(null);
+ }
+
+ @Override
+ public DataSourceAnalysis getAnalysisForDataSource(Query<?> query)
+ {
+ final Query<?> subQuery = this.getQuery();
+ if (!(subQuery instanceof BaseQuery)) {
+ // We must verify that the subQuery is a BaseQuery, because it is
required to make "getBaseQuerySegmentSpec"
+ // work properly. All built-in query types are BaseQuery, so we only
expect this with funky extension queries.
Review Comment:
I didn't see a `getBaseQuerySegmentSpec` method on `BaseQuery`, is this
comment still legitimate?
--
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]
