You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "cecemei (via GitHub)" <gi...@apache.org> on 2026/06/22 18:26:18 UTC
[PR] minor: add projectionTrace query context for diagnosing projection match failures (druid)
cecemei opened a new pull request, #19613:
URL: https://github.com/apache/druid/pull/19613
### Description
Debugging why `matchAggregateProjection` returns null is difficult today because there are 14 distinct rejection sites spread across the matching pipeline, and there's no visibility into which one fired without a debugger or global DEBUG logging.
This PR adds a `projectionTrace` query context flag. When set to true, rejection reasons are logged at INFO level so they're visible without changing log4j config. When unset, the same messages are emitted at DEBUG level.
Usage:
```
{
"context": { "projectionTrace": true }
}
```
Rejection messages include the projection name and the specific reason (e.g., incompatible ordering, missing column, granularity mismatch), making it straightforward to identify which projection was rejected and why.
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [ ] 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
Re: [PR] minor: add projectionTrace query context for diagnosing projection match failures (druid)
Posted by "cecemei (via GitHub)" <gi...@apache.org>.
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: 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
Re: [PR] minor: add projectionTrace query context for diagnosing projection match failures (druid)
Posted by "cecemei (via GitHub)" <gi...@apache.org>.
cecemei commented on code in PR #19613:
URL: https://github.com/apache/druid/pull/19613#discussion_r3456415744
##########
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:
good point, i added the logging if debug is set
--
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
Re: [PR] minor: add debug logs for diagnosing projection match failures (druid)
Posted by "cecemei (via GitHub)" <gi...@apache.org>.
cecemei merged PR #19613:
URL: https://github.com/apache/druid/pull/19613
--
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
Re: [PR] minor: add projectionTrace query context for diagnosing projection match failures (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #19613:
URL: https://github.com/apache/druid/pull/19613#discussion_r3457938708
##########
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
+1 to this. IMO, we should minimize the # of possible query context configuration combinations (there are frankly so many, some of which are documented and others not). My opinion: a user seeking debug information about a particular query should not need to go enable multiple query context flags to get the information they are looking for. For debug, I'd rather get the kitchen sink and then filter myself manually.
--
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
Re: [PR] minor: add projectionTrace query context for diagnosing projection match failures (druid)
Posted by "cecemei (via GitHub)" <gi...@apache.org>.
cecemei commented on code in PR #19613:
URL: https://github.com/apache/druid/pull/19613#discussion_r3456423960
##########
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:
yea it would be suboptimal if logging for a lot of segments, maybe could be helpful in the case when only several segments out of all are missing proper projections, it gives some amount of visibility and operatibility to projection at least.
--
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
Re: [PR] minor: add projectionTrace query context for diagnosing projection match failures (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #19613:
URL: https://github.com/apache/druid/pull/19613#discussion_r3457938708
##########
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
+1 to this. IMO, we should minimize the # of possible query context configuration combinations (there are frankly so many, some of which are documented and others not). My opinion: a user seeking debug information about a particular query should not need to go enable multiple query context flags to get the information they are looking for, _especially_ if they do not know what they are looking for (debugging). For debug, I'd rather get the kitchen sink and then filter myself manually.
--
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
Re: [PR] minor: add projectionTrace query context for diagnosing projection match failures (druid)
Posted by "cecemei (via GitHub)" <gi...@apache.org>.
cecemei commented on code in PR #19613:
URL: https://github.com/apache/druid/pull/19613#discussion_r3461585197
##########
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 guess if we filter by Projections class we could always reduce the noise. Updated this to use debug flag.
--
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
Re: [PR] minor: add projectionTrace query context for diagnosing projection match failures (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
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: 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
Re: [PR] minor: add projectionTrace query context for diagnosing projection match failures (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #19613:
URL: https://github.com/apache/druid/pull/19613#discussion_r3457938708
##########
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
+1 to this. IMO, we should minimize the # of possible query context configuration combinations (there are frankly so many, some of which are documented, others not). My opinion: a user seeking debug information about a particular query should not need to go enable multiple query context flags to get the information they are looking for. For debug, I'd rather get the kitchen sink and then filter myself manually.
--
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
Re: [PR] minor: add projectionTrace query context for diagnosing projection match failures (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #19613:
URL: https://github.com/apache/druid/pull/19613#discussion_r3456258253
##########
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:
yea, i considered the noise briefly too, but then said to myself it was ok though because the noisy SQL logs are all on the broker, while these logs are only going to be on the historicals and realtime servers, per segment.
If you really want to add a new flag, i still think we should also do this logging if `debug` is set, so that way someone wanting "all the logs" doesn't have to set a bunch of different flags, so the `logTrace` method could check either `projectionTrace` or `debug`
--
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
Re: [PR] minor: add projectionTrace query context for diagnosing projection match failures (druid)
Posted by "cecemei (via GitHub)" <gi...@apache.org>.
cecemei commented on code in PR #19613:
URL: https://github.com/apache/druid/pull/19613#discussion_r3455751063
##########
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 decided not because 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: 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
Re: [PR] minor: add projectionTrace query context for diagnosing projection match failures (druid)
Posted by "cecemei (via GitHub)" <gi...@apache.org>.
cecemei commented on code in PR #19613:
URL: https://github.com/apache/druid/pull/19613#discussion_r3455846085
##########
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:
this is 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
Re: [PR] minor: add projectionTrace query context for diagnosing projection match failures (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #19613:
URL: https://github.com/apache/druid/pull/19613#discussion_r3456259803
##########
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:
i guess it was just jarring to see `logTrace(queryCursorBuildSpec.getQueryContext()...` like 20 times in the same file with all the stuff sourced from a `CursorBuildSpec`.
--
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
Re: [PR] minor: add projectionTrace query context for diagnosing projection match failures (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #19613:
URL: https://github.com/apache/druid/pull/19613#discussion_r3456262914
##########
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:
yea, i think its fine for now, though 'only 1-2 lines' is _per projection, per segment_ of the query, so its going to be pretty verbose anywhere that is touching more than a few segments if there are any projections at all.
--
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
Re: [PR] minor: add projectionTrace query context for diagnosing projection match failures (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #19613:
URL: https://github.com/apache/druid/pull/19613#discussion_r3457938708
##########
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
+1 to this. IMO, we should minimize the # of possible query context configuration combinations. My opinion: a user seeking debug information about a particular query should not need to go enable multiple query context flags to get the information they are looking for. For debug, I'd rather get the kitchen sink and then filter myself manually.
--
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
Re: [PR] minor: add projectionTrace query context for diagnosing projection match failures (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #19613:
URL: https://github.com/apache/druid/pull/19613#discussion_r3457938708
##########
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
+1 to this. IMO, we should minimize the # of possible query context configuration combinations (there are frankly so many, some of which are documented and others not). My opinion: a user seeking debug information about a particular query should not need to go enable multiple query context flags to get the information they are looking for, _especially_ if they do not know what they are looking for (debugging). For debug, I'd rather get the kitchen sink and then filter out the noise myself manually.
--
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
Re: [PR] minor: add projectionTrace query context for diagnosing projection match failures (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #19613:
URL: https://github.com/apache/druid/pull/19613#discussion_r3457938708
##########
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
+1 to this. IMO, we should minimize the # of possible query context configuration combinations (there are frankly so many, some of which are documented and others not). My opinion: a user seeking debug information about a particular query should not need to go enable multiple query context flags to get the information they are looking for, _especially._ if they do not know what they are looking for. For debug, I'd rather get the kitchen sink and then filter myself manually.
--
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