kgyrtkirk commented on PR #15428:
URL: https://github.com/apache/druid/pull/15428#issuecomment-1833637842
* regarding `minTopNThreshold`: I was doing an equiv refactor - so that it
retains the old behaviour;
* if I change the limit to `2` it still runs just fine; however at
execution time [this
conditional](https://github.com/apache/druid/blob/7467d2c00d56793a03844252e43aaf5b631856b9/processing/src/main/java/org/apache/druid/query/topn/TopNQueryQueryToolChest.java#L631)
decides to run the other branch: so although it does not affect the plan
itself; it does affect the execution path choosen
* I also seen that `minTopNThreshold` can be [overriden in the
`queryContext`](https://github.com/apache/druid/blob/7467d2c00d56793a03844252e43aaf5b631856b9/processing/src/main/java/org/apache/druid/query/topn/TopNQueryQueryToolChest.java#L629-L630)
; so there is not much reason to configure it at the `framework` level; but I
was thinking there were whatever reasons....and I only wanted to fix this issue
* I have to say the
[usage](https://github.com/apache/druid/blob/7467d2c00d56793a03844252e43aaf5b631856b9/sql/src/test/java/org/apache/druid/sql/calcite/util/SqlTestFramework.java#L216-L226)
`minTopNThreshold` raises my elbows quite a bit - especially because in case
`mergeBufferCount` is not `0` it have no effect...
* I would rather put that change into a separate PR - it seems like it
will be trivial; but in case things got out of hand... it will be better to
have it separately
* I think `cannotVectorize` and `msqCompatible` are a bit different kind of
settings; as they define the expected behaviour of the test; or kinda like
obstacles it should be able to jump over :) do you have in mind something like:
`@TestConfigX(msqCompatible = false)` ? we could probably do something like
that at some point - but I'm not sure of the direct benefits of changing that
right away
* fyi: `cannotVectorize` also has an unexpected feature:
https://github.com/apache/druid/issues/15423 :)
> Can you add a Javadoc to this method
not sure which method :) but I've added some apidoc to
`SqlTestFrameworkConfig`
--
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]
