Re: [PR] Remove redundant TopNQueryConfig#minTopNThreshold (druid)

2026-03-20 Thread via GitHub 


gianm merged PR #18790:
URL: https://github.com/apache/druid/pull/18790


-- 
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]

Re: [PR] Remove redundant TopNQueryConfig#minTopNThreshold (druid)

2026-03-11 Thread via GitHub 


zhan7236 commented on PR #18790:
URL: https://github.com/apache/druid/pull/18790#issuecomment-4037371290

   Fixed the CI failure in `testExactTopNOnInnerJoinWithLimit`. The root cause 
was an incorrect CRC hash in the quidem testcase header.
   
   The previous commit correctly moved `!set minTopNThreshold 1` to its 
alphabetically sorted position (as suggested), but the CRC was computed 
manually without actually running the test, resulting in an incorrect hash 
(`b5816ff1` instead of the correct `f16f81b5`).
   
   Regenerated the file using `-Dquidem.overwrite` and verified the test passes 
locally.


-- 
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]

Re: [PR] Remove redundant TopNQueryConfig#minTopNThreshold (druid)

2026-03-10 Thread via GitHub 


zhan7236 commented on PR #18790:
URL: https://github.com/apache/druid/pull/18790#issuecomment-4035868400

   > @zhan7236 would you like to make the above change?
   
   Sure, I'll update the PR based on your suggestions. Thanks!


-- 
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]

Re: [PR] Remove redundant TopNQueryConfig#minTopNThreshold (druid)

2026-03-10 Thread via GitHub 


gianm commented on PR #18790:
URL: https://github.com/apache/druid/pull/18790#issuecomment-4033877374

   @zhan7236 would you like to make the above change?


-- 
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]

Re: [PR] Remove redundant TopNQueryConfig#minTopNThreshold (druid)

2026-03-03 Thread via GitHub 


gianm commented on PR #18790:
URL: https://github.com/apache/druid/pull/18790#issuecomment-3992314504

   I see a failure in 
`src/test/quidem/org.apache.druid.sql.calcite.DecoupledPlanningCalciteJoinQueryTest/testExactTopNOnInnerJoinWithLimit.iq`,
 which uses `minTopNThreshold`. We may be able to fix it by replacing this:
   
   ```
   !use druidtest:///?minTopNThreshold=1
   ```
   
   With this:
   
   ```
   !set minTopNThreshold 1
   !use druidtest:///
   ```
   
   Which would set it in the context.


-- 
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]

Re: [PR] Remove redundant TopNQueryConfig#minTopNThreshold (druid)

2026-03-02 Thread via GitHub 


gianm commented on PR #18790:
URL: https://github.com/apache/druid/pull/18790#issuecomment-3989352199

   Pushed an empty commit to the branch so the github actions can run.


-- 
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]

Re: [PR] Remove redundant TopNQueryConfig#minTopNThreshold (druid)

2026-03-02 Thread via GitHub 


gianm closed pull request #18790: Remove redundant 
TopNQueryConfig#minTopNThreshold
URL: https://github.com/apache/druid/pull/18790


-- 
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]

Re: [PR] Remove redundant TopNQueryConfig#minTopNThreshold (druid)

2026-01-30 Thread via GitHub 


gianm commented on code in PR #18790:
URL: https://github.com/apache/druid/pull/18790#discussion_r2748899824


##
docs/configuration/index.md:
##
@@ -2223,9 +2223,11 @@ context). If query does have `maxQueuedBytes` in the 
context, then that value is
 
 ### TopN query config
 
+The `druid.query.topN.minTopNThreshold` configuration property has been 
deprecated. Use the `minTopNThreshold` query context parameter instead.

Review Comment:
   This patch appears to remove it, not just deprecate it. I think it's fine, 
but the docs should be removed too, and a release note should be added. (Use 
the "Release notes" label and draft one in the PR 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: [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]

Re: [PR] Remove redundant TopNQueryConfig#minTopNThreshold (druid)

2026-01-30 Thread via GitHub 


github-actions[bot] commented on PR #18790:
URL: https://github.com/apache/druid/pull/18790#issuecomment-3826743964

   This pull request has been marked as stale due to 60 days of inactivity.
   It will be closed in 4 weeks if no further activity occurs. If you think
   that's incorrect or this pull request should instead be reviewed, please 
simply
   write any comment. Even if closed, you can still revive the PR at any time or
   discuss it on the [email protected] list.
   Thank you for your contributions.


-- 
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]

Re: [PR] Remove redundant TopNQueryConfig#minTopNThreshold (druid)

2025-11-30 Thread via GitHub 


github-advanced-security[bot] commented on code in PR #18790:
URL: https://github.com/apache/druid/pull/18790#discussion_r2575122489


##
server/src/test/java/org/apache/druid/server/QueryStackTests.java:
##
@@ -365,10 +376,23 @@
 return conglomerate;
   }
 
+  /**
+   * @deprecated The minTopNThreshold parameter is no longer used. Use query 
context to set minTopNThreshold.
+   */
+  @Deprecated
+  public static QueryRunnerFactoryConglomerate 
createQueryRunnerFactoryConglomerate(
+  final DruidProcessingConfig processingConfig,
+  final Integer minTopNThreshold,

Review Comment:
   ## Useless parameter
   
   The parameter 'minTopNThreshold' is never used.
   
   [Show more 
details](https://github.com/apache/druid/security/code-scanning/10571)



##
server/src/test/java/org/apache/druid/server/QueryStackTests.java:
##
@@ -272,25 +272,28 @@
* Returns a new {@link QueryRunnerFactoryConglomerate}. Adds relevant 
closeables to the passed-in {@link Closer}.
*/
   public static QueryRunnerFactoryConglomerate 
createQueryRunnerFactoryConglomerate(final Closer closer)
-  {
-return createQueryRunnerFactoryConglomerate(closer, 
TopNQueryConfig.DEFAULT_MIN_TOPN_THRESHOLD);
-  }
-
-  public static QueryRunnerFactoryConglomerate 
createQueryRunnerFactoryConglomerate(
-  final Closer closer,
-  final Integer minTopNThreshold
-  )
   {
 return createQueryRunnerFactoryConglomerate(
 closer,
 getProcessingConfig(
 DEFAULT_NUM_MERGE_BUFFERS
 ),
-minTopNThreshold,
 TestHelper.makeJsonMapper()
 );
   }
 
+  /**
+   * @deprecated The minTopNThreshold parameter is no longer used. Use query 
context to set minTopNThreshold.
+   */
+  @Deprecated
+  public static QueryRunnerFactoryConglomerate 
createQueryRunnerFactoryConglomerate(
+  final Closer closer,
+  final Integer minTopNThreshold

Review Comment:
   ## Useless parameter
   
   The parameter 'minTopNThreshold' is never used.
   
   [Show more 
details](https://github.com/apache/druid/security/code-scanning/10572)



-- 
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]