You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "kgyrtkirk (via GitHub)" <gi...@apache.org> on 2024/07/30 09:49:52 UTC
[I] Remove redundant TopNQueryConfig#minTopNThreshold (druid)
kgyrtkirk opened a new issue, #16817:
URL: https://github.com/apache/druid/issues/16817
### Description
Only setting which could be done in it is [minTopNThreshold](https://github.com/apache/druid/blob/954aaafe0c85c1f4967bdb5798c17d4dc813ddd4/processing/src/main/java/org/apache/druid/query/topn/TopNQueryConfig.java#L36)
However that config setting value can be overriden by the context variable [minTopNThreshold](https://druid.apache.org/docs/latest/querying/query-context/#topn) - which is applied in every cases [here](https://github.com/apache/druid/blob/954aaafe0c85c1f4967bdb5798c17d4dc813ddd4/processing/src/main/java/org/apache/druid/query/topn/TopNQueryQueryToolChest.java#L637-L638)
### Motivation
The query context approach is more flexible:
* can be changed for a single query as well
* could still be set globally via runtime properties
Possibly the whole `TopNQueryConfig` class can be removed as the class will be empty without this field.
--
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.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: [I] Remove redundant TopNQueryConfig#minTopNThreshold (druid)
Posted by "itsautfullday (via GitHub)" <gi...@apache.org>.
itsautfullday commented on issue #16817:
URL: https://github.com/apache/druid/issues/16817#issuecomment-2367272606
Hello just dropping an update, I'll be trying to have the MR submitted by 30th Sept EOD IST
--
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: [I] Remove redundant TopNQueryConfig#minTopNThreshold (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm closed issue #16817: Remove redundant TopNQueryConfig#minTopNThreshold
URL: https://github.com/apache/druid/issues/16817
--
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: [I] Remove redundant TopNQueryConfig#minTopNThreshold (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on issue #16817:
URL: https://github.com/apache/druid/issues/16817#issuecomment-2358344864
oh course! feel free to submit a PR !
The above things look reasonable to me :)
It will most likely flow to a few places...but I guess in most cases you will just have to remove those things :crossed_fingers:
I think you could keep the `TopNQueryConfig.DEFAULT_MIN_TOPN_THRESHOLD` where it is now - or move it over to `QueryContexts` ; there are quite a few `DEFAULT_*` fields there.
I believe along the way you'll have to also remove `SqlTestFrameworkConfig#MinTopNThreshold`; and every place it was used set it via the querycontext parameter to the same value.
Don't hesitate to submit a draft PR and ask for help :)
--
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: [I] Remove redundant TopNQueryConfig#minTopNThreshold (druid)
Posted by "itsautfullday (via GitHub)" <gi...@apache.org>.
itsautfullday commented on issue #16817:
URL: https://github.com/apache/druid/issues/16817#issuecomment-2358209020
Hello! Would it be possible for me to contribute to this? I was thinking of removing the function call to getMinTopNThreshold() and replace it with TopNQueryConfig.DEFAULT_MIN_TOPN_THRESHOLD in the TopNQueryQueryToolChest.java class. I would also like to point out that this class and getMinTopNThreshold method are specifically getting overriden to provide a value from minTopNThresholdSupplier. It might be that the point of this function is this testing based subclass and overriding?
--
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: [I] Remove redundant TopNQueryConfig#minTopNThreshold (druid)
Posted by "itsautfullday (via GitHub)" <gi...@apache.org>.
itsautfullday commented on issue #16817:
URL: https://github.com/apache/druid/issues/16817#issuecomment-2359979577
Thanks! I'll try to have the MR submitted by 21st Sept 24 EOD IST
--
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