Support complex variance object inputs for variance SQL agg function#14463
Support complex variance object inputs for variance SQL agg function#14463jon-wei merged 4 commits intoapache:masterfrom
Conversation
jon-wei
force-pushed
the
variance_sql_fix
branch
from
c772a77 to
7b390e8
Compare
abhishekagarwal87
left a comment
abhishekagarwal87
left a comment
There was a problem hiding this comment.
Some minor comments. but lgtm otherwise
| ); | ||
| } | ||
|
|
||
| private static class VarPopSqlAggFunction extends SqlAggFunction |
There was a problem hiding this comment.
can you add a note at the top that these various aggregate functions are same as SqlAvgAggFunction except that these versions pass OperandTypes.Any while standard function in calcite pass OperandTypes.Numeric
There was a problem hiding this comment.
Changed this to use aggregatorBuilder and added a comment about this
There was a problem hiding this comment.
I think a lot of boilerplate code here can be removed if you add a class called DruidSqlAvgAggFunction and in that class, the constructor passes the Any operand type. Then all the variance and std dev aggregation functions can extend that class and simply pass the kind value
There was a problem hiding this comment.
I think the approach of #14249 which is adding the SqlAggFunction equivalent of OperatorConversions.OperatorBuilder is probably the better way to do this
There was a problem hiding this comment.
that's replacing the constructor with fluent API but you will still need to reuse stuff.
There was a problem hiding this comment.
yeah, I guess i was thinking with that pattern can just have a function that makes SqlAggFunction based on the parts that change instead of a bunch of separate classes. The other PR is removing many of the custom SqlAggFunction to replace with the builder so making a more complicated setup here with base class seems like it would conflict even more with the goals there
There was a problem hiding this comment.
Changed this have a function that makes SqlAggFunction and uses the aggregator builder
| SqlKind.VAR_POP, | ||
| ReturnTypes.AVG_AGG_FUNCTION, | ||
| null, | ||
| OperandTypes.ANY, // Can be more specific after https://github.com/apache/druid/pull/14195 is merged |
There was a problem hiding this comment.
does this mean instead of OperandTypes.ANY you would use OperandTypes.or to check for either numeric or RowSignatures.complexTypeChecker with the appropriate native complex type? Any reason not to pull that change into this PR? it seems generally useful for validating any complex input types
There was a problem hiding this comment.
oh i should have read the PR description, you covered this 😛
I still think it would be nicer to pull in those changes since this PR seems to be easier to merge and i think lots of other things could take advantage of this helper method existing, but I guess this isn't a blocker
There was a problem hiding this comment.
I pulled in the changes for RowSignatures.complexTypeChecker
There was a problem hiding this comment.
I think the approach of #14249 which is adding the SqlAggFunction equivalent of OperatorConversions.OperatorBuilder is probably the better way to do this
jon-wei
force-pushed
the
variance_sql_fix
branch
from
7b390e8 to
79417b6
Compare
| } | ||
|
|
||
| /** | ||
| * Creates a SqlAggFunction that is the same as SqlAvgAggFunction but with an operand type that accepts |
…pache#14463) * Support complex variance object inputs for variance SQL agg function * Add test * Include complexTypeChecker, address PR comments * Checkstyle, javadoc link
3 participants
This PR fixes an issue with the
VARIANCEand related SQL aggregation functions, allowing them to accept variance aggregator objects as inputs (currently the SQL function only accepts numeric operands).The new
SqlAggFunctiondefinitions are the same asSqlAvgAggFunctionfromSqlStdOperatorTablebut with the operand type broadened to include variance aggregator objects in addition to numerics.Note: This pulls in the
RowSignatures.complexTypeCheckerchanges from #14195 for tighter operand type checkingThis PR has: