cecemei commented on code in PR #19613:
URL: https://github.com/apache/druid/pull/19613#discussion_r3455833040
##########
processing/src/main/java/org/apache/druid/segment/projections/Projections.java:
##########
@@ -67,6 +69,17 @@
public class Projections
{
+ private static final Logger log = new Logger(Projections.class);
+
+ private static void logTrace(QueryContext context, String format, Object...
args)
Review Comment:
a narrower input param would be more declarative? yes all callers are
getting it from `CursorBuildSpec`.
##########
processing/src/main/java/org/apache/druid/segment/projections/Projections.java:
##########
@@ -161,35 +174,42 @@ public static ProjectionMatch matchAggregateProjection(
)
{
if
(!queryCursorBuildSpec.isCompatibleOrdering(projection.getOrderingWithTimeColumnSubstitution()))
{
+ logTrace(queryCursorBuildSpec.getQueryContext(),
"matchAggregateProjection: projection [%s] rejected — incompatible ordering",
projection.getName());
return null;
}
if
(CollectionUtils.isNullOrEmpty(queryCursorBuildSpec.getPhysicalColumns())) {
+ logTrace(queryCursorBuildSpec.getQueryContext(),
"matchAggregateProjection: projection [%s] rejected — no physical columns in
query", projection.getName());
return null;
}
if (isUnalignedInterval(projection, queryCursorBuildSpec, dataInterval)) {
+ logTrace(queryCursorBuildSpec.getQueryContext(),
"matchAggregateProjection: projection [%s] rejected — unaligned interval",
projection.getName());
return null;
}
ProjectionMatchBuilder matchBuilder = new ProjectionMatchBuilder();
// match virtual columns first, which will populate the 'remapColumns' of
the match builder
matchBuilder = matchQueryVirtualColumns(projection, queryCursorBuildSpec,
physicalColumnChecker, matchBuilder);
if (matchBuilder == null) {
+ logTrace(queryCursorBuildSpec.getQueryContext(),
"matchAggregateProjection: projection [%s] rejected — virtual column mismatch",
projection.getName());
Review Comment:
we could consider collecting failure reasons when forceProjection or
useProjection is set in the future, and maybe add it as a system flag.
even though many messages are added here, only 1-2 lines would be logged so
it should not be too verbose.
##########
processing/src/main/java/org/apache/druid/query/QueryContexts.java:
##########
@@ -126,6 +126,7 @@ public class QueryContexts
public static final String NO_PROJECTIONS = "noProjections";
public static final String FORCE_PROJECTION = "forceProjections";
public static final String USE_PROJECTION = "useProjection";
+ public static final String PROJECTION_TRACE = "projectionTrace";
Review Comment:
i've thought about that as well, but considered not because 1. debug=true
already triggers a lot of things at once, e.x SQL planner rule,
CalciteRulesManager in particular is very noisy when it logs planner rules.
It'd be very noisy if someone is just trying to understand why a projection
isn't matching.
--
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]
