Fix druid sql group by queries returning complex aggregation type#8099
Fix druid sql group by queries returning complex aggregation type#8099gianm merged 2 commits intoapache:masterfrom
Conversation
gianm
left a comment
gianm
left a comment
There was a problem hiding this comment.
Main code looks good, just had one comment about a needless check.
Could you also include a regression unit test that fails in master but passes with this patch? Maybe BloomFilterSqlAggregatorTest would be a good spot to put it.
I'd suggest including a comment like,
// Regression test for https://github.com/apache/incubator-druid/issues/8093.
| ); | ||
| // We don't really have a way to cast complex type. So might as well not do anything and return. | ||
| final ValueType columnValueType = aggregateRowSignature.getColumnType(expression.getDirectColumn()); | ||
| if (expression.isDirectColumnAccess() && columnValueType == ValueType.COMPLEX) { |
There was a problem hiding this comment.
expression.isDirectColumnAccess() is already known true due to the check above, so isn't needed here.
There was a problem hiding this comment.
Main code looks good, just had one comment about a needless check.
Could you also include a regression unit test that fails in master but passes with this patch? Maybe BloomFilterSqlAggregatorTest would be a good spot to put it.
I'd suggest including a comment like,
// Regression test for https://github.com/apache/incubator-druid/issues/8093.
Removed the check.
I have a PR out for SQL support for t-digests that has a test testGeneratingSketchAndComputingQuantileOnFly (952a553#diff-506b4b10165c4dd7af44701956a3cea5R267) that fails without this patch.
There was a problem hiding this comment.
That sounds fine, but could you please add the comment to the other PR's test then? The idea is that if a test is verifying that a bug was fixed it's good for it to link to the original issue.
3 participants
Fixes #8093 .
@gianm - thanks for your suggestion. That fixed the issue. Please review when you get a chance.