clintropolis commented on code in PR #19613:
URL: https://github.com/apache/druid/pull/19613#discussion_r3455104609
##########
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 wonder if instead of adding a new flag if this should just check
`ENABLE_DEBUG` context flag
##########
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:
why not just pass the `CursorBuildSpec` into this method? it looks like all
of the callers are getting it from there anyway
##########
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());
Review Comment:
might be nice to print the ordering the `CursorBuildSpec` wanted and the
ordering the projection provides
##########
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:
this and some of these other top level logs seem kind of redundant since the
methods they call also log when returning nulls, but i guess we sort of need
these here though because otherwise i'm not sure its obvious which caller of
which match method was the failing one.
This is probably ok for now since its just logs, but in the future we might
consider collecting these failure reasons instead of logging them when they
happen so that we can produce more concise messaging.
--
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]
