You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/12/06 04:47:34 UTC
[GitHub] [druid] somu-imply opened a new pull request, #13501: Analysis refactor
somu-imply opened a new pull request, #13501:
URL: https://github.com/apache/druid/pull/13501
Work in progress for refactoring `analysis.forDataSource()` and moving analysis inside Individual data sources
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] imply-cheddar commented on pull request #13501: Analysis refactor
Posted by GitBox <gi...@apache.org>.
imply-cheddar commented on PR #13501:
URL: https://github.com/apache/druid/pull/13501#issuecomment-1343706487
It's complaining that parts of the if conditions that you changed when you adjusted the `isQuery()` are not covered in the tests. It needs a branch coverage of 50%, currently 11 of 24 branches are covered. So you just need one more branch covered. Just for posterity, it's probably great to try to find a way to cover at least one of those branches...
It looks like there is a `ServerManagerTest` and a `ClientQuerySegmentWalkerTest`. It is probably worth it to just look and see if there's a relatively straightforward way to build tests for one/both of those classes that get the exception to be thrown.
Other than that, i think this is ready for merge.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] imply-cheddar commented on a diff in pull request #13501: Analysis refactor
Posted by GitBox <gi...@apache.org>.
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] cheddar commented on pull request #13501: Analysis refactor
Posted by GitBox <gi...@apache.org>.
cheddar commented on PR #13501:
URL: https://github.com/apache/druid/pull/13501#issuecomment-1347623942
@somu-imply when I went to merge, I realized that none of your commit messages really made for a good commit message on the whole PR. Going forward, try to make your initial commit message contain enough context for it to make a good final commit message. That'll also make your PR creation time faster as Github will grab the initial commit message and use it for a description.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] somu-imply commented on a diff in pull request #13501: Analysis refactor
Posted by GitBox <gi...@apache.org>.
somu-imply commented on code in PR #13501:
URL: https://github.com/apache/druid/pull/13501#discussion_r1042563236
##########
processing/src/main/java/org/apache/druid/query/JoinDataSource.java:
##########
@@ -479,4 +481,47 @@ public byte[] getCacheKey()
}
return keyBuilder.build();
}
+
+ @Override
+ public DataSourceAnalysis getAnalysisForDataSource()
Review Comment:
The one in the constructor was calling this only. Made a slight change to have the analysis in the constructor call a private method to populate the analysis and this public method just to return the one in the constructor
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] somu-imply commented on pull request #13501: Analysis refactor
Posted by GitBox <gi...@apache.org>.
somu-imply commented on PR #13501:
URL: https://github.com/apache/druid/pull/13501#issuecomment-1343103356
The branch coverage is failing on me for server module although my refactor just renamed a couple of things there
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] imply-cheddar commented on a diff in pull request #13501: Analysis refactor
Posted by GitBox <gi...@apache.org>.
imply-cheddar commented on code in PR #13501:
URL: https://github.com/apache/druid/pull/13501#discussion_r1042118533
##########
processing/src/main/java/org/apache/druid/query/QueryDataSource.java:
##########
@@ -112,6 +113,25 @@ public byte[] getCacheKey()
return null;
}
+ @Override
+ public DataSourceAnalysis getAnalysisForDataSource()
+ {
+ final Query<?> subQuery = this.getQuery();
+ if (!(subQuery instanceof BaseQuery)) {
+ // We must verify that the subQuery is a BaseQuery, because it is required to make
+ // "DataSourceAnalysis.getBaseQuerySegmentSpec" work properly.
+ // All built-in query types are BaseQuery, so we only expect this with funky extension queries.
+ throw new IAE("Cannot analyze subquery of class[%s]", subQuery.getClass().getName());
+ }
+ final DataSource current = subQuery.getDataSource();
+ DataSourceAnalysis currentAnalysis = current.getAnalysisForDataSource();
+ // check if the baseQuery is already present while moving up
+ // if not add the current query as the baseQuery to the analysis
+ if (!currentAnalysis.getBaseQuery().isPresent()) {
Review Comment:
It would be safer for `DataSourceAnalysis` to encapsulate this check. Maybe name the method `maybeWithBaseQuery(subQuery)` and move the if statement inside of that.
##########
processing/src/main/java/org/apache/druid/query/JoinDataSource.java:
##########
@@ -479,4 +481,47 @@ public byte[] getCacheKey()
}
return keyBuilder.build();
}
+
+ @Override
+ public DataSourceAnalysis getAnalysisForDataSource()
Review Comment:
The result of this is cached in the constructor, is there a reason not to return that and move the logic into a private method?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] somu-imply commented on a diff in pull request #13501: Analysis refactor
Posted by GitBox <gi...@apache.org>.
somu-imply commented on code in PR #13501:
URL: https://github.com/apache/druid/pull/13501#discussion_r1042562174
##########
processing/src/main/java/org/apache/druid/query/QueryDataSource.java:
##########
@@ -112,6 +113,25 @@ public byte[] getCacheKey()
return null;
}
+ @Override
+ public DataSourceAnalysis getAnalysisForDataSource()
+ {
+ final Query<?> subQuery = this.getQuery();
+ if (!(subQuery instanceof BaseQuery)) {
+ // We must verify that the subQuery is a BaseQuery, because it is required to make
+ // "DataSourceAnalysis.getBaseQuerySegmentSpec" work properly.
+ // All built-in query types are BaseQuery, so we only expect this with funky extension queries.
+ throw new IAE("Cannot analyze subquery of class[%s]", subQuery.getClass().getName());
+ }
+ final DataSource current = subQuery.getDataSource();
+ DataSourceAnalysis currentAnalysis = current.getAnalysisForDataSource();
+ // check if the baseQuery is already present while moving up
+ // if not add the current query as the baseQuery to the analysis
+ if (!currentAnalysis.getBaseQuery().isPresent()) {
Review Comment:
Great idea. Done
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] cheddar merged pull request #13501: Analysis refactor
Posted by GitBox <gi...@apache.org>.
cheddar merged PR #13501:
URL: https://github.com/apache/druid/pull/13501
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] somu-imply commented on a diff in pull request #13501: Analysis refactor
Posted by GitBox <gi...@apache.org>.
somu-imply commented on code in PR #13501:
URL: https://github.com/apache/druid/pull/13501#discussion_r1041774418
##########
processing/src/main/java/org/apache/druid/query/DataSource.java:
##########
@@ -124,4 +124,7 @@
*/
byte[] getCacheKey();
+ DataSourceAnalysis getAnalysisForDataSource();
+
+ DataSourceAnalysis getAnalysisForDataSource(Query<?> query);
Review Comment:
Addressed, query data source now populates the baseQuery while moving up the tree. A method to update with new analysis object has been added
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] imply-cheddar commented on a diff in pull request #13501: Analysis refactor
Posted by GitBox <gi...@apache.org>.
imply-cheddar commented on code in PR #13501:
URL: https://github.com/apache/druid/pull/13501#discussion_r1042794855
##########
processing/src/main/java/org/apache/druid/query/JoinDataSource.java:
##########
@@ -482,13 +482,18 @@ public byte[] getCacheKey()
return keyBuilder.build();
}
- @Override
- public DataSourceAnalysis getAnalysisForDataSource()
+ private DataSourceAnalysis getAnalysis()
{
final Triple<DataSource, DimFilter, List<PreJoinableClause>> flattened = flattenJoin(this);
return new DataSourceAnalysis(flattened.first, null, flattened.second, flattened.third);
}
+ @Override
+ public DataSourceAnalysis getAnalysisForDataSource()
Review Comment:
This rename made me realize... I like the name `getAnalysis()` a lot more for the method on the interface. Can we rename that?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] somu-imply commented on a diff in pull request #13501: Analysis refactor
Posted by GitBox <gi...@apache.org>.
somu-imply commented on code in PR #13501:
URL: https://github.com/apache/druid/pull/13501#discussion_r1041642557
##########
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:
This is interpolating the base data source into the message. Maybe I need to update the message as the original analysis.getDataSource() does not exist anymore
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] somu-imply commented on pull request #13501: Analysis refactor
Posted by GitBox <gi...@apache.org>.
somu-imply commented on PR #13501:
URL: https://github.com/apache/druid/pull/13501#issuecomment-1344608148
Tests have been added and now it passes Travis. This should be in a good shape to be approved and merged
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] somu-imply commented on a diff in pull request #13501: Analysis refactor
Posted by GitBox <gi...@apache.org>.
somu-imply commented on code in PR #13501:
URL: https://github.com/apache/druid/pull/13501#discussion_r1042957503
##########
processing/src/main/java/org/apache/druid/query/JoinDataSource.java:
##########
@@ -482,13 +482,18 @@ public byte[] getCacheKey()
return keyBuilder.build();
}
- @Override
- public DataSourceAnalysis getAnalysisForDataSource()
+ private DataSourceAnalysis getAnalysis()
{
final Triple<DataSource, DimFilter, List<PreJoinableClause>> flattened = flattenJoin(this);
return new DataSourceAnalysis(flattened.first, null, flattened.second, flattened.third);
}
+ @Override
+ public DataSourceAnalysis getAnalysisForDataSource()
Review Comment:
Done :)
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] somu-imply commented on a diff in pull request #13501: Analysis refactor
Posted by GitBox <gi...@apache.org>.
somu-imply commented on code in PR #13501:
URL: https://github.com/apache/druid/pull/13501#discussion_r1041774694
##########
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:
The comment has been updated to point to `DataSourceAnalysis. getBaseQuerySegmentSpec`
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org